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

Message ID 1423970145-31985-5-git-send-email-cunming.liang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Cunming Liang Feb. 15, 2015, 3:15 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>
---
 v7 changes:
   rollback to use strnlen

 v5 changes:
   using strlen instead of strnlen.

 lib/librte_eal/common/eal_common_options.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
  

Comments

Olivier Matz Feb. 16, 2015, 2:51 p.m. UTC | #1
Hi,

On 02/15/2015 04:15 AM, 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>

Sorry but I don't think using strnlen() is appropriate here. See
http://dpdk.org/ml/archives/dev/2015-February/013309.html

Regards,
Olivier
  
Cunming Liang Feb. 17, 2015, 1:29 a.m. UTC | #2
> -----Original Message-----

> From: Olivier MATZ [mailto:olivier.matz@6wind.com]

> Sent: Monday, February 16, 2015 10:52 PM

> To: Liang, Cunming; dev@dpdk.org

> Cc: Ananyev, Konstantin; nhorman@tuxdriver.com

> Subject: Re: [PATCH v7 04/19] eal: fix wrong strnlen() return value in 32bit icc

> 

> Hi,

> 

> On 02/15/2015 04:15 AM, 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>

> 

> Sorry but I don't think using strnlen() is appropriate here. See

> http://dpdk.org/ml/archives/dev/2015-February/013309.html

> 

> 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.

[LCM] To me, personally I think either strlen() or strnlen(str, EAL_ARG_MAX) is ok.
Neil's point is that 'eal_parse_common_option()' extern as a EAL global function, itself should avoid the dirty input. That's for security programming.
As strlen()  intended to be used only to calculate the size of incoming untrusted data in a buffer of known size.
Your point is strlen() is enough as it only be used in EAL, and so far all input comes from optarg which is trusted data from getopt_long().
Add Thomas in cc list, I'll submit a v8 to make sure in both case there's patch series ready.
> 

> 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.

> [LCM] I'm afraid not getting your point here. It causes segfault only if the input string is NULL, doesn't it ?

As it already check the case, so using strnlen do protect against the unterminated string.
>

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

> 

> Regards,

> Olivier
  

Patch

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 178e303..62d9120 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -52,6 +52,7 @@ 
 #include "eal_filesystem.h"
 
 #define BITS_PER_HEX 4
+#define EAL_ARG_MAX  4096
 
 const char
 eal_short_options[] =
@@ -167,7 +168,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 = strnlen(coremask, EAL_ARG_MAX);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
 	if (i == 0)
@@ -227,7 +228,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 = strnlen(corelist, EAL_ARG_MAX);
 	while ((i > 0) && isblank(corelist[i - 1]))
 		i--;
 
@@ -472,7 +473,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 = strnlen(lcores, EAL_ARG_MAX);
 	while ((i > 0) && isblank(lcores[i - 1]))
 		i--;