Re: [PATCH] net/failsafe: fix parameters parsing
From: Gaëtan Rivet <hidden>
Date: 2017-08-17 15:25:57
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.
quoted hunk ↗ jump to hunk
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. 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.
quoted hunk ↗ jump to hunk
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", ¶ms[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 ? "," : "", ¶ms[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], ¶ms[a], temp);
+ i += temp;
+ } else if (params[b] == '(') {
+ temp = b;
b += closing_paren(¶ms[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