From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0048.outbound.protection.outlook.com [104.47.2.48]) by dpdk.org (Postfix) with ESMTP id 930EB917F; Thu, 17 Aug 2017 17:54:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=OiCPoRzUPcRYgnluJzxssPON1bJyc89fshXruDfiSU8=; b=bm3VJUZ/rrW/noeek3mdXVNTeE/03IW7D8P01Lrw7HbfgbLZCxZQtYw4n3eLYG1/JPVEieAGePAN6ujIMpx42M3N5sX4Wjm4aq9lSna1CaVJsgvys9y0HziQpaJFEhDuNbPQyhUVnscH4FnOIXtbLn+zw3oPXRyE4yycE49AdFo= Received: from DB6PR0502MB3048.eurprd05.prod.outlook.com (10.172.250.136) by DB6PR05MB3208.eurprd05.prod.outlook.com (10.170.221.138) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.1362.18; Thu, 17 Aug 2017 15:54:23 +0000 Received: from DB6PR0502MB3048.eurprd05.prod.outlook.com ([fe80::e938:efe9:effc:7ef8]) by DB6PR0502MB3048.eurprd05.prod.outlook.com ([fe80::e938:efe9:effc:7ef8%14]) with mapi id 15.01.1341.024; Thu, 17 Aug 2017 15:54:23 +0000 From: Matan Azrad To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "dev@dpdk.org" , Ori Kam , "stable@dpdk.org" Thread-Topic: [PATCH] net/failsafe: fix parameters parsing Thread-Index: AQHTF20gviU1FquxMkGyQ9F2MipUdaKIrHzQ Date: Thu, 17 Aug 2017 15:54:23 +0000 Message-ID: References: <1502979581-27282-1-git-send-email-matan@mellanox.com> <20170817152546.GP8124@bidouze.vm.6wind.com> In-Reply-To: <20170817152546.GP8124@bidouze.vm.6wind.com> Accept-Language: en-US, he-IL Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=matan@mellanox.com; x-originating-ip: [193.47.165.251] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB6PR05MB3208; 6:z883iuTgBjFbceMpn9ZDwBXu7sGMgZkDQPoX2hc2Pofm2Z7JjoMRXfheroPZTQT7c25Uftpmf9XcFf1R2RAwDnF94CzPee21liJu7HCrRlX7r1areRcM5bB7XklfulthAe10cHSJPNFqpz2yiWX3QDSPwMZOu57SOJ5gaKYChXompNwrJZ13N6tv+lsxF5thTeb5eQaKyMUvmTJlzMXcIYMNzeWMvhaHNPJysZpEUPvRKdTC3QP3aiExPm6YXvJObVIu1cfwMfbKPf4N58nffxcihXflVevV4LymqGXQzNXhsbDi4H8TyXCwUcyk0UJAGT9Bvr51cBWXIg+WdtU+xA==; 5:W003lVexbhXFUILs/hjeK7og0rAn4ek3UPwzGLVIFMsTwpY7cLXYd7He6eMnzw0bhX73j93LGeuddpwAnckajhkAemQC/R6PlWmdG/GO0mJy95yjBsneLqeWJ5om0mvjyodVX6tHjDeaoUBVt2aWEw==; 24:WR4J3duBHbuZa07hGWjwRhEfSwrNt1a7TtUamB89EwB2vrz8j0/bNVt8iW9sZuhaeECs+5RqAYQ6XViUSsmziVABraYh+aLvZl/TAS1wfQs=; 7:5CUKcOIkeR/4cuET7kj9zRzz8Hf30c6EdskLVD1z73GP53XExCLlnrwMgw5h7qEq4PZdfUPFWf1EubhFDJOIqLIyAUi1pNicf1cM2nIdG+RmI2/d2cDP73ZH9Gps0swW3A4P45P8fFF2cG8VljFRGYAOehL4RaexYEA2i44PhS8GLnHcg8RgKyudsKo32yeFRuATY2fDRUnW3LsfH9QtA6ZGk5HbDDE3TG3Gnp0KIt0= x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 4c5b5bc3-b05d-4d6f-0c2e-08d4e5883b29 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(2017030254152)(48565401081)(300000503095)(300135400095)(2017052603031)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:DB6PR05MB3208; x-ms-traffictypediagnostic: DB6PR05MB3208: x-exchange-antispam-report-test: UriScan:; x-microsoft-antispam-prvs: x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(601004)(2401047)(8121501046)(5005006)(3002001)(100000703101)(100105400095)(10201501046)(93006095)(93001095)(6055026)(6041248)(20161123560025)(20161123555025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123564025)(20161123562025)(20161123558100)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:DB6PR05MB3208; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:DB6PR05MB3208; x-forefront-prvs: 0402872DA1 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(39860400002)(43784003)(13464003)(189002)(199003)(377454003)(24454002)(4326008)(3660700001)(68736007)(6506006)(3280700002)(50986999)(106356001)(54356999)(76176999)(101416001)(66066001)(105586002)(6246003)(478600001)(110136004)(33656002)(74316002)(229853002)(8936002)(6436002)(6916009)(53546010)(81156014)(81166006)(9686003)(14454004)(97736004)(5250100002)(99286003)(54906002)(8676002)(2900100001)(55016002)(6116002)(3846002)(102836003)(7736002)(53936002)(305945005)(7696004)(25786009)(5660300001)(2950100002)(189998001)(86362001)(2906002); DIR:OUT; SFP:1101; SCL:1; SRVR:DB6PR05MB3208; H:DB6PR0502MB3048.eurprd05.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Aug 2017 15:54:23.5691 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR05MB3208 Subject: Re: [dpdk-dev] [PATCH] 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: Thu, 17 Aug 2017 15:54:26 -0000 > -----Original Message----- > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > Sent: Thursday, August 17, 2017 6:26 PM > To: Matan Azrad > Cc: dev@dpdk.org; Ori Kam ; stable@dpdk.org > Subject: Re: [PATCH] net/failsafe: fix parameters parsing >=20 > Hi Matan, >=20 > Thanks for spotting the bug. >=20 > 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. >=20 > Actually snprintf should still be used. >=20 Why? We just want to copy from buffer to buffer, no needs snprintf overheads If actually we are not using complicated format. > > > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD") > > Cc: stable@dpdk.org > > > > Signed-off-by: Matan Azrad > > --- > > 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] =3D {0}; > > - size_t a, b; > > + size_t a, b, temp; >=20 > temp is not descriptive enough. What are about a, b, i ? =20 > You are declaring this variable here because you want to re-use it instea= d of > `start`. This is an overreach however, this fix must be restricted to the= actual > bug. temp is helping to address the original bug, don't we want to reuse variabl= e=20 Instead of 2 if statement variables, maybe other name for all? Something like: a=3D> start b=3D> end i =3D> next temp=3D> saved_val =20 >=20 > > int i; > > > > a =3D 0; > > @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char > params[DEVARGS_MAXLEN]) > > ERROR("Invalid parameter"); > > return -EINVAL; > > } > > - if (params[b] =3D=3D ',' || params[b] =3D=3D '\0') >=20 > Declare the temporary variable in this scope, with a descriptive name suc= h as > "len", "length", "param_len" or something close. >=20 > > - i +=3D snprintf(&buffer[i], b - a + 1, "%s", ¶ms[a]); > > - if (params[b] =3D=3D '(') { > > - size_t start =3D b; > > + if (params[b] =3D=3D ',' || params[b] =3D=3D '\0') { > > + temp =3D b - a + 1; >=20 > The value here should be "b - a". > If a !=3D 0 however, then we are parsing a new parameter and buffer alrea= dy > contains at least one. A comma should be added to separate the two. >=20 > An example might clarify what I mean: >=20 > if (params[b] =3D=3D ',' || params[b] =3D=3D '\0') { > size_t param_len =3D b - a; >=20 > if (a) > param_len +=3D 1; > snprintf(&buffer[i], param_len + 1, "%s%s", > a ? "," : "", ¶ms[a]); > i +=3D param_len; > } >=20 > The conditionals about `a` are ugly however, if you find a better way to = write > those you are most welcome :). >=20 > > + memcpy(&buffer[i], ¶ms[a], temp); > > + i +=3D temp; > > + } else if (params[b] =3D=3D '(') { > > + temp =3D b; > > b +=3D closing_paren(¶ms[b]); > > - if (b =3D=3D start) >=20 I think the last comma is harmless for next parse But I can just change the last coma to '\0' in the end of function(if exist= s). But, This solves another issue, don't it? Maybe in different patch? > The changes should be limited to the actual bug. No need to change this. >=20 > > + if (b =3D=3D temp) > > return -EINVAL; > > b +=3D 1; > > if (params[b] =3D=3D '\0') > > -- > > 2.7.4 > > >=20 > Thanks again for the debug and sorry for being nitpicky. >=20 > -- > Ga=EBtan Rivet > 6WIND Regards, Matan Azrad.