Re: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name comparison

["Trahe, Fiona" <fiona.trahe@intel.com>]

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.