Re: [PATCH v2] net/failsafe: fix parameters parsing
From: Gaëtan Rivet <hidden>
Date: 2017-08-21 09:34:37
Hi Matan, On Sun, Aug 20, 2017 at 01:07:23AM +0300, Matan Azrad wrote:
quoted hunk ↗ jump to hunk
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]) 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]);
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.
quoted hunk ↗ jump to hunk
+ 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.
snprintf(params, DEVARGS_MAXLEN, "%s", buffer); return 0; } -- 2.7.4
-- Gaëtan Rivet 6WIND