From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f169.google.com (mail-wr0-f169.google.com [209.85.128.169]) by dpdk.org (Postfix) with ESMTP id 56DBE7D9B for ; Mon, 21 Aug 2017 11:34:37 +0200 (CEST) Received: by mail-wr0-f169.google.com with SMTP id 30so21203153wrk.0 for ; Mon, 21 Aug 2017 02:34:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=TEDLCD8VDOkvVo56k90WrOY8cvbu1yEZ5bPpYHV2bUw=; b=HBP1OBUp5sXKjoMByK3UPqL0Z1JrWr5NF4FQTyaf6B3bIyqG41TTGzlL7ejyXswsYA hkRK8bW+CD9DwX3Oyey/GrKS7gGE9rDzP8K1GrOlNyLdf+c3XSrjQCNdei7cg4WELC0l h35G74fPl58RMhrPX6jkgxjU9gJTyY5Y/6Wz/xo2tlTuXTA+qre6zkuswnQJ0VXfkZJK GwTsqwD3QUExXY37k0oI6XoOTUnZhBFnYsi5yMtgIuro7zsvBzY61s8nDdKLL5HQOiQL dzGNVDm9Gn4d1x/j+aC3X/Zg1XjuXyIxqrS22WrrQRUudwbDlHgCuKCkNBuxsn/ZYgA/ Nqtg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=TEDLCD8VDOkvVo56k90WrOY8cvbu1yEZ5bPpYHV2bUw=; b=aYR4JYa012lD1/nAvwUPETJFjuwnlCEHV3iH8AraM4sTCwottpbTJV3FqQIHmivM2A 9PDbMzHU8OFUCHC8IAmEQ/o1OBWYRxSbJDoxIkLwce9NROIgJkL7taLoC/Yonl37JQA2 3fSm1UiPci8B3PE1VecespX9RbAFBNECJLrzfzsjGHmmgI6JSVy/BfFcfGCwQPSMMQeb 1uUlt4nFCvQha6YFIKnCPdmJx/xDvgDLsdsNUaKCnSz207D6feB8EkHfibk5G95K/Npn +sTJHAoDL5QPTDy19nPYsBvT3HfeSl++gXLBiue0+OI7nXicPcxNdsS3Y+/TQI8YoEi5 1F7Q== X-Gm-Message-State: AHYfb5i0lSnn8/K8X7aDb4fvhFXeSijHaCKc+DQdXfk62ytY1RHzrGbn 1la+KNN7wJFqWN85z9k= X-Received: by 10.223.172.137 with SMTP id o9mr10861421wrc.275.1503308076780; Mon, 21 Aug 2017 02:34:36 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id t13sm1504548wrg.27.2017.08.21.02.34.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 21 Aug 2017 02:34:35 -0700 (PDT) Date: Mon, 21 Aug 2017 11:34:26 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: dev@dpdk.org, stable@dpdk.org Message-ID: <20170821093426.GV8124@bidouze.vm.6wind.com> References: <1503180443-9687-1-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1503180443-9687-1-git-send-email-matan@mellanox.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2] net/failsafe: fix parameters parsing X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Aug 2017 09:34:37 -0000 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 > --- > 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. > + 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