| Message ID | 1423791501-1555-5-git-send-email-cunming.liang@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 922FAAD84; Fri, 13 Feb 2015 02:38:50 +0100 (CET) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 7C72BAD7C for <dev@dpdk.org>; Fri, 13 Feb 2015 02:38:45 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 12 Feb 2015 17:31:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,568,1418112000"; d="scan'208";a="526919218" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by orsmga003.jf.intel.com with ESMTP; 12 Feb 2015 17:30:28 -0800 Received: from shecgisg004.sh.intel.com (shecgisg004.sh.intel.com [10.239.29.89]) by shvmail01.sh.intel.com with ESMTP id t1D1cd7L005641; Fri, 13 Feb 2015 09:38:39 +0800 Received: from shecgisg004.sh.intel.com (localhost [127.0.0.1]) by shecgisg004.sh.intel.com (8.13.6/8.13.6/SuSE Linux 0.8) with ESMTP id t1D1cZFw001643; Fri, 13 Feb 2015 09:38:37 +0800 Received: (from cliang18@localhost) by shecgisg004.sh.intel.com (8.13.6/8.13.6/Submit) id t1D1cZ8n001639; Fri, 13 Feb 2015 09:38:35 +0800 From: Cunming Liang <cunming.liang@intel.com> To: dev@dpdk.org Date: Fri, 13 Feb 2015 09:38:06 +0800 Message-Id: <1423791501-1555-5-git-send-email-cunming.liang@intel.com> X-Mailer: git-send-email 1.7.4.1 In-Reply-To: <1423791501-1555-1-git-send-email-cunming.liang@intel.com> References: <1423728996-3004-1-git-send-email-cunming.liang@intel.com> <1423791501-1555-1-git-send-email-cunming.liang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Cunming Liang
Feb. 13, 2015, 1:38 a.m. UTC
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@intel.com>
Signed-off-by: Cunming Liang <cunming.liang@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(-)
Comments
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@intel.com> > Signed-off-by: Cunming Liang <cunming.liang@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. This seems like a bug in icc, and should be fixed there. Neil
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@intel.com> >> Signed-off-by: Cunming Liang <cunming.liang@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. Regards, Olivier
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@intel.com> > >> Signed-off-by: Cunming Liang <cunming.liang@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 >
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman > Sent: Friday, February 13, 2015 5:55 PM > To: Olivier MATZ > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in 32bit icc > > 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@intel.com> > > >> Signed-off-by: Cunming Liang <cunming.liang@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. Why do you consider is as public API? It is declared in private librte_eal header file: lib/librte_eal/common/eal_options.h So yes, in theory it can be used by some other functions inside librte_eal. But it shouldn't be visible outside librte_eal. Konstantin > 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 > >
On Fri, Feb 13, 2015 at 06:11:02PM +0000, Ananyev, Konstantin wrote: > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman > > Sent: Friday, February 13, 2015 5:55 PM > > To: Olivier MATZ > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in 32bit icc > > > > 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@intel.com> > > > >> Signed-off-by: Cunming Liang <cunming.liang@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. > > Why do you consider is as public API? > It is declared in private librte_eal header file: > lib/librte_eal/common/eal_options.h > So yes, in theory it can be used by some other functions inside librte_eal. > But it shouldn't be visible outside librte_eal. > Konstantin > I apologize, I misread the Makefile. I thought I saw that it was symlinked as a publically accessible header file, but thats not the case. As such, it should be ok. Acked-by: Neil Horman <nhorman@tuxdriver.com> > > 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 > > >
> -----Original Message----- > From: Neil Horman [mailto:nhorman@tuxdriver.com] > Sent: Saturday, February 14, 2015 1:55 AM > To: Olivier MATZ > Cc: Liang, Cunming; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in > 32bit icc > > 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@intel.com> > > >> Signed-off-by: Cunming Liang <cunming.liang@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 [LCM] It sounds reasonable to me. I'll rollback the code and use strnlen(coremask, ARG_MAX) instead. > > > Regards, > > Olivier > >
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
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); while ((i > 0) && isblank(coremask[i - 1])) i--; if (i == 0) @@ -227,7 +227,7 @@ eal_parse_corelist(const char *corelist) /* Remove all blank characters ahead and after */ while (isblank(*corelist)) corelist++; - i = strnlen(corelist, sysconf(_SC_ARG_MAX)); + i = strlen(corelist); while ((i > 0) && isblank(corelist[i - 1])) i--; @@ -472,7 +472,7 @@ eal_parse_lcores(const char *lcores) /* Remove all blank characters ahead and after */ while (isblank(*lcores)) lcores++; - i = strnlen(lcores, sysconf(_SC_ARG_MAX)); + i = strlen(lcores); while ((i > 0) && isblank(lcores[i - 1])) i--;