patches for DPDK stable branches
 help / color / mirror / Atom feed

From: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: dev@dpdk.org, stable@dpdk.org
Subject: Re: [dpdk-stable] [PATCH v2] net/failsafe: fix parameters parsing
Date: Mon, 21 Aug 2017 11:34:26 +0200	[thread overview]
Message-ID: <20170821093426.GV8124@bidouze.vm.6wind.com> (raw)
In-Reply-To: <1503180443-9687-1-git-send-email-matan@mellanox.com>

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

next prev parent reply	other threads:[~2017-08-21  9:34 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-17 14:19 [dpdk-stable] [PATCH] " Matan Azrad
2017-08-17 15:25 ` Gaëtan Rivet
2017-08-17 15:54   ` Matan Azrad
2017-08-17 16:24     ` Gaëtan Rivet
2017-08-17 18:52       ` Matan Azrad
2017-08-18  9:03         ` Gaëtan Rivet
2017-08-19 21:24           ` Matan Azrad
2017-08-19 22:07             ` [dpdk-stable] [PATCH v2] " Matan Azrad
2017-08-21  9:34               ` Gaëtan Rivet [this message]
2017-08-21 12:02                 ` Matan Azrad
2017-08-21 13:25                   ` Gaëtan Rivet
2017-08-27  7:23                     ` [dpdk-stable] [PATCH v3] " Matan Azrad
2017-08-28  7:52                       ` Gaëtan Rivet
2017-08-29 17:05                         ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170821093426.GV8124@bidouze.vm.6wind.com \
    --to=gaetan.rivet@6wind.com \
    --cc=dev@dpdk.org \
    --cc=matan@mellanox.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).