Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

["Etelson, Gregory" <getelson@nvidia.com>]

Hello Ferruh,

[:snip:]

>> *** stack smashing detected ***: terminated
>>
>> The corruption occurred in `parse_int()` called from
>> `parse_indlst_id2ptr()`.
>>
>> Inside `parse_int()` the arg parameter referenced 8 bytes of memory
>> while the target buffer was 4 bytes allocated on caller optimized stack:
>>
>> (gdb) p *arg
>> $1 = { ... size = 8, ...}
>>
>
> Thanks Gregory, I can see the problem now.
>

[: Thumbs up :]

[:snip:]

> BUT back to the root cause of the problem,
> `parse_int()` tries to be generic and it support different size of
> variables [1], but it fails on this.
>
> `parse_int()` gets 'size' as argument, but it doesn't use parameter
> value, instead overwrites it with 'size = arg->size;' and uses this
> value, in this case when context provides larger variable size than what
> `parse_int()` gets as parameter, the problem you observed occurs.
>
> What do you think to use 'size' from parameter list, as it is intended,
> instead of using 'arg->size'?
> Or perhaps use 'buf' and 'size' from parameter if they are valid, else
> get the from context/arg [2]?
> I think this solves your problem, can you please verify it?
>
> btw, 'buf' usage is a little more complex, since `parse_int()` checks
> for "ctx->object != NULL" to continue, I can't really be sure about
> intention there, but please check usage in `parse_port()`, it looks like
> when 'buf' provided expectation is to get parsed value in the 'buf'.
>
>
>
>
> [1]
>  switch (size) {
>  case sizeof(uint8_t):
>    ...
>  case sizeof(uint16_t):
>    ...
>  ...
>  case sizeof(uint64_t):
>    ...
>
>
> [2]
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 0d521159e97d..cd123c69265e 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -10805,8 +10805,10 @@ parse_int(struct context *ctx, const struct
> token *token,
>                         goto error;
>                 return len;
>         }
> -       buf = (uint8_t *)ctx->object + arg->offset;
> -       size = arg->size;
> +       if (buf == NULL || size == 0) {
> +               buf = (uint8_t *)ctx->object + arg->offset;
> +               size = arg->size;
> +       }
>         if (u > RTE_LEN2MASK(size * CHAR_BIT, uint64_t))
>                 return -1;
>  objmask:
>

About the fault root cause.
There were 2 uncoupled resources in that case: static token size and 
variable size passed to parse_int().
parse_int() caller must provide a buffer large enough for token size.
Otherwise parse_int() will corrupt memory outside the input buffer.

In the generic solution parse_int() caller allocates target buffer using 
existing knowledge about input token size.

Testpmd add_port() imitates the ARGS_ENTRY() macro that extrapolates 
target buffer size from RTE structure member.

Current testpmd cannot use that approach directly because indirect action 
references internal testpmd ID.

Testpmd indirect ID has no defined type or token that leads to indirect ID 
parser.

As a solution, testpmd can provide centralized parser function for all 
indirect IDs. The function will parse ID value and use the token as the 
key to indirect database search:

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index ce71818705..7fb3b61d37 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c

+typedef uint32_t indirect_id_t;
+
+static int
+parse_indirect_index(struct context *ctx, const struct token *token,
+               const char *str, unsigned int len,
+               void *buf, unsigned int size)
+{
+       indirect_id_t id;
+
+       ctx->object = &id;
+       parse_int(ctx, token, str, len, ctx->object, sizeof(id));
+
+       switch (ctx->curr) {
+       case INDIRECT_ACTION_ID2PTR:
+               /**/
+               break;
+       case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
+               /**/
+               break;
+       case INDIRECT_LIST_ACTION_ID2PTR_CONF:
+               /**/
+               break;
+       default:
+               /**/
+       }
+
+
+}
+

Regards,
Gregory