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
