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

[Neil Horman]

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.  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
>