From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH v6 04/19] eal: fix wrong strnlen() return value in 32bit icc Date: Mon, 16 Feb 2015 15:47:32 +0100 Message-ID: <54E20304.2070000@6wind.com> References: <1423728996-3004-1-git-send-email-cunming.liang@intel.com> <1423791501-1555-1-git-send-email-cunming.liang@intel.com> <1423791501-1555-5-git-send-email-cunming.liang@intel.com> <20150213134933.GA13495@neilslaptop.think-freely.org> <54DE04B8.6080708@6wind.com> <20150213175523.GA17402@neilslaptop.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "dev-VfR2kkLFssw@public.gmane.org" To: "Liang, Cunming" , Neil Horman Return-path: In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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