diff mbox

[dpdk-dev,v4,03/17] eal: fix wrong strnlen() return value in 32bit icc

Message ID 1422842559-13617-4-git-send-email-cunming.liang@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Cunming Liang Feb. 2, 2015, 2:02 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>
---
 lib/librte_eal/common/eal_common_options.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Olivier Matz Feb. 8, 2015, 7:59 p.m. UTC | #1
Hi,

On 02/02/2015 03:02 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>
> ---
>  lib/librte_eal/common/eal_common_options.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 29ebb6f..22d5d37 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -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 = strnlen(corelist, PATH_MAX);
>  	while ((i > 0) && isblank(corelist[i - 1]))
>  		i--;
>  
> @@ -469,7 +469,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, PATH_MAX);
>  	while ((i > 0) && isblank(lcores[i - 1]))
>  		i--;
>  
> 

I think PATH_MAX is not equivalent to _SC_ARG_MAX.

But the main question is: why do we need to use strnlen() here instead
of strlen? We can expect that argv[] pointers are always nul-terminated.
Replacing them by strlen() would probably also solve the icc issue.

Regards,
Olivier
Cunming Liang Feb. 9, 2015, 11:57 a.m. UTC | #2
> -----Original Message-----

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

> Sent: Monday, February 09, 2015 4:00 AM

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

> Subject: Re: [dpdk-dev] [PATCH v4 03/17] eal: fix wrong strnlen() return value in

> 32bit icc

> 

> Hi,

> 

> On 02/02/2015 03:02 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>

> > ---

> >  lib/librte_eal/common/eal_common_options.c | 4 ++--

> >  1 file changed, 2 insertions(+), 2 deletions(-)

> >

> > diff --git a/lib/librte_eal/common/eal_common_options.c

> b/lib/librte_eal/common/eal_common_options.c

> > index 29ebb6f..22d5d37 100644

> > --- a/lib/librte_eal/common/eal_common_options.c

> > +++ b/lib/librte_eal/common/eal_common_options.c

> > @@ -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 = strnlen(corelist, PATH_MAX);

> >  	while ((i > 0) && isblank(corelist[i - 1]))

> >  		i--;

> >

> > @@ -469,7 +469,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, PATH_MAX);

> >  	while ((i > 0) && isblank(lcores[i - 1]))

> >  		i--;

> >

> >

> 

> I think PATH_MAX is not equivalent to _SC_ARG_MAX.

> 

> But the main question is: why do we need to use strnlen() here instead

> of strlen? We can expect that argv[] pointers are always nul-terminated.

> Replacing them by strlen() would probably also solve the icc issue.

[LCM] You're right, here strlen() also solve icc issue and no risk for argv[].
But follows practice suggestion, keeping using those with 'n' function in DPDK is not bad.
There's additional two reason to keep strnlen and PATH_MAX.
1. PATH_MAX is defined as 4096 which is enough as our input. It doesn't matter to be _SC_ARG_MAX or not.
2. strnlen and PATH_MAX already used in eal_parse_coremask, to keep the style consistent in '-l' and '--lcores'.

> 

> Regards,

> Olivier
Olivier Matz Feb. 9, 2015, 5:13 p.m. UTC | #3
Hi,

On 02/09/2015 12:57 PM, Liang, Cunming wrote:
>>> @@ -469,7 +469,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, PATH_MAX);
>>>  	while ((i > 0) && isblank(lcores[i - 1]))
>>>  		i--;
>>>
>>>
>>
>> I think PATH_MAX is not equivalent to _SC_ARG_MAX.
>>
>> But the main question is: why do we need to use strnlen() here instead
>> of strlen? We can expect that argv[] pointers are always nul-terminated.
>> Replacing them by strlen() would probably also solve the icc issue.
> [LCM] You're right, here strlen() also solve icc issue and no risk for argv[].
> But follows practice suggestion, keeping using those with 'n' function in DPDK is not bad.
> There's additional two reason to keep strnlen and PATH_MAX.
> 1. PATH_MAX is defined as 4096 which is enough as our input. It doesn't matter to be _SC_ARG_MAX or not.

PATH_MAX is 4096 but it's not related to the maximum argument length.

> 2. strnlen and PATH_MAX already used in eal_parse_coremask, to keep the style consistent in '-l' and '--lcores'.

I don't think it's a valid argument.

What is the problem of using strlen()? It looks it solves all the
issues. Using strlen on valid strings is not a security issue.


Regards,
Olivier
Cunming Liang Feb. 10, 2015, 12:54 a.m. UTC | #4
> -----Original Message-----

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

> Sent: Tuesday, February 10, 2015 1:13 AM

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

> Subject: Re: [dpdk-dev] [PATCH v4 03/17] eal: fix wrong strnlen() return value in

> 32bit icc

> 

> Hi,

> 

> On 02/09/2015 12:57 PM, Liang, Cunming wrote:

> >>> @@ -469,7 +469,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, PATH_MAX);

> >>>  	while ((i > 0) && isblank(lcores[i - 1]))

> >>>  		i--;

> >>>

> >>>

> >>

> >> I think PATH_MAX is not equivalent to _SC_ARG_MAX.

> >>

> >> But the main question is: why do we need to use strnlen() here instead

> >> of strlen? We can expect that argv[] pointers are always nul-terminated.

> >> Replacing them by strlen() would probably also solve the icc issue.

> > [LCM] You're right, here strlen() also solve icc issue and no risk for argv[].

> > But follows practice suggestion, keeping using those with 'n' function in DPDK is

> not bad.

> > There's additional two reason to keep strnlen and PATH_MAX.

> > 1. PATH_MAX is defined as 4096 which is enough as our input. It doesn't matter

> to be _SC_ARG_MAX or not.

> 

> PATH_MAX is 4096 but it's not related to the maximum argument length.

> 

> > 2. strnlen and PATH_MAX already used in eal_parse_coremask, to keep the

> style consistent in '-l' and '--lcores'.

> 

> I don't think it's a valid argument.

> 

> What is the problem of using strlen()? It looks it solves all the

> issues. Using strlen on valid strings is not a security issue.

[LCM] All right, I buy in your point.
> 

> 

> Regards,

> Olivier
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 29ebb6f..22d5d37 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -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 = strnlen(corelist, PATH_MAX);
 	while ((i > 0) && isblank(corelist[i - 1]))
 		i--;
 
@@ -469,7 +469,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, PATH_MAX);
 	while ((i > 0) && isblank(lcores[i - 1]))
 		i--;