Hi Fiona, > -----Original Message----- > From: Trahe, Fiona <fiona.tr...@intel.com> > Sent: Friday, February 22, 2019 9:09 PM > To: Anoob Joseph <ano...@marvell.com>; Akhil Goyal <akhil.go...@nxp.com>; > Doherty, Declan <declan.dohe...@intel.com>; De Lara Guarch, Pablo > <pablo.de.lara.gua...@intel.com> > Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Narayana Prasad Raju > Athreya <pathr...@marvell.com>; dev@dpdk.org; Ankur Dwivedi > <adwiv...@marvell.com>; Trahe, Fiona <fiona.tr...@intel.com> > Subject: RE: [PATCH] lib/cryptodev: fix driver name comparison > > Hi Anoob, > > > > > > @@ -542,8 +543,8 @@ rte_cryptodev_get_dev_id(const char *name) > > > > > return -1; > > > > > > > > > > for (i = 0; i < cryptodev_globals.nb_devs; i++) > > > > > - if ((strcmp(cryptodev_globals.devs[i].data->name, name) > > > > > - == 0) && > > > > > + if ((strncmp(cryptodev_globals.devs[i].data->name, > name, > > > > > + RTE_CRYPTODEV_NAME_MAX_LEN) > == 0) > > > && > > > [Fiona] Is this safe? The const passed to this may not be the full > > > length of RTE_CRYPTODEV_NAME_MAX_LEN. Does this prototype need to > > > specify that a full length const filled with trailing zeros must be > > > passed in? And if so is this an ABI breakage? > > > > > > > [Anoob] strcmp itself is not safe when we have buffers which are not > > NULL terminated. Strncmp will make sure the check won't exceed > RTE_CRYPTODEV_NAME_MAX_LEN. > > > > From man page, "The strncmp() function is similar, except it only > > compares the first (at most) n bytes of > > s1 and s2." > > > > The main issue here is the usage of strncmp with strlen(driver_name), > > as in the below cases. Strlen will return string length, which doesn't > > include \0. strcmp is good enough to fix the issue. But usage of > > strcmp would assume that the const is filled with trailing zero. IMO, none > > of > these options are really safe. So please advise on what would be the best > solution here. I'll revise the patch accordingly. > [Fiona] I agree and think it is safest as you've coded it. However I'd suggest > adding a comment on the relevant APIs saying that the string must be passed in > in a buffer of size <use relevant #define> with trailing zeros.
[Anoob] Do you want this patch to address that? And wouldn't specifying something like that explicitly, be an ABI breakage? Also, I think the same is applicable for other similar functions (rte_eth_dev_get_port_by_name() etc), wherever we expect a string. Please do share your thoughts on what all I should include in this patch. Thanks, Anoob
