[dpdk-dev] [PATCH v2] net/failsafe: fix parameters parsing
Matan Azrad
matan at mellanox.com
Mon Aug 21 14:02:44 CEST 2017
Hi Gaetan
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet at 6wind.com]
> Sent: Monday, August 21, 2017 12:34 PM
> To: Matan Azrad <matan at mellanox.com>
> Cc: dev at dpdk.org; stable at 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: stable at dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan at 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", ¶ms[a]);
> > - if (params[b] == '(') {
> > + if (params[b] == ',' || params[b] == '\0') {
> > + size_t len = b - a + 1;
> > +
> > + snprintf(&buffer[i], len, "%s", ¶ms[a]);
>
> There it should be:
>
> snprintf(&buffer[i], len + 1, "%s", ¶ms[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(¶ms[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 ? "," : "", ¶ms[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
More information about the dev
mailing list