[dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in 32bit icc

[Neil Horman]

On Fri, Feb 13, 2015 at 06:11:02PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Neil Horman
> > Sent: Friday, February 13, 2015 5:55 PM
> > To: Olivier MATZ
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in 32bit icc
> > 
> > On Fri, Feb 13, 2015 at 03:05:44PM +0100, Olivier MATZ wrote:
> > > Hi Neil,
> > >
> > > On 02/13/2015 02:49 PM, Neil Horman wrote:
> > > > On Fri, Feb 13, 2015 at 09:38:06AM +0800, Cunming Liang wrote:
> > > >> The problem is that strnlen() here may return invalid value with 32bit icc.
> > > >> (actually it returns it’s second parameter,e.g: sysconf(_SC_ARG_MAX)).
> > > >> It starts to manifest hwen max_len parameter is > 2M and using icc –m32 –O2 (or above).
> > > >>
> > > >> Suggested-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
> > > >> Signed-off-by: Cunming Liang <cunming.liang at intel.com>
> > > >> ---
> > > >>  v5 changes:
> > > >>    using strlen instead of strnlen.
> > > >>
> > > >>  lib/librte_eal/common/eal_common_options.c | 6 +++---
> > > >>  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> > > >> index 178e303..9cf2faa 100644
> > > >> --- a/lib/librte_eal/common/eal_common_options.c
> > > >> +++ b/lib/librte_eal/common/eal_common_options.c
> > > >> @@ -167,7 +167,7 @@ eal_parse_coremask(const char *coremask)
> > > >>  	if (coremask[0] == '0' && ((coremask[1] == 'x')
> > > >>  		|| (coremask[1] == 'X')))
> > > >>  		coremask += 2;
> > > >> -	i = strnlen(coremask, PATH_MAX);
> > > >> +	i = strlen(coremask);
> > > > This is crash prone.  If coremask is passed in without a trailing null pointer,
> > > > strlen will return a huge value that can overrun the array.
> > >
> > > We discussed that in a previous thread:
> > > http://dpdk.org/ml/archives/dev/2015-February/012552.html
> > >
> > > coremask is always a valid nul-terminated string as it comes from
> > > argv[] table.
> > > It is not a memory fragment that is controlled by a user, so I don't
> > > think using strnlen() instead of strlen() would solve any issue.
> > >
> > Thats absolutely false,  you can't in any way make that assertion.
> > eal_parse_common_option is a public API call. 
> 
> Why do you consider is as public API?
> It is declared in private librte_eal header file:
> lib/librte_eal/common/eal_options.h
> So yes, in theory it can be used by some other functions inside librte_eal.
> But it shouldn't be visible outside librte_eal.
> Konstantin
> 
I apologize, I misread the Makefile.  I thought I saw that it was symlinked as a
publically accessible header file, but thats not the case.  As such, it should
be ok.

Acked-by: Neil Horman <nhorman at tuxdriver.com>

> > An application can construct its
> > own string to pass into the parser.  The test applications all use the command
> > line functions so its not a visible issue from the test apps, but you can't
> > assume what the test apps do is what everyone will do.  It would be one thing if
> > you could make the parse_common_option function private, but with the current
> > layout you can't so you have to be ready for garbage input.
> > 
> > Neil
> > 
> > > Regards,
> > > Olivier
> > >