Hi Akhil, Declan, Pablo, Can you review this patch and share your thoughts?
Thanks, Anoob > -----Original Message----- > From: Trahe, Fiona <fiona.tr...@intel.com> > Sent: Monday, February 25, 2019 5:22 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>; 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 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