[dpdk-dev] [PATCH] trace: fix build with gcc 10

[dpdk-dev] [PATCH] trace: fix build with gcc 10

[Phil Yang]

GCC 10 compiling output:
eal_common_trace_utils.c: In function 'eal_trace_dir_args_save':
eal_common_trace_utils.c:290:24: error: '__builtin___sprintf_chk'   \
	may write a terminating nul past the end of the destination \
	[-Werror=format-overflow=]
  290 |  sprintf(dir_path, "%s/", optarg);
      |                        ^

Fixes: 8af866df8d8c ("trace: add trace directory configuration parameter")

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Lijian Zhang <lijian.zhang@arm.com>
Tested-by: Lijian Zhang <lijian.zhang@arm.com>
---
 lib/librte_eal/common/eal_common_trace_utils.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_trace_utils.c b/lib/librte_eal/common/eal_common_trace_utils.c
index fce8892..c079642 100644
--- a/lib/librte_eal/common/eal_common_trace_utils.c
+++ b/lib/librte_eal/common/eal_common_trace_utils.c
@@ -276,7 +276,10 @@ eal_trace_dir_args_save(char const *optarg)
 		return -EINVAL;
 	}
 
-	if (strlen(optarg) >= size) {
+	/* the specified trace directory name cannot
+	 * exceed PATH_MAX-1.
+	 */
+	if (strlen(optarg) >= (size - 1)) {
 		trace_err("input string is too big");
 		return -ENAMETOOLONG;
 	}
-- 
2.7.4

Re: [dpdk-dev] [PATCH] trace: fix build with gcc 10

[Stephen Hemminger]

On Tue, 28 Apr 2020 00:47:38 +0800
Phil Yang <phil.yang@arm.com> wrote:

> -	if (strlen(optarg) >= size) {
> +	/* the specified trace directory name cannot
> +	 * exceed PATH_MAX-1.
> +	 */
> +	if (strlen(optarg) >= (size - 1)) {
>  		trace_err("input string is too big");

strnlen() is useful for these kinds of cases.

[dpdk-dev] [EXT] [PATCH] trace: fix build with gcc 10

[Sunil Kumar Kori]

Sent from Workspace ONE Boxer
On 27-Apr-2020 10:18 PM, Phil Yang <phil.yang@arm.com> wrote:
>
> External Email
>
> ----------------------------------------------------------------------
> GCC 10 compiling output:
> eal_common_trace_utils.c: In function 'eal_trace_dir_args_save':
> eal_common_trace_utils.c:290:24: error: '__builtin___sprintf_chk'   \
>         may write a terminating nul past the end of the destination \
>         [-Werror=format-overflow=]
>   290 |  sprintf(dir_path, "%s/", optarg);
>       |                        ^
>
> Fixes: 8af866df8d8c ("trace: add trace directory configuration parameter")
>
Hello, there is one more thread going on regarding this. Please have a look on below patch.
http://patches.dpdk.org/patch/69382/

I have two points:
1. Will this patch resolves both mentioned warnings/error in  patch 69382 ?
2. David has suggested another way of doing it. Please check that too.

> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Lijian Zhang <lijian.zhang@arm.com>
> Tested-by: Lijian Zhang <lijian.zhang@arm.com>
> ---
>  lib/librte_eal/common/eal_common_trace_utils.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/eal_common_trace_utils.c b/lib/librte_eal/common/eal_common_trace_utils.c
> index fce8892..c079642 100644
> --- a/lib/librte_eal/common/eal_common_trace_utils.c
> +++ b/lib/librte_eal/common/eal_common_trace_utils.c
> @@ -276,7 +276,10 @@ eal_trace_dir_args_save(char const *optarg)
>                  return -EINVAL;
>          }
>
> -       if (strlen(optarg) >= size) {
> +       /* the specified trace directory name cannot
> +        * exceed PATH_MAX-1.
> +        */
> +       if (strlen(optarg) >= (size - 1)) {
>                  trace_err("input string is too big");
>                  return -ENAMETOOLONG;
>          }
> --
> 2.7.4
>

Re: [dpdk-dev] [EXT] [PATCH] trace: fix build with gcc 10

[Phil Yang]

From: Sunil Kumar Kori <skori@marvell.com> 
Sent: Tuesday, April 28, 2020 11:49 AM
To: Phil Yang <Phil.Yang@arm.com>; jerinj@marvell.com; dev@dpdk.org
Cc: David Marchand <david.marchand@redhat.com>; Ruifeng Wang <Ruifeng.Wang@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>; nd <nd@arm.com>
Subject: [EXT] [PATCH] trace: fix build with gcc 10

Sent from Workspace ONE Boxer
On 27-Apr-2020 10:18 PM, Phil Yang <mailto:phil.yang@arm.com> wrote: 
>
> External Email 
>
> ---------------------------------------------------------------------- 
> GCC 10 compiling output: 
> eal_common_trace_utils.c: In function 'eal_trace_dir_args_save': 
> eal_common_trace_utils.c:290:24: error: '__builtin___sprintf_chk'   \ 
>         may write a terminating nul past the end of the destination \ 
>         [-Werror=format-overflow=] 
>   290 |  sprintf(dir_path, "%s/", optarg); 
>       |                        ^ 
>
> Fixes: 8af866df8d8c ("trace: add trace directory configuration parameter") 
>
Hello, there is one more thread going on regarding this. Please have a look on below patch. 
http://patches.dpdk.org/patch/69382/

Hi Sunil,

Sorry, I didn’t notice that. Thanks for the link. 

I have two points:
1. Will this patch resolves both mentioned warnings/error in  patch 69382 ?
[Phil] Yes, this patch resolved the same issue mentioned by David in patch 69382.

2. David has suggested another way of doing it. Please check that too.
[Phil]  I think both David’s and my patches are correct.
My patch can guarantee a correct ‘size’ information in snprinf(). It omits the memory allocation operation for the incorrect input arguments case.
David’s suggestion resolves the potential directory copy fail issue and it saves some memory space in the normal case. But it needs to allocate memory in the incorrect input case.

So, I think we can bind these two patches together?

Thanks,
Phil

> Signed-off-by: Phil Yang <mailto:phil.yang@arm.com> 
> Reviewed-by: Lijian Zhang <mailto:lijian.zhang@arm.com> 
> Tested-by: Lijian Zhang <mailto:lijian.zhang@arm.com> 
> --- 
>  lib/librte_eal/common/eal_common_trace_utils.c | 5 ++++- 
>  1 file changed, 4 insertions(+), 1 deletion(-) 
>
> diff --git a/lib/librte_eal/common/eal_common_trace_utils.c b/lib/librte_eal/common/eal_common_trace_utils.c 
> index fce8892..c079642 100644 
> --- a/lib/librte_eal/common/eal_common_trace_utils.c 
> +++ b/lib/librte_eal/common/eal_common_trace_utils.c 
> @@ -276,7 +276,10 @@ eal_trace_dir_args_save(char const *optarg) 
>                  return -EINVAL; 
>          } 
>   
> -       if (strlen(optarg) >= size) { 
> +       /* the specified trace directory name cannot 
> +        * exceed PATH_MAX-1. 
> +        */ 
> +       if (strlen(optarg) >= (size - 1)) { 
>                  trace_err("input string is too big"); 
>                  return -ENAMETOOLONG; 
>          } 
> -- 
> 2.7.4 
>

Re: [dpdk-dev] [PATCH] trace: fix build with gcc 10

[Phil Yang]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, April 28, 2020 12:59 AM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: jerinj@marvell.com; skori@marvell.com; dev@dpdk.org;
> david.marchand@redhat.com; Lijian Zhang <Lijian.Zhang@arm.com>;
> Ruifeng Wang <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] trace: fix build with gcc 10
> 
> On Tue, 28 Apr 2020 00:47:38 +0800
> Phil Yang <phil.yang@arm.com> wrote:
> 
> > -	if (strlen(optarg) >= size) {
> > +	/* the specified trace directory name cannot
> > +	 * exceed PATH_MAX-1.
> > +	 */
> > +	if (strlen(optarg) >= (size - 1)) {
> >  		trace_err("input string is too big");
> 
> strnlen() is useful for these kinds of cases.

Thanks, Stephen.
I think it checks the dir_name length here, not to trim the input 'optarg'.
So I think the strlen() can handle it correctly.

Thanks,
Phil 

Re: [dpdk-dev] [EXT] [PATCH] trace: fix build with gcc 10

[Sunil Kumar Kori]

>-----Original Message-----
>From: Phil Yang <Phil.Yang@arm.com>
>Sent: Tuesday, April 28, 2020 12:30 PM
>To: Sunil Kumar Kori <skori@marvell.com>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; dev@dpdk.org
>Cc: David Marchand <david.marchand@redhat.com>; Ruifeng Wang
><Ruifeng.Wang@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>; nd
><nd@arm.com>; nd <nd@arm.com>
>Subject: RE: [EXT] [PATCH] trace: fix build with gcc 10
>
>From: Sunil Kumar Kori <skori@marvell.com>
>Sent: Tuesday, April 28, 2020 11:49 AM
>To: Phil Yang <Phil.Yang@arm.com>; jerinj@marvell.com; dev@dpdk.org
>Cc: David Marchand <david.marchand@redhat.com>; Ruifeng Wang
><Ruifeng.Wang@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>; nd
><nd@arm.com>
>Subject: [EXT] [PATCH] trace: fix build with gcc 10
>
>Sent from Workspace ONE Boxer
>On 27-Apr-2020 10:18 PM, Phil Yang <mailto:phil.yang@arm.com> wrote:
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> GCC 10 compiling output:
>> eal_common_trace_utils.c: In function 'eal_trace_dir_args_save':
>> eal_common_trace_utils.c:290:24: error: '__builtin___sprintf_chk'   \
>>         may write a terminating nul past the end of the destination \
>>         [-Werror=format-overflow=]
>>   290 |  sprintf(dir_path, "%s/", optarg);
>>       |                        ^
>>
>> Fixes: 8af866df8d8c ("trace: add trace directory configuration parameter")
>>
>Hello, there is one more thread going on regarding this. Please have a look on
>below patch.
>https://urldefense.proofpoint.com/v2/url?u=http-
>3A__patches.dpdk.org_patch_69382_&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7x
>tfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d9IIuq6vHQO6NrIPjaE&m=WFCcD0EavY
>uGMZUUoJWIQunwTAwgxAju2rK4s3Nr-t4&s=iMF8PSGMB8S-
>rDR0kJGOZ1el3MzeOKfxZQxX-Oyg54g&e=
>
>Hi Sunil,
>
>Sorry, I didn’t notice that. Thanks for the link.
>
>I have two points:
>1. Will this patch resolves both mentioned warnings/error in  patch 69382 ?
>[Phil] Yes, this patch resolved the same issue mentioned by David in patch
>69382.
>
>2. David has suggested another way of doing it. Please check that too.
>[Phil]  I think both David’s and my patches are correct.
>My patch can guarantee a correct ‘size’ information in snprinf(). It omits the
>memory allocation operation for the incorrect input arguments case.
>David’s suggestion resolves the potential directory copy fail issue and it saves
>some memory space in the normal case. But it needs to allocate memory in
>the incorrect input case.
>
>So, I think we can bind these two patches together?
Make sense.
So can you please combine both the patches and share ?

>
>Thanks,
>Phil
>
>> Signed-off-by: Phil Yang <mailto:phil.yang@arm.com>
>> Reviewed-by: Lijian Zhang <mailto:lijian.zhang@arm.com>
>> Tested-by: Lijian Zhang <mailto:lijian.zhang@arm.com>
>> ---
>>  lib/librte_eal/common/eal_common_trace_utils.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_trace_utils.c
>b/lib/librte_eal/common/eal_common_trace_utils.c
>> index fce8892..c079642 100644
>> --- a/lib/librte_eal/common/eal_common_trace_utils.c
>> +++ b/lib/librte_eal/common/eal_common_trace_utils.c
>> @@ -276,7 +276,10 @@ eal_trace_dir_args_save(char const *optarg)
>>                  return -EINVAL;
>>          }
>>
>> -       if (strlen(optarg) >= size) {
>> +       /* the specified trace directory name cannot
>> +        * exceed PATH_MAX-1.
>> +        */
>> +       if (strlen(optarg) >= (size - 1)) {
>>                  trace_err("input string is too big");
>>                  return -ENAMETOOLONG;
>>          }
>> --
>> 2.7.4
>>

Re: [dpdk-dev] [EXT] [PATCH] trace: fix build with gcc 10

[Phil Yang]

> -----Original Message-----
> From: Sunil Kumar Kori <skori@marvell.com>
> Sent: Tuesday, April 28, 2020 6:52 PM
> To: Phil Yang <Phil.Yang@arm.com>; jerinj@marvell.com; dev@dpdk.org
> Cc: David Marchand <david.marchand@redhat.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>; nd
> <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [EXT] [PATCH] trace: fix build with gcc 10
> 

<snip>

> >>
> >Hello, there is one more thread going on regarding this. Please have a look
> on
> >below patch.
> >https://urldefense.proofpoint.com/v2/url?u=http-
> >3A__patches.dpdk.org_patch_69382_&d=DwIGaQ&c=nKjWec2b6R0mOyPa
> z7x
> >tfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d9IIuq6vHQO6NrIPjaE&m=WFCcD0E
> avY
> >uGMZUUoJWIQunwTAwgxAju2rK4s3Nr-t4&s=iMF8PSGMB8S-
> >rDR0kJGOZ1el3MzeOKfxZQxX-Oyg54g&e=
> >
> >Hi Sunil,
> >
> >Sorry, I didn’t notice that. Thanks for the link.
> >
> >I have two points:
> >1. Will this patch resolves both mentioned warnings/error in  patch 69382 ?
> >[Phil] Yes, this patch resolved the same issue mentioned by David in patch
> >69382.
> >
> >2. David has suggested another way of doing it. Please check that too.
> >[Phil]  I think both David’s and my patches are correct.
> >My patch can guarantee a correct ‘size’ information in snprinf(). It omits the
> >memory allocation operation for the incorrect input arguments case.
> >David’s suggestion resolves the potential directory copy fail issue and it
> saves
> >some memory space in the normal case. But it needs to allocate memory in
> >the incorrect input case.
> >
> >So, I think we can bind these two patches together?
> Make sense.
> So can you please combine both the patches and share ?
> 

Sure. 
I will update it in v2.

[dpdk-dev] [PATCH v2] trace: fix build with gcc 10

[Phil Yang]

Prevent from writing beyond the allocated memory.

GCC 10 compiling output:
eal_common_trace_utils.c: In function 'eal_trace_dir_args_save':
eal_common_trace_utils.c:290:24: error: '__builtin___sprintf_chk'   \
	may write a terminating nul past the end of the destination \
	[-Werror=format-overflow=]
  290 |  sprintf(dir_path, "%s/", optarg);
      |                        ^

Fixes: 8af866df8d8c ("trace: add trace directory configuration parameter")

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Lijian Zhang <lijian.zhang@arm.com>
Tested-by: Lijian Zhang <lijian.zhang@arm.com>
---
v2:
use asprintf instead of sprintf.

 lib/librte_eal/common/eal_common_trace_utils.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_trace_utils.c b/lib/librte_eal/common/eal_common_trace_utils.c
index fce8892..2ffb8af 100644
--- a/lib/librte_eal/common/eal_common_trace_utils.c
+++ b/lib/librte_eal/common/eal_common_trace_utils.c
@@ -268,7 +268,7 @@ eal_trace_dir_args_save(char const *optarg)
 {
 	struct trace *trace = trace_obj_get();
 	uint32_t size = sizeof(trace->dir);
-	char *dir_path = NULL;
+	char *dir_path;
 	int rc;
 
 	if (optarg == NULL) {
@@ -276,18 +276,20 @@ eal_trace_dir_args_save(char const *optarg)
 		return -EINVAL;
 	}
 
-	if (strlen(optarg) >= size) {
+	/* the specified trace directory name cannot
+	 * exceed PATH_MAX-1.
+	 */
+	if (strlen(optarg) >= (size - 1)) {
 		trace_err("input string is too big");
 		return -ENAMETOOLONG;
 	}
 
-	dir_path = (char *)calloc(1, size);
-	if (dir_path == NULL) {
-		trace_err("fail to allocate memory");
+	rc = asprintf(&dir_path, "%s/", optarg);
+	if (rc == -1) {
+		trace_err("failed to copy directory: %s", strerror(errno));
 		return -ENOMEM;
 	}
 
-	sprintf(dir_path, "%s/", optarg);
 	rc = trace_dir_update(dir_path);
 
 	free(dir_path);
-- 
2.7.4

Re: [dpdk-dev] [EXT] [PATCH v2] trace: fix build with gcc 10

[Sunil Kumar Kori]

Looks good to me.

Regards
Sunil Kumar Kori

>-----Original Message-----
>From: Phil Yang <phil.yang@arm.com>
>Sent: Tuesday, April 28, 2020 8:07 PM
>To: Sunil Kumar Kori <skori@marvell.com>; dev@dpdk.org
>Cc: david.marchand@redhat.com; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; lijian.zhang@arm.com; ruifeng.wang@arm.com;
>nd@arm.com
>Subject: [EXT] [PATCH v2] trace: fix build with gcc 10
>
>External Email
>
>----------------------------------------------------------------------
>Prevent from writing beyond the allocated memory.
>
>GCC 10 compiling output:
>eal_common_trace_utils.c: In function 'eal_trace_dir_args_save':
>eal_common_trace_utils.c:290:24: error: '__builtin___sprintf_chk'   \
>	may write a terminating nul past the end of the destination \
>	[-Werror=format-overflow=]
>  290 |  sprintf(dir_path, "%s/", optarg);
>      |                        ^
>
>Fixes: 8af866df8d8c ("trace: add trace directory configuration parameter")
>
>Signed-off-by: Phil Yang <phil.yang@arm.com>
>Reviewed-by: Lijian Zhang <lijian.zhang@arm.com>
>Tested-by: Lijian Zhang <lijian.zhang@arm.com>

Acked-by: Sunil Kumar Kori <skori@marvell.com>
>---
>v2:
>use asprintf instead of sprintf.
>
> lib/librte_eal/common/eal_common_trace_utils.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
>diff --git a/lib/librte_eal/common/eal_common_trace_utils.c
>b/lib/librte_eal/common/eal_common_trace_utils.c
>index fce8892..2ffb8af 100644
>--- a/lib/librte_eal/common/eal_common_trace_utils.c
>+++ b/lib/librte_eal/common/eal_common_trace_utils.c
>@@ -268,7 +268,7 @@ eal_trace_dir_args_save(char const *optarg)  {
> 	struct trace *trace = trace_obj_get();
> 	uint32_t size = sizeof(trace->dir);
>-	char *dir_path = NULL;
>+	char *dir_path;
> 	int rc;
>
> 	if (optarg == NULL) {
>@@ -276,18 +276,20 @@ eal_trace_dir_args_save(char const *optarg)
> 		return -EINVAL;
> 	}
>
>-	if (strlen(optarg) >= size) {
>+	/* the specified trace directory name cannot
>+	 * exceed PATH_MAX-1.
>+	 */
>+	if (strlen(optarg) >= (size - 1)) {
> 		trace_err("input string is too big");
> 		return -ENAMETOOLONG;
> 	}
>
>-	dir_path = (char *)calloc(1, size);
>-	if (dir_path == NULL) {
>-		trace_err("fail to allocate memory");
>+	rc = asprintf(&dir_path, "%s/", optarg);
>+	if (rc == -1) {
>+		trace_err("failed to copy directory: %s", strerror(errno));
> 		return -ENOMEM;
> 	}
>
>-	sprintf(dir_path, "%s/", optarg);
> 	rc = trace_dir_update(dir_path);
>
> 	free(dir_path);
>--
>2.7.4