Re: [PATCH] net/failsafe: fix parameters parsing

-----Original Message-----
From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
Sent: Thursday, August 17, 2017 6:26 PM
To: Matan Azrad <redacted>
Cc: dev@dpdk.org; Ori Kam <redacted>; 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:

quoted

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.

quoted

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

Signed-off-by: Matan Azrad <redacted>
---
 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
 

quoted

 	int i;

 	a = 0;

@@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char

params[DEVARGS_MAXLEN])

quoted

 			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.

quoted

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

quoted

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

quoted

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