trace: fix build with gcc 10
Checks
Commit Message
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(-)
Comments
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.
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
>
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
>
> -----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
>-----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
>>
> -----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.
@@ -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;
}