From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 89738F72 for ; Thu, 2 Feb 2017 14:55:03 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP; 02 Feb 2017 05:55:02 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,324,1477983600"; d="scan'208";a="1121184984" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by fmsmga002.fm.intel.com with ESMTP; 02 Feb 2017 05:55:01 -0800 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.77]) by irsmsx110.ger.corp.intel.com ([169.254.15.101]) with mapi id 14.03.0248.002; Thu, 2 Feb 2017 13:55:00 +0000 From: "Mrozowicz, SlawomirX" To: Stephen Hemminger , "Doherty, Declan" CC: "dev@dpdk.org" Thread-Topic: bugs and glitches in rte_cryptodev_devices_get Thread-Index: AQHSfKvNzGDS4PCdLEmABsE9qVpYEaFVurHQ Date: Thu, 2 Feb 2017 13:55:00 +0000 Message-ID: <158888A50F43E34AAE179517F56C9745652770@IRSMSX103.ger.corp.intel.com> References: <20170201085321.7168505a@xeon-e3> In-Reply-To: <20170201085321.7168505a@xeon-e3> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] bugs and glitches in rte_cryptodev_devices_get 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, 02 Feb 2017 13:55:04 -0000 >-----Original Message----- >From: Stephen Hemminger [mailto:stephen@networkplumber.org] >Sent: Wednesday, February 1, 2017 5:54 PM >To: Mrozowicz, SlawomirX ; Doherty, >Declan >Cc: dev@dpdk.org >Subject: bugs and glitches in rte_cryptodev_devices_get > >The function rte_cryptodev_devices_get has several issues. I was just goin= g >to fix it, but think it need to be explained. > >One potentially serious one (reported by coverity) is: > >*** CID 141067: (BAD_COMPARE) >/lib/librte_cryptodev/rte_cryptodev.c: 503 in rte_cryptodev_devices_get() >497 && (*devs + i)->attached =3D=3D >498 RTE_CRYPTODEV_ATTACHED) >{ >499 >500 dev =3D (*devs + i)->device; >501 >502 if (dev) >>>> CID 141067: (BAD_COMPARE) >>>> Truncating the result of "strncmp" to "unsigned char" may cause it= to be >misinterpreted as 0. Note that "strncmp" may return an integer besides -1,= 0, >or 1. >503 cmp =3D strncmp(dev->driver->name, >504 dev_name, >505 strlen(dev_name)); >506 else >507 cmp =3D strncmp((*devs + i)->data->name, >508 dev_name, >/lib/librte_cryptodev/rte_cryptodev.c: 507 in rte_cryptodev_devices_get() >501 >502 if (dev) >503 cmp =3D strncmp(dev->driver->name, >504 dev_name, >505 strlen(dev_name)); >506 else >>>> CID 141067: (BAD_COMPARE) >>>> Truncating the result of "strncmp" to "unsigned char" may cause it= to be >misinterpreted as 0. Note that "strncmp" may return an integer besides -1,= 0, >or 1. >507 cmp =3D strncmp((*devs + i)->data->name, >508 dev_name, >509 strlen(dev_name)); >510 >511 if (cmp =3D=3D 0) >512 devices[count++] =3D (*devs + i)->data->dev_id; > > >But also: > >1. Incorrect function signature: > * function returns int but never a negative value. should be unsigned. > * devices argument is not modified should be const. [SM] Ok. To be changed. > >2. Original ABI seems short sighted with limit of 256 cryptodevs > * this seems like 8 bit mindset, should really use unsigned int inste= ad > of uint8_t for number of devices. [SM] Ok. To be changed to uint8_t. > >3. Wacky indention of the if statement. [SM] To be changed. > >4. Make variables local to the block they are used (cmp, dev) [SM] Ok. To be changed. > >5. Use array instead of pointer: > ie. instead of *devs + i use devs[i] [SM] We can't change it like this. devs[i] provide wrong address (null) for= i>0 > > >The overall code in question is: > > >int >rte_cryptodev_devices_get(const char *dev_name, uint8_t *devices, > uint8_t nb_devices) >{ > uint8_t i, cmp, count =3D 0; > struct rte_cryptodev **devs =3D &rte_cryptodev_globals->devs; > struct rte_device *dev; > > for (i =3D 0; i < rte_cryptodev_globals->max_devs && count < >nb_devices; > i++) { > > if ((*devs + i) > && (*devs + i)->attached =3D=3D > RTE_CRYPTODEV_ATTACHED) >{ > > dev =3D (*devs + i)->device; > > if (dev) > cmp =3D strncmp(dev->driver->name, > dev_name, > strlen(dev_name)); > else > cmp =3D strncmp((*devs + i)->data->name, > dev_name, > strlen(dev_name)); > > if (cmp =3D=3D 0) > devices[count++] =3D (*devs + i)->data->dev_id; > } > } > > return count; >} > >Please fix it. >