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

On Mon, Aug 21, 2017 at 12:02:44PM +0000, Matan Azrad wrote:

Hi Gaetan

quoted

-----Original Message-----
From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
Sent: Monday, August 21, 2017 12:34 PM
To: Matan Azrad <redacted>
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:

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 validates the last comma removing.

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

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

quoted

 			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.

quoted

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

quoted

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.

quoted

quoted

+			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])

quoted

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

quoted

quoted

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

-- 
Gaëtan Rivet
6WIND