Re: [dpdk-dev] [PATCH v2] net/failsafe: fix parameters parsing

[Matan Azrad]

Hi Gaetan

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.ri...@6wind.com]
> Sent: Monday, August 21, 2017 12:34 PM
> To: Matan Azrad <ma...@mellanox.com>
> Cc: dev@dpdk.org; sta...@dpdk.org
> Subject: Re: [PATCH v2] net/failsafe: fix parameters parsing
> 
> Hi Matan,
> 
> On Sun, Aug 20, 2017 at 01:07:23AM +0300, Matan Azrad wrote:
> > The corrupted code used wrongly snprintf return value as the number of
> > characters actually copied, in spite of the meanning is the number of
> > characters which would be generated for the given input.
> >
> > It caused to remain zerod bytes between the failsafe command line non
> > sub device parameters indicates end of string.
> >
> > Hence, when rte_kvargs_parse tried to parse all parameters, it got end
> > of string after the first one and the others weren't parsed.
> >
> > So, if the mac parameters was the first in command line it was taken
> > while hotplug_poll was left default, and vice versa.
> >
> > The fix updates the buffer index by dedicated variable contains the
> > copy size, by the way validates the last comma removing.
> >
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Matan Azrad <ma...@mellanox.com>
> > ---
> >  drivers/net/failsafe/failsafe_args.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/failsafe/failsafe_args.c
> > b/drivers/net/failsafe/failsafe_args.c
> > index 1f22416..2a5760a 100644
> > --- a/drivers/net/failsafe/failsafe_args.c
> > +++ b/drivers/net/failsafe/failsafe_args.c
> > @@ -286,10 +286,14 @@ fs_remove_sub_devices_definition(char
> params[DEVARGS_MAXLEN])
> >                     ERROR("Invalid parameter");
> >                     return -EINVAL;
> >             }
> > -           if (params[b] == ',' || params[b] == '\0')
> > -                   i += snprintf(&buffer[i], b - a + 1, "%s", &params[a]);
> > -           if (params[b] == '(') {
> > +           if (params[b] == ',' || params[b] == '\0') {
> > +                   size_t len = b - a + 1;
> > +
> > +                   snprintf(&buffer[i], len, "%s", &params[a]);
> 
> There it should be:
> 
> snprintf(&buffer[i], len + 1, "%s", &params[a]);
> 

You right - will be fixed in V3.

> This is due to the use of snprintf intead of memcpy. It illustrates actually 
> why
> the overhead of using snprintf is worth it, as it would exactly be the 
> situation
> where memcpy would copy the last character as a comma and rely on buffer
> being clean (well, it is, but that's beside the point :).
> 

I don't think so.
We know where is the end of string in this branch - so don't need snprintf for 
safety.
Actually, it illustrates why in this case we should use memcpy :)  

> If for any reason (performance? Lunacy?) someone wanted to change the
> initialization of buffer (for example, directly working on params instead of
> copying in a temporary buffer first, or simply removing the "= {0};" above
> because he is a maniac), then this chunk of code would become unsafe
> without snprintf.
> 
For performance\latency I think this one will replace snprintf too.
Even for "={0}" removing the branch in the end of function I added(i>0) covers 
it.

> > +                   i += len;
> > +           } else if (params[b] == '(') {
> >                     size_t start = b;
> > +
> >                     b += closing_paren(&params[b]);
> >                     if (b == start)
> >                             return -EINVAL;
> > @@ -300,6 +304,9 @@ fs_remove_sub_devices_definition(char
> params[DEVARGS_MAXLEN])
> >             a = b + 1;
> >     }
> >  out:
> > +   if (i > 0)
> > +           /* For last comma preventing. */
> > +           buffer[i - 1] = '\0';
> 
> I don't think there is a simple way to keep this in the kvarg branch (that
> would involve precomputing the final length of i and checking that we
> reached it within said branch -- pretty ugly).
> 
> You would have had to send a v3 anyway due to the OB1 error above.
> 
> Compare with this version:
> 
> 
>                 if (params[b] == ',' || params[b] == '\0') {
>                         size_t param_len = b - a;
> 
>                         if (i > 0)
>                                 param_len += 1;
>                         snprintf(&buffer[i], param_len + 1, "%s%s",
>                                  i ? "," : "", &params[a]);
>                         i += param_len;
>                 }
> 
> The only added logic is the `a ? "," : ""` conditional, and it allows to keep 
> all
> changes within the kvarg branch, avoiding scattering output string formatting
> logic.
> 
> Please use this instead in your v3.

But you use these 2 branches per parameter, 
I suggest only one branch for any parameters number.

Take example of 2 non sub device parameters:
You suggest 4 branches and I suggest only 1 to handle the last comma removing.

Is it OK for you to use it as is?

> 
> >     snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
> >     return 0;
> >  }
> > --
> > 2.7.4
> >
> 
> --
> Gaëtan Rivet
> 6WIND