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

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

[Matan Azrad]

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 uses memcpy instead of snprintf
which is good enouth for this copy scenario.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/failsafe/failsafe_args.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
index 1f22416..0a98b04 100644
--- a/drivers/net/failsafe/failsafe_args.c
+++ b/drivers/net/failsafe/failsafe_args.c
@@ -271,7 +271,7 @@ static int
 fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])
 {
 	char buffer[DEVARGS_MAXLEN] = {0};
-	size_t a, b;
+	size_t a, b, temp;
 	int i;
 
 	a = 0;
@@ -286,12 +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] == '(') {
-			size_t start = b;
+		if (params[b] == ',' || params[b] == '\0') {
+			temp = b - a + 1;
+			memcpy(&buffer[i], &params[a], temp);
+			i += temp;
+		} else if (params[b] == '(') {
+			temp = b;
 			b += closing_paren(&params[b]);
-			if (b == start)
+			if (b == temp)
 				return -EINVAL;
 			b += 1;
 			if (params[b] == '\0')
-- 
2.7.4

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

[Gaëtan Rivet]

Hi Matan,

Thanks for spotting the bug.

On Thu, Aug 17, 2017 at 05:19:41PM +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 uses memcpy instead of snprintf
> which is good enouth for this copy scenario.

Actually snprintf should still be used.

> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/failsafe/failsafe_args.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
> index 1f22416..0a98b04 100644
> --- a/drivers/net/failsafe/failsafe_args.c
> +++ b/drivers/net/failsafe/failsafe_args.c
> @@ -271,7 +271,7 @@ static int
>  fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])
>  {
>  	char buffer[DEVARGS_MAXLEN] = {0};
> -	size_t a, b;
> +	size_t a, b, temp;

temp is not descriptive enough.
You are declaring this variable here because you want to re-use it
instead of `start`. This is an overreach however, this fix must be
restricted to the actual bug.

>  	int i;
>  
>  	a = 0;
> @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])
>  			ERROR("Invalid parameter");
>  			return -EINVAL;
>  		}
> -		if (params[b] == ',' || params[b] == '\0')

Declare the temporary variable in this scope, with a descriptive name
such as "len", "length", "param_len" or something close.

> -			i += snprintf(&buffer[i], b - a + 1, "%s", &params[a]);
> -		if (params[b] == '(') {
> -			size_t start = b;
> +		if (params[b] == ',' || params[b] == '\0') {
> +			temp = b - a + 1;

The value here should be "b - a".
If a != 0 however, then we are parsing a new parameter and buffer
already contains at least one. A comma should be added to separate the
two.

An example might clarify what I mean:

                if (params[b] == ',' || params[b] == '\0') {
                        size_t param_len = b - a;

                        if (a)
                                param_len += 1;
                        snprintf(&buffer[i], param_len + 1, "%s%s",
                                 a ? "," : "", &params[a]);
                        i += param_len;
                }

The conditionals about `a` are ugly however, if you find a better way to write
those you are most welcome :).

> +			memcpy(&buffer[i], &params[a], temp);
> +			i += temp;
> +		} else if (params[b] == '(') {
> +			temp = b;
>  			b += closing_paren(&params[b]);
> -			if (b == start)

The changes should be limited to the actual bug. No need to change this.

> +			if (b == temp)
>  				return -EINVAL;
>  			b += 1;
>  			if (params[b] == '\0')
> -- 
> 2.7.4
> 

Thanks again for the debug and sorry for being nitpicky.

-- 
Gaëtan Rivet
6WIND

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

[Matan Azrad]

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Thursday, August 17, 2017 6:26 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> 
> Hi Matan,
> 
> Thanks for spotting the bug.
> 
> On Thu, Aug 17, 2017 at 05:19:41PM +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 uses memcpy instead of snprintf which is good
> > enouth for this copy scenario.
> 
> Actually snprintf should still be used.
> 
Why?
We just want to copy from buffer to buffer, no needs snprintf overheads
If actually we are not using complicated format.

> >
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> >  drivers/net/failsafe/failsafe_args.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/failsafe/failsafe_args.c
> > b/drivers/net/failsafe/failsafe_args.c
> > index 1f22416..0a98b04 100644
> > --- a/drivers/net/failsafe/failsafe_args.c
> > +++ b/drivers/net/failsafe/failsafe_args.c
> > @@ -271,7 +271,7 @@ static int
> >  fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])  {
> >  	char buffer[DEVARGS_MAXLEN] = {0};
> > -	size_t a, b;
> > +	size_t a, b, temp;
> 
> temp is not descriptive enough.

What are about a, b, i ?
 
> You are declaring this variable here because you want to re-use it instead of
> `start`. This is an overreach however, this fix must be restricted to the actual
> bug.

temp is helping to address the original bug, don't we want to reuse variable 
Instead of 2  if statement variables, maybe other name for all?

Something like:
a=> start
b=> end
i => next
temp=> saved_val
 

> 
> >  	int i;
> >
> >  	a = 0;
> > @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char
> params[DEVARGS_MAXLEN])
> >  			ERROR("Invalid parameter");
> >  			return -EINVAL;
> >  		}
> > -		if (params[b] == ',' || params[b] == '\0')
> 
> Declare the temporary variable in this scope, with a descriptive name such as
> "len", "length", "param_len" or something close.
> 
> > -			i += snprintf(&buffer[i], b - a + 1, "%s", &params[a]);
> > -		if (params[b] == '(') {
> > -			size_t start = b;
> > +		if (params[b] == ',' || params[b] == '\0') {
> > +			temp = b - a + 1;
> 
> The value here should be "b - a".
> If a != 0 however, then we are parsing a new parameter and buffer already
> contains at least one. A comma should be added to separate the two.
> 
> An example might clarify what I mean:
> 
>                 if (params[b] == ',' || params[b] == '\0') {
>                         size_t param_len = b - a;
> 
>                         if (a)
>                                 param_len += 1;
>                         snprintf(&buffer[i], param_len + 1, "%s%s",
>                                  a ? "," : "", &params[a]);
>                         i += param_len;
>                 }
> 
> The conditionals about `a` are ugly however, if you find a better way to write
> those you are most welcome :).
> 
> > +			memcpy(&buffer[i], &params[a], temp);
> > +			i += temp;
> > +		} else if (params[b] == '(') {
> > +			temp = b;
> >  			b += closing_paren(&params[b]);
> > -			if (b == start)
> 

I think the last comma is harmless for next parse
But I can just change the last coma to '\0' in the end of function(if exists).
But, This solves another issue, don't it?  Maybe in different patch?

> The changes should be limited to the actual bug. No need to change this.
> 
> > +			if (b == temp)
> >  				return -EINVAL;
> >  			b += 1;
> >  			if (params[b] == '\0')
> > --
> > 2.7.4
> >
> 
> Thanks again for the debug and sorry for being nitpicky.
> 
> --
> Gaëtan Rivet
> 6WIND

Regards,
Matan Azrad.

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

[Gaëtan Rivet]

On Thu, Aug 17, 2017 at 03:54:23PM +0000, Matan Azrad wrote:
> 
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Thursday, August 17, 2017 6:26 PM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> > Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> > 
> > Hi Matan,
> > 
> > Thanks for spotting the bug.
> > 
> > On Thu, Aug 17, 2017 at 05:19:41PM +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 uses memcpy instead of snprintf which is good
> > > enouth for this copy scenario.
> > 
> > Actually snprintf should still be used.
> > 
> Why?
> We just want to copy from buffer to buffer, no needs snprintf overheads
> If actually we are not using complicated format.
> 

snprintf plays nice with strings and ensure that it will be terminated
by a '\0'. It is generally preferable, particularly considering that the
parameter string we are building here is meant to be parsed by another
lib (rte_kvargs).

> > >
> > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> > >  drivers/net/failsafe/failsafe_args.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/failsafe/failsafe_args.c
> > > b/drivers/net/failsafe/failsafe_args.c
> > > index 1f22416..0a98b04 100644
> > > --- a/drivers/net/failsafe/failsafe_args.c
> > > +++ b/drivers/net/failsafe/failsafe_args.c
> > > @@ -271,7 +271,7 @@ static int
> > >  fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])  {
> > >  	char buffer[DEVARGS_MAXLEN] = {0};
> > > -	size_t a, b;
> > > +	size_t a, b, temp;
> > 
> > temp is not descriptive enough.
> 
> What are about a, b, i ?
>  
> > You are declaring this variable here because you want to re-use it instead of
> > `start`. This is an overreach however, this fix must be restricted to the actual
> > bug.
> 
> temp is helping to address the original bug, don't we want to reuse variable 
> Instead of 2  if statement variables, maybe other name for all?
> 

The compiler should be able to see that their use does not overlap.

The issue is that temp is meant to describe a length limit in one branch
and be a marker of a starting point in another. Thus both variable
should be named differently to make the intent clear.

When declaring temp (or saved_val) in the higher-scope, you are thus
forced to use generic, non-descriptive name precisely because you are
sharing the variable between two different uses.

> Something like:
> a=> start
> b=> end
> i => next
> temp=> saved_val
>  
> 
> > 
> > >  	int i;
> > >
> > >  	a = 0;
> > > @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char
> > params[DEVARGS_MAXLEN])
> > >  			ERROR("Invalid parameter");
> > >  			return -EINVAL;
> > >  		}
> > > -		if (params[b] == ',' || params[b] == '\0')
> > 
> > Declare the temporary variable in this scope, with a descriptive name such as
> > "len", "length", "param_len" or something close.
> > 
> > > -			i += snprintf(&buffer[i], b - a + 1, "%s", &params[a]);
> > > -		if (params[b] == '(') {
> > > -			size_t start = b;
> > > +		if (params[b] == ',' || params[b] == '\0') {
> > > +			temp = b - a + 1;
> > 
> > The value here should be "b - a".
> > If a != 0 however, then we are parsing a new parameter and buffer already
> > contains at least one. A comma should be added to separate the two.
> > 
> > An example might clarify what I mean:
> > 
> >                 if (params[b] == ',' || params[b] == '\0') {
> >                         size_t param_len = b - a;
> > 
> >                         if (a)
> >                                 param_len += 1;
> >                         snprintf(&buffer[i], param_len + 1, "%s%s",
> >                                  a ? "," : "", &params[a]);
> >                         i += param_len;
> >                 }
> > 
> > The conditionals about `a` are ugly however, if you find a better way to write
> > those you are most welcome :).
> > 
> > > +			memcpy(&buffer[i], &params[a], temp);
> > > +			i += temp;
> > > +		} else if (params[b] == '(') {
> > > +			temp = b;
> > >  			b += closing_paren(&params[b]);
> > > -			if (b == start)
> > 
> 
> I think the last comma is harmless for next parse
> But I can just change the last coma to '\0' in the end of function(if exists).
> But, This solves another issue, don't it?  Maybe in different patch?
> 

It might be harmless, but I prefer having a clean output.
Which other issue? Handling the commas seems in line with
properly copying regular parameters.

You could certainly check for the presence of a comma, but you already
see how ugly the code flow would be ("if it exists"), and you will keep a
dirty string between two loops, with possibly other branches wanting to
return on error.

I prefer having a compact, self-contained logic addressing kvargs which
should not leak conceptual gotcha that the next programmer would need to
be wary of when trying to touch the code in other parts of the function.

> > The changes should be limited to the actual bug. No need to change this.
> > 
> > > +			if (b == temp)
> > >  				return -EINVAL;
> > >  			b += 1;
> > >  			if (params[b] == '\0')
> > > --
> > > 2.7.4
> > >
> > 
> > Thanks again for the debug and sorry for being nitpicky.
> > 
> > --
> > Gaëtan Rivet
> > 6WIND
> 
> Regards,
> Matan Azrad.

-- 
Gaëtan Rivet
6WIND

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

[Matan Azrad]

Hi Gaetan

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Thursday, August 17, 2017 7:25 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> 
> On Thu, Aug 17, 2017 at 03:54:23PM +0000, Matan Azrad wrote:
> >
> >
> > > -----Original Message-----
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > Sent: Thursday, August 17, 2017 6:26 PM
> > > To: Matan Azrad <matan@mellanox.com>
> > > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> > > Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> > >
> > > Hi Matan,
> > >
> > > Thanks for spotting the bug.
> > >
> > > On Thu, Aug 17, 2017 at 05:19:41PM +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 uses memcpy instead of snprintf which is
> > > > good enouth for this copy scenario.
> > >
> > > Actually snprintf should still be used.
> > >
> > Why?
> > We just want to copy from buffer to buffer, no needs snprintf
> > overheads If actually we are not using complicated format.
> >
> 
> snprintf plays nice with strings and ensure that it will be terminated by a '\0'.
> It is generally preferable, particularly considering that the parameter string
> we are building here is meant to be parsed by another lib (rte_kvargs).
>
In this specific scenario it is not needed because we for sure know that the length doesn't pass after '\0'.
By using memcpy we can prevent format parsing and other checks done by snprintf.

> > > >
> > > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > > ---
> > > >  drivers/net/failsafe/failsafe_args.c | 14 ++++++++------
> > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/net/failsafe/failsafe_args.c
> > > > b/drivers/net/failsafe/failsafe_args.c
> > > > index 1f22416..0a98b04 100644
> > > > --- a/drivers/net/failsafe/failsafe_args.c
> > > > +++ b/drivers/net/failsafe/failsafe_args.c
> > > > @@ -271,7 +271,7 @@ static int
> > > >  fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])
> {
> > > >  	char buffer[DEVARGS_MAXLEN] = {0};
> > > > -	size_t a, b;
> > > > +	size_t a, b, temp;
> > >
> > > temp is not descriptive enough.
> >
> > What are about a, b, i ?
> >
> > > You are declaring this variable here because you want to re-use it
> > > instead of `start`. This is an overreach however, this fix must be
> > > restricted to the actual bug.
> >
> > temp is helping to address the original bug, don't we want to reuse
> > variable Instead of 2  if statement variables, maybe other name for all?
> >
> 
> The compiler should be able to see that their use does not overlap.
> 

This is new for me, thanks for the lesson.
I will update it to len & start as you suggested.

> The issue is that temp is meant to describe a length limit in one branch and
> be a marker of a starting point in another. Thus both variable should be
> named differently to make the intent clear.
> 
> When declaring temp (or saved_val) in the higher-scope, you are thus forced
> to use generic, non-descriptive name precisely because you are sharing the
> variable between two different uses.
> 
> > Something like:
> > a=> start
> > b=> end
> > i => next
> > temp=> saved_val
> >
> >
You didn't answer what are about my suggestions for new names instead of 
a,b and I which are not descriptive too (like temp :))

> > >
> > > >  	int i;
> > > >
> > > >  	a = 0;
> > > > @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char
> > > params[DEVARGS_MAXLEN])
> > > >  			ERROR("Invalid parameter");
> > > >  			return -EINVAL;
> > > >  		}
> > > > -		if (params[b] == ',' || params[b] == '\0')
> > >
> > > Declare the temporary variable in this scope, with a descriptive
> > > name such as "len", "length", "param_len" or something close.
> > >
> > > > -			i += snprintf(&buffer[i], b - a + 1, "%s", &params[a]);
> > > > -		if (params[b] == '(') {
> > > > -			size_t start = b;
> > > > +		if (params[b] == ',' || params[b] == '\0') {
> > > > +			temp = b - a + 1;
> > >
> > > The value here should be "b - a".
> > > If a != 0 however, then we are parsing a new parameter and buffer
> > > already contains at least one. A comma should be added to separate the
> two.
> > >
> > > An example might clarify what I mean:
> > >
> > >                 if (params[b] == ',' || params[b] == '\0') {
> > >                         size_t param_len = b - a;
> > >
> > >                         if (a)
> > >                                 param_len += 1;
> > >                         snprintf(&buffer[i], param_len + 1, "%s%s",
> > >                                  a ? "," : "", &params[a]);
> > >                         i += param_len;
> > >                 }
> > >
> > > The conditionals about `a` are ugly however, if you find a better
> > > way to write those you are most welcome :).
> > >
> > > > +			memcpy(&buffer[i], &params[a], temp);
> > > > +			i += temp;
> > > > +		} else if (params[b] == '(') {
> > > > +			temp = b;
> > > >  			b += closing_paren(&params[b]);
> > > > -			if (b == start)
> > >
> >
> > I think the last comma is harmless for next parse But I can just
> > change the last coma to '\0' in the end of function(if exists).
> > But, This solves another issue, don't it?  Maybe in different patch?
> >
> 
> It might be harmless, but I prefer having a clean output.
> Which other issue? Handling the commas seems in line with properly copying
> regular parameters.

Yes, but it is another issue in this specific copy (don't refer to snprintf return value) 
But, it's OK, I think them both can be handle by this patch :)

> 
> You could certainly check for the presence of a comma, but you already see
> how ugly the code flow would be ("if it exists"), and you will keep a dirty
> string between two loops, with possibly other branches wanting to return on
> error.
> 
> I prefer having a compact, self-contained logic addressing kvargs which
> should not leak conceptual gotcha that the next programmer would need to
> be wary of when trying to touch the code in other parts of the function.
>

If we leave always b-a+1 we potentially can get the next buffer strings:
p1,p2...pN,
or 
p1,p2,pN(\0)
 while i is the index of the character after a comma or '\0'

so, for prevent the last comma we can override always in the end of function(not between 2 loops) '\0' like this:
if (i >0 )
/* For prevent last comma. */
	buffer[i-1] = '\0';
instead of using 2 conditions per non sub device param - 'if(a)' and 'a ? "," : ""'
I think each future programmer can easily understand it.

 
> > > The changes should be limited to the actual bug. No need to change this.
> > >
> > > > +			if (b == temp)
> > > >  				return -EINVAL;
> > > >  			b += 1;
> > > >  			if (params[b] == '\0')
> > > > --
> > > > 2.7.4
> > > >
> > >
> > > Thanks again for the debug and sorry for being nitpicky.
> > >
> > > --
> > > Gaëtan Rivet
> > > 6WIND
> >
> > Regards,
> > Matan Azrad.
> 
> --
> Gaëtan Rivet
> 6WIND


Regards
Matan Azrad.

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

[Gaëtan Rivet]

On Thu, Aug 17, 2017 at 06:52:48PM +0000, Matan Azrad wrote:
> Hi Gaetan
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Thursday, August 17, 2017 7:25 PM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> > Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> > 
> > On Thu, Aug 17, 2017 at 03:54:23PM +0000, Matan Azrad wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > > Sent: Thursday, August 17, 2017 6:26 PM
> > > > To: Matan Azrad <matan@mellanox.com>
> > > > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> > > > Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> > > >
> > > > Hi Matan,
> > > >
> > > > Thanks for spotting the bug.
> > > >
> > > > On Thu, Aug 17, 2017 at 05:19:41PM +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 uses memcpy instead of snprintf which is
> > > > > good enouth for this copy scenario.
> > > >
> > > > Actually snprintf should still be used.
> > > >
> > > Why?
> > > We just want to copy from buffer to buffer, no needs snprintf
> > > overheads If actually we are not using complicated format.
> > >
> > 
> > snprintf plays nice with strings and ensure that it will be terminated by a '\0'.
> > It is generally preferable, particularly considering that the parameter string
> > we are building here is meant to be parsed by another lib (rte_kvargs).
> >
> In this specific scenario it is not needed because we for sure know that the length doesn't pass after '\0'.
> By using memcpy we can prevent format parsing and other checks done by snprintf.
> 

This overhead is negligible.
snprintf is preferred over memcpy / strncpy in DPDK.

> > > > >
> > > > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > > > ---
> > > > >  drivers/net/failsafe/failsafe_args.c | 14 ++++++++------
> > > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/failsafe/failsafe_args.c
> > > > > b/drivers/net/failsafe/failsafe_args.c
> > > > > index 1f22416..0a98b04 100644
> > > > > --- a/drivers/net/failsafe/failsafe_args.c
> > > > > +++ b/drivers/net/failsafe/failsafe_args.c
> > > > > @@ -271,7 +271,7 @@ static int
> > > > >  fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN])
> > {
> > > > >  	char buffer[DEVARGS_MAXLEN] = {0};
> > > > > -	size_t a, b;
> > > > > +	size_t a, b, temp;
> > > >
> > > > temp is not descriptive enough.
> > >
> > > What are about a, b, i ?
> > >
> > > > You are declaring this variable here because you want to re-use it
> > > > instead of `start`. This is an overreach however, this fix must be
> > > > restricted to the actual bug.
> > >
> > > temp is helping to address the original bug, don't we want to reuse
> > > variable Instead of 2  if statement variables, maybe other name for all?
> > >
> > 
> > The compiler should be able to see that their use does not overlap.
> > 
> 
> This is new for me, thanks for the lesson.
> I will update it to len & start as you suggested.
> 
> > The issue is that temp is meant to describe a length limit in one branch and
> > be a marker of a starting point in another. Thus both variable should be
> > named differently to make the intent clear.
> > 
> > When declaring temp (or saved_val) in the higher-scope, you are thus forced
> > to use generic, non-descriptive name precisely because you are sharing the
> > variable between two different uses.
> > 
> > > Something like:
> > > a=> start
> > > b=> end
> > > i => next
> > > temp=> saved_val
> > >
> > >
> You didn't answer what are about my suggestions for new names instead of 
> a,b and I which are not descriptive too (like temp :))
> 

Sure :) . Had I reviewed myself I would probably have asked for better names.
I was writing the parsing utilities all at once and used the same conventions
throughout failsafe_args.c

There might be a patch at some point changing all those names.
But I think this is going beyond the scope of fixing the isolation of
kvargs. My point there is only to avoid making things worse than they
are.

> > > >
> > > > >  	int i;
> > > > >
> > > > >  	a = 0;
> > > > > @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char
> > > > params[DEVARGS_MAXLEN])
> > > > >  			ERROR("Invalid parameter");
> > > > >  			return -EINVAL;
> > > > >  		}
> > > > > -		if (params[b] == ',' || params[b] == '\0')
> > > >
> > > > Declare the temporary variable in this scope, with a descriptive
> > > > name such as "len", "length", "param_len" or something close.
> > > >
> > > > > -			i += snprintf(&buffer[i], b - a + 1, "%s", &params[a]);
> > > > > -		if (params[b] == '(') {
> > > > > -			size_t start = b;
> > > > > +		if (params[b] == ',' || params[b] == '\0') {
> > > > > +			temp = b - a + 1;
> > > >
> > > > The value here should be "b - a".
> > > > If a != 0 however, then we are parsing a new parameter and buffer
> > > > already contains at least one. A comma should be added to separate the
> > two.
> > > >
> > > > An example might clarify what I mean:
> > > >
> > > >                 if (params[b] == ',' || params[b] == '\0') {
> > > >                         size_t param_len = b - a;
> > > >
> > > >                         if (a)
> > > >                                 param_len += 1;
> > > >                         snprintf(&buffer[i], param_len + 1, "%s%s",
> > > >                                  a ? "," : "", &params[a]);
> > > >                         i += param_len;
> > > >                 }
> > > >
> > > > The conditionals about `a` are ugly however, if you find a better
> > > > way to write those you are most welcome :).
> > > >
> > > > > +			memcpy(&buffer[i], &params[a], temp);
> > > > > +			i += temp;
> > > > > +		} else if (params[b] == '(') {
> > > > > +			temp = b;
> > > > >  			b += closing_paren(&params[b]);
> > > > > -			if (b == start)
> > > >
> > >
> > > I think the last comma is harmless for next parse But I can just
> > > change the last coma to '\0' in the end of function(if exists).
> > > But, This solves another issue, don't it?  Maybe in different patch?
> > >
> > 
> > It might be harmless, but I prefer having a clean output.
> > Which other issue? Handling the commas seems in line with properly copying
> > regular parameters.
> 
> Yes, but it is another issue in this specific copy (don't refer to snprintf return value) 
> But, it's OK, I think them both can be handle by this patch :)
> 
> > 
> > You could certainly check for the presence of a comma, but you already see
> > how ugly the code flow would be ("if it exists"), and you will keep a dirty
> > string between two loops, with possibly other branches wanting to return on
> > error.
> > 
> > I prefer having a compact, self-contained logic addressing kvargs which
> > should not leak conceptual gotcha that the next programmer would need to
> > be wary of when trying to touch the code in other parts of the function.
> >
> 
> If we leave always b-a+1 we potentially can get the next buffer strings:
> p1,p2...pN,
> or 
> p1,p2,pN(\0)
>  while i is the index of the character after a comma or '\0'
> 
> so, for prevent the last comma we can override always in the end of function(not between 2 loops) '\0' like this:
> if (i >0 )
> /* For prevent last comma. */
> 	buffer[i-1] = '\0';
> instead of using 2 conditions per non sub device param - 'if(a)' and 'a ? "," : ""'
> I think each future programmer can easily understand it.
> 
>  

Sure, I agree that it would work. It would also probably be parseable.
But it depends on your implementation. I think you'll have to write this
`buffer[i-1] = '\0';` outside the branch dealing with proper kvargs, which
mean scattering within the rest of the function code that should be
dealing with how those kvargs are presented to the next stage.

I think the most constructive thing to do would be for you to write a v2
proposing your solution. If you are able to contain all logic
pertaining to the kvargs within this single branch along with the other
remarks (keeping snprintf, using a local "len" variable), then perfect
:). Otherwise, while imperfect I proposed an alternative that should be
able to do that.

> > > > The changes should be limited to the actual bug. No need to change this.
> > > >
> > > > > +			if (b == temp)
> > > > >  				return -EINVAL;
> > > > >  			b += 1;
> > > > >  			if (params[b] == '\0')
> > > > > --
> > > > > 2.7.4
> > > > >
> > > >
> > > > Thanks again for the debug and sorry for being nitpicky.
> > > >
> > > > --
> > > > Gaëtan Rivet
> > > > 6WIND
> > >
> > > Regards,
> > > Matan Azrad.
> > 
> > --
> > Gaëtan Rivet
> > 6WIND
> 
> 
> Regards
> Matan Azrad.

-- 
Gaëtan Rivet
6WIND

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

[Matan Azrad]

Hi Gaetan

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Friday, August 18, 2017 12:03 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> 
> On Thu, Aug 17, 2017 at 06:52:48PM +0000, Matan Azrad wrote:
> > Hi Gaetan
> >
> > > -----Original Message-----
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > Sent: Thursday, August 17, 2017 7:25 PM
> > > To: Matan Azrad <matan@mellanox.com>
> > > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> > > Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> > >
> > > On Thu, Aug 17, 2017 at 03:54:23PM +0000, Matan Azrad wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > > > Sent: Thursday, August 17, 2017 6:26 PM
> > > > > To: Matan Azrad <matan@mellanox.com>
> > > > > Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>;
> stable@dpdk.org
> > > > > Subject: Re: [PATCH] net/failsafe: fix parameters parsing
> > > > >
> > > > > Hi Matan,
> > > > >
> > > > > Thanks for spotting the bug.
> > > > >
> > > > > On Thu, Aug 17, 2017 at 05:19:41PM +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 uses memcpy instead of
> > > > > > snprintf which is good enouth for this copy scenario.
> > > > >
> > > > > Actually snprintf should still be used.
> > > > >
> > > > Why?
> > > > We just want to copy from buffer to buffer, no needs snprintf
> > > > overheads If actually we are not using complicated format.
> > > >
> > >
> > > snprintf plays nice with strings and ensure that it will be terminated by a
> '\0'.
> > > It is generally preferable, particularly considering that the
> > > parameter string we are building here is meant to be parsed by another
> lib (rte_kvargs).
> > >
> > In this specific scenario it is not needed because we for sure know that the
> length doesn't pass after '\0'.
> > By using memcpy we can prevent format parsing and other checks done by
> snprintf.
> >
> 
> This overhead is negligible.
> snprintf is preferred over memcpy / strncpy in DPDK.
> 

I don't think it is negligible (think only about the switch statement 
which should be used for format parsing).
In this case we know by sure that the copy size is not dangerous and
there is not justify to use snprintf function.

I think also if we wouldn't know the '\0' place,  it is preferred to use
strlen and memcpy instead of snprintf in any uncomplicated format cases.

Maybe it is behind to this fix(only enhancement) but I suggest you
to replace all failsafe snprintf usages for simple formats include this fix line
after this patch.

> > > > > >
> > > > > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > > > > > Cc: stable@dpdk.org
> > > > > >
> > > > > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > > > > ---
> > > > > >  drivers/net/failsafe/failsafe_args.c | 14 ++++++++------
> > > > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/failsafe/failsafe_args.c
> > > > > > b/drivers/net/failsafe/failsafe_args.c
> > > > > > index 1f22416..0a98b04 100644
> > > > > > --- a/drivers/net/failsafe/failsafe_args.c
> > > > > > +++ b/drivers/net/failsafe/failsafe_args.c
> > > > > > @@ -271,7 +271,7 @@ static int
> > > > > > fs_remove_sub_devices_definition(char
> params[DEVARGS_MAXLEN])
> > > {
> > > > > >  	char buffer[DEVARGS_MAXLEN] = {0};
> > > > > > -	size_t a, b;
> > > > > > +	size_t a, b, temp;
> > > > >
> > > > > temp is not descriptive enough.
> > > >
> > > > What are about a, b, i ?
> > > >
> > > > > You are declaring this variable here because you want to re-use
> > > > > it instead of `start`. This is an overreach however, this fix
> > > > > must be restricted to the actual bug.
> > > >
> > > > temp is helping to address the original bug, don't we want to
> > > > reuse variable Instead of 2  if statement variables, maybe other name
> for all?
> > > >
> > >
> > > The compiler should be able to see that their use does not overlap.
> > >
> >
> > This is new for me, thanks for the lesson.
> > I will update it to len & start as you suggested.
> >
> > > The issue is that temp is meant to describe a length limit in one
> > > branch and be a marker of a starting point in another. Thus both
> > > variable should be named differently to make the intent clear.
> > >
> > > When declaring temp (or saved_val) in the higher-scope, you are thus
> > > forced to use generic, non-descriptive name precisely because you
> > > are sharing the variable between two different uses.
> > >
> > > > Something like:
> > > > a=> start
> > > > b=> end
> > > > i => next
> > > > temp=> saved_val
> > > >
> > > >
> > You didn't answer what are about my suggestions for new names instead
> > of a,b and I which are not descriptive too (like temp :))
> >
> 
> Sure :) . Had I reviewed myself I would probably have asked for better
> names.
> I was writing the parsing utilities all at once and used the same conventions
> throughout failsafe_args.c
> 
> There might be a patch at some point changing all those names.
> But I think this is going beyond the scope of fixing the isolation of kvargs. My
> point there is only to avoid making things worse than they are.

By sure this patch is not making  the things worse than they are(even v1) :)
> 
> > > > >
> > > > > >  	int i;
> > > > > >
> > > > > >  	a = 0;
> > > > > > @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char
> > > > > params[DEVARGS_MAXLEN])
> > > > > >  			ERROR("Invalid parameter");
> > > > > >  			return -EINVAL;
> > > > > >  		}
> > > > > > -		if (params[b] == ',' || params[b] == '\0')
> > > > >
> > > > > Declare the temporary variable in this scope, with a descriptive
> > > > > name such as "len", "length", "param_len" or something close.
> > > > >
> > > > > > -			i += snprintf(&buffer[i], b - a + 1, "%s",
> &params[a]);
> > > > > > -		if (params[b] == '(') {
> > > > > > -			size_t start = b;
> > > > > > +		if (params[b] == ',' || params[b] == '\0') {
> > > > > > +			temp = b - a + 1;
> > > > >
> > > > > The value here should be "b - a".
> > > > > If a != 0 however, then we are parsing a new parameter and
> > > > > buffer already contains at least one. A comma should be added to
> > > > > separate the
> > > two.
> > > > >
> > > > > An example might clarify what I mean:
> > > > >
> > > > >                 if (params[b] == ',' || params[b] == '\0') {
> > > > >                         size_t param_len = b - a;
> > > > >
> > > > >                         if (a)
> > > > >                                 param_len += 1;
> > > > >                         snprintf(&buffer[i], param_len + 1, "%s%s",
> > > > >                                  a ? "," : "", &params[a]);
> > > > >                         i += param_len;
> > > > >                 }
> > > > >
> > > > > The conditionals about `a` are ugly however, if you find a
> > > > > better way to write those you are most welcome :).
> > > > >
> > > > > > +			memcpy(&buffer[i], &params[a], temp);
> > > > > > +			i += temp;
> > > > > > +		} else if (params[b] == '(') {
> > > > > > +			temp = b;
> > > > > >  			b += closing_paren(&params[b]);
> > > > > > -			if (b == start)
> > > > >
> > > >
> > > > I think the last comma is harmless for next parse But I can just
> > > > change the last coma to '\0' in the end of function(if exists).
> > > > But, This solves another issue, don't it?  Maybe in different patch?
> > > >
> > >
> > > It might be harmless, but I prefer having a clean output.
> > > Which other issue? Handling the commas seems in line with properly
> > > copying regular parameters.
> >
> > Yes, but it is another issue in this specific copy (don't refer to
> > snprintf return value) But, it's OK, I think them both can be handle
> > by this patch :)
> >
> > >
> > > You could certainly check for the presence of a comma, but you
> > > already see how ugly the code flow would be ("if it exists"), and
> > > you will keep a dirty string between two loops, with possibly other
> > > branches wanting to return on error.
> > >
> > > I prefer having a compact, self-contained logic addressing kvargs
> > > which should not leak conceptual gotcha that the next programmer
> > > would need to be wary of when trying to touch the code in other parts of
> the function.
> > >
> >
> > If we leave always b-a+1 we potentially can get the next buffer strings:
> > p1,p2...pN,
> > or
> > p1,p2,pN(\0)
> >  while i is the index of the character after a comma or '\0'
> >
> > so, for prevent the last comma we can override always in the end of
> function(not between 2 loops) '\0' like this:
> > if (i >0 )
> > /* For prevent last comma. */
> > 	buffer[i-1] = '\0';
> > instead of using 2 conditions per non sub device param - 'if(a)' and 'a ? "," :
> ""'
> > I think each future programmer can easily understand it.
> >
> >
> 
> Sure, I agree that it would work. It would also probably be parseable.
> But it depends on your implementation. I think you'll have to write this
> `buffer[i-1] = '\0';` outside the branch dealing with proper kvargs, which
> mean scattering within the rest of the function code that should be dealing
> with how those kvargs are presented to the next stage.
> 
> I think the most constructive thing to do would be for you to write a v2
> proposing your solution. If you are able to contain all logic pertaining to the
> kvargs within this single branch along with the other remarks (keeping
> snprintf, using a local "len" variable), then perfect :). Otherwise, while
> imperfect I proposed an alternative that should be able to do that.
> 

OK, I will create V2, thanks for this review!

> > > > > The changes should be limited to the actual bug. No need to change
> this.
> > > > >
> > > > > > +			if (b == temp)
> > > > > >  				return -EINVAL;
> > > > > >  			b += 1;
> > > > > >  			if (params[b] == '\0')
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > > >
> > > > > Thanks again for the debug and sorry for being nitpicky.
> > > > >
> > > > > --
> > > > > Gaëtan Rivet
> > > > > 6WIND
> > > >
> > > > Regards,
> > > > Matan Azrad.
> > >
> > > --
> > > Gaëtan Rivet
> > > 6WIND
> >
> >
> > Regards
> > Matan Azrad.
> 
> --
> Gaëtan Rivet
> 6WIND

Regards
Matan Azrad

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

[Matan Azrad]

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@dpdk.org

Signed-off-by: Matan Azrad <matan@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]);
+			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';
 	snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
 	return 0;
 }
-- 
2.7.4

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

[Gaëtan Rivet]

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@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@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]);

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 :).

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.

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

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

-- 
Gaëtan Rivet
6WIND

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

[Matan Azrad]

Hi Gaetan

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Monday, August 21, 2017 12:34 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@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@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@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

Re: [dpdk-stable] [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@6wind.com]
> > Sent: Monday, August 21, 2017 12:34 PM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: dev@dpdk.org; stable@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@dpdk.org
> > >
> > > Signed-off-by: Matan Azrad <matan@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

[dpdk-stable] [PATCH v3] net/failsafe: fix parameters parsing

[Matan Azrad]

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 comma separation.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/failsafe/failsafe_args.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
index 1f22416..ae857b0 100644
--- a/drivers/net/failsafe/failsafe_args.c
+++ b/drivers/net/failsafe/failsafe_args.c
@@ -286,10 +286,17 @@ 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;
+
+			if (i > 0)
+				len += 1;
+			snprintf(&buffer[i], len + 1, "%s%s",
+					i ? "," : "", &params[a]);
+			i += len;
+		} else if (params[b] == '(') {
 			size_t start = b;
+
 			b += closing_paren(&params[b]);
 			if (b == start)
 				return -EINVAL;
@@ -393,6 +400,7 @@ failsafe_args_parse(struct rte_eth_dev *dev, const char *params)
 					&dev->data->mac_addrs[0]);
 			if (ret < 0)
 				goto free_kvlist;
+
 			mac_from_arg = 1;
 		}
 	}
-- 
2.7.4

Re: [dpdk-stable] [PATCH v3] net/failsafe: fix parameters parsing

[Gaëtan Rivet]

Hi Matan,

thanks

On Sun, Aug 27, 2017 at 10:23:14AM +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 comma separation.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_args.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
> index 1f22416..ae857b0 100644
> --- a/drivers/net/failsafe/failsafe_args.c
> +++ b/drivers/net/failsafe/failsafe_args.c
> @@ -286,10 +286,17 @@ 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;
> +
> +			if (i > 0)
> +				len += 1;
> +			snprintf(&buffer[i], len + 1, "%s%s",
> +					i ? "," : "", &params[a]);
> +			i += len;
> +		} else if (params[b] == '(') {
>  			size_t start = b;
> +
>  			b += closing_paren(&params[b]);
>  			if (b == start)
>  				return -EINVAL;
> @@ -393,6 +400,7 @@ failsafe_args_parse(struct rte_eth_dev *dev, const char *params)
>  					&dev->data->mac_addrs[0]);
>  			if (ret < 0)
>  				goto free_kvlist;
> +
>  			mac_from_arg = 1;
>  		}
>  	}
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [PATCH v3] net/failsafe: fix parameters parsing

[Ferruh Yigit]

On 8/28/2017 8:52 AM, Gaëtan Rivet wrote:
> Hi Matan,
> 
> thanks
> 
> On Sun, Aug 27, 2017 at 10:23:14AM +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 comma separation.
>>
>> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Applied to dpdk-next-net/master, thanks.