Hi Anoob
> -----Original Message----- > From: Anoob Joseph [mailto:ano...@marvell.com] > Sent: Saturday, February 23, 2019 6:12 AM > To: Trahe, Fiona <fiona.tr...@intel.com>; Akhil Goyal <akhil.go...@nxp.com>; > Doherty, Declan > <declan.dohe...@intel.com>; De Lara Guarch, Pablo > <pablo.de.lara.gua...@intel.com>; Yigit, Ferruh > <ferruh.yi...@intel.com>; Thomas Monjalon <tho...@monjalon.net> > Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Narayana Prasad Raju > Athreya > <pathr...@marvell.com>; dev@dpdk.org; Ankur Dwivedi <adwiv...@marvell.com> > Subject: RE: [PATCH] lib/cryptodev: fix driver name comparison > > 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? [Fiona] Yes, I think it should be in this patch as this patch is causing it. But it's up to the maintainers what's acceptable - it seems to me that it's an ABI breakage, avoiding saying it explicitly doesn't make it less so. > > 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
