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

[Olivier MATZ <olivier.matz@6wind.com>]

Hi,

On 02/15/2015 02:32 AM, Liang, Cunming wrote:
>>>>> --- 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
> [LCM] It sounds reasonable to me. I'll rollback the code and use strnlen(coremask, ARG_MAX) instead.

I still don't agree that we should use strnlen(coremask, ARG_MAX).

The API of eal_parse_coremask() requires that a valid string is passed
as an argument, so strlen() is perfectly fine. It's up to the caller to
ensure that the string is valid.

Using strnlen(coremask, ARG_MAX) in eal_parse_coremask() with an
arbitrary length does not protect from having a segfault in case the
string is invalid and the caller's buffer length is < ARG_MAX.

This would still be true even if eal_parse_coremask() is public.

Regards,
Olivier