From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id E6CF6B609 for ; Mon, 16 Feb 2015 15:47:44 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1YNN1F-00079z-EY; Mon, 16 Feb 2015 15:51:34 +0100 Message-ID: <54E20304.2070000@6wind.com> Date: Mon, 16 Feb 2015 15:47:32 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.3.0 MIME-Version: 1.0 To: "Liang, Cunming" , Neil Horman 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> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in 32bit icc X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Feb 2015 14:47:45 -0000 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