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

[Gaëtan Rivet]

On Mon, Aug 21, 2017 at 12:02:44PM +0000, Matan Azrad wrote:
> 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", &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.
> 

This is not a performance-critical section. The latency is irrelevant
because this code is executed once, before any packet processing has
started.

The only focus is on correctness and simplicity.

> > > +			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.
> 

Same as above: performance is irrelevant at this point.

> Is it OK for you to use it as is?
> 

I'm ok with the simplest solution that fixes the bug :) .

> > 
> > >  	snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
> > >  	return 0;
> > >  }
> > > --
> > > 2.7.4
> > >

-- 
Gaëtan Rivet
6WIND