* [dpdk-dev] [PATCH] eal/trace: fix coverity issues
@ 2020-04-27 12:04 Sunil Kumar Kori
2020-04-27 12:26 ` David Marchand
0 siblings, 1 reply; 6+ messages in thread
From: Sunil Kumar Kori @ 2020-04-27 12:04 UTC (permalink / raw)
To: Jerin Jacob, Sunil Kumar Kori; +Cc: dev
Pointer was being dereferenced without NULL checking.
Coverity issue: 357768
Fixes: 8c8066ea6a7b ("trace: add trace mode configuration parameter")
Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
---
lib/librte_eal/common/eal_common_trace_utils.c | 3 ++-
1 file changed, 2 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 fce8892c3..119e97119 100644
--- a/lib/librte_eal/common/eal_common_trace_utils.c
+++ b/lib/librte_eal/common/eal_common_trace_utils.c
@@ -227,15 +227,16 @@ int
eal_trace_mode_args_save(const char *optarg)
{
struct trace *trace = trace_obj_get();
- size_t len = strlen(optarg);
unsigned long tmp;
char *pattern;
+ size_t len;
if (optarg == NULL) {
trace_err("no optarg is passed");
return -EINVAL;
}
+ len = strlen(optarg);
if (len == 0) {
trace_err("value is not provided with option");
return -EINVAL;
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] eal/trace: fix coverity issues
2020-04-27 12:04 [dpdk-dev] [PATCH] eal/trace: fix coverity issues Sunil Kumar Kori
@ 2020-04-27 12:26 ` David Marchand
2020-04-27 13:46 ` [dpdk-dev] [EXT] " Sunil Kumar Kori
0 siblings, 1 reply; 6+ messages in thread
From: David Marchand @ 2020-04-27 12:26 UTC (permalink / raw)
To: Sunil Kumar Kori; +Cc: Jerin Jacob, dev
On Mon, Apr 27, 2020 at 2:04 PM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> Pointer was being dereferenced without NULL checking.
>
> Coverity issue: 357768
>
> Fixes: 8c8066ea6a7b ("trace: add trace mode configuration parameter")
>
> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
> ---
> lib/librte_eal/common/eal_common_trace_utils.c | 3 ++-
> 1 file changed, 2 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 fce8892c3..119e97119 100644
> --- a/lib/librte_eal/common/eal_common_trace_utils.c
> +++ b/lib/librte_eal/common/eal_common_trace_utils.c
> @@ -227,15 +227,16 @@ int
> eal_trace_mode_args_save(const char *optarg)
> {
> struct trace *trace = trace_obj_get();
> - size_t len = strlen(optarg);
> unsigned long tmp;
> char *pattern;
> + size_t len;
>
> if (optarg == NULL) {
> trace_err("no optarg is passed");
> return -EINVAL;
> }
>
> + len = strlen(optarg);
> if (len == 0) {
> trace_err("value is not provided with option");
> return -EINVAL;
I was looking at some gcc 10 complaints on string manipulation later
in eal_trace_dir_args_save().
https://build.opensuse.org/package/live_build_log/home:dmarchan:branches:home:bluca:dpdk/dpdk/Fedora_Rawhide/x86_64
[ 126s] CC rte_malloc.o
[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:
In function 'eal_trace_dir_args_save':
[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:290:24:
error: 'sprintf' may write a terminating nul past the end of the
destination [-Werror=format-overflow=]
[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
[ 127s] | ^
[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:290:2:
note: 'sprintf' output between 2 and 4097 bytes into a destination of
size 4096
[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
[ 127s] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[ 127s] cc1: all warnings being treated as errors
Could we use asprintf in all this code and avoid malloc + sprintf ?
Something like the _untested_ snippet?
diff --git a/lib/librte_eal/common/eal_common_trace_utils.c
b/lib/librte_eal/common/eal_common_trace_utils.c
index fce8892c38..4769bade97 100644
--- a/lib/librte_eal/common/eal_common_trace_utils.c
+++ b/lib/librte_eal/common/eal_common_trace_utils.c
@@ -267,8 +267,7 @@ int
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,19 +275,18 @@ eal_trace_dir_args_save(char const *optarg)
return -EINVAL;
}
- if (strlen(optarg) >= size) {
- 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);
+ if ((size_t)rc >= sizeof(trace->dir)) {
+ trace_err("input string is too big");
+ rc = -ENAMETOOLONG;
+ } else {
+ rc = trace_dir_update(dir_path);
+ }
free(dir_path);
return rc;
--
David Marchand
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH] eal/trace: fix coverity issues
2020-04-27 12:26 ` David Marchand
@ 2020-04-27 13:46 ` Sunil Kumar Kori
2020-04-27 13:52 ` David Marchand
0 siblings, 1 reply; 6+ messages in thread
From: Sunil Kumar Kori @ 2020-04-27 13:46 UTC (permalink / raw)
To: David Marchand; +Cc: Jerin Jacob Kollanukkaran, dev
>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Monday, April 27, 2020 5:56 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev <dev@dpdk.org>
>Subject: [EXT] Re: [dpdk-dev] [PATCH] eal/trace: fix coverity issues
>
>External Email
>
>----------------------------------------------------------------------
>On Mon, Apr 27, 2020 at 2:04 PM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>>
>> Pointer was being dereferenced without NULL checking.
>>
>> Coverity issue: 357768
>>
>> Fixes: 8c8066ea6a7b ("trace: add trace mode configuration parameter")
>>
>> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
>> ---
>> lib/librte_eal/common/eal_common_trace_utils.c | 3 ++-
>> 1 file changed, 2 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 fce8892c3..119e97119 100644
>> --- a/lib/librte_eal/common/eal_common_trace_utils.c
>> +++ b/lib/librte_eal/common/eal_common_trace_utils.c
>> @@ -227,15 +227,16 @@ int
>> eal_trace_mode_args_save(const char *optarg) {
>> struct trace *trace = trace_obj_get();
>> - size_t len = strlen(optarg);
>> unsigned long tmp;
>> char *pattern;
>> + size_t len;
>>
>> if (optarg == NULL) {
>> trace_err("no optarg is passed");
>> return -EINVAL;
>> }
>>
>> + len = strlen(optarg);
>> if (len == 0) {
>> trace_err("value is not provided with option");
>> return -EINVAL;
>
>I was looking at some gcc 10 complaints on string manipulation later
>in eal_trace_dir_args_save().
>
>https://urldefense.proofpoint.com/v2/url?u=https-
>3A__build.opensuse.org_package_live-5Fbuild-5Flog_home-3Admarchan-
>3Abranches-3Ahome-3Abluca-3Adpdk_dpdk_Fedora-5FRawhide_x86-
>5F64&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHMy
>aF1_d9IIuq6vHQO6NrIPjaE&m=NZ72Sr2OMEYZD7PIY59lshlAxZJJJepF5oxbHv0j
>5Zg&s=yOCA3PfhZojqJv0iVKlzeqM7tYGVv0jjrnVcajUx_qA&e=
>
>[ 126s] CC rte_malloc.o
>[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:
>In function 'eal_trace_dir_args_save':
>[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:29
>0:24:
>error: 'sprintf' may write a terminating nul past the end of the
>destination [-Werror=format-overflow=]
>[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
>[ 127s] | ^
>[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:29
>0:2:
>note: 'sprintf' output between 2 and 4097 bytes into a destination of
>size 4096
>[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
>[ 127s] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>[ 127s] cc1: all warnings being treated as errors
>
>
>Could we use asprintf in all this code and avoid malloc + sprintf ?
>
As I understood from above warnings/errors, real problem is writing beyond destination i.e. dir_path.
If this is the case then it can be simply handled using snprintf(); with correct "size" information.
Suggested code changes are correct. I am just trying to achieve this with lesser code changes.
Also I think, fix for this should be a separate patch.
Suggestions please ?
>
>Something like the _untested_ snippet?
>
>diff --git a/lib/librte_eal/common/eal_common_trace_utils.c
>b/lib/librte_eal/common/eal_common_trace_utils.c
>index fce8892c38..4769bade97 100644
>--- a/lib/librte_eal/common/eal_common_trace_utils.c
>+++ b/lib/librte_eal/common/eal_common_trace_utils.c
>@@ -267,8 +267,7 @@ int
> 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,19 +275,18 @@ eal_trace_dir_args_save(char const *optarg)
> return -EINVAL;
> }
>
>- if (strlen(optarg) >= size) {
>- 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);
>+ if ((size_t)rc >= sizeof(trace->dir)) {
>+ trace_err("input string is too big");
>+ rc = -ENAMETOOLONG;
>+ } else {
>+ rc = trace_dir_update(dir_path);
>+ }
>
> free(dir_path);
> return rc;
>
>
>
>--
>David Marchand
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH] eal/trace: fix coverity issues
2020-04-27 13:46 ` [dpdk-dev] [EXT] " Sunil Kumar Kori
@ 2020-04-27 13:52 ` David Marchand
2020-04-27 14:00 ` Sunil Kumar Kori
2020-04-30 13:59 ` Sunil Kumar Kori
0 siblings, 2 replies; 6+ messages in thread
From: David Marchand @ 2020-04-27 13:52 UTC (permalink / raw)
To: Sunil Kumar Kori; +Cc: Jerin Jacob Kollanukkaran, dev
On Mon, Apr 27, 2020 at 3:46 PM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> >-----Original Message-----
> >From: David Marchand <david.marchand@redhat.com>
> >Sent: Monday, April 27, 2020 5:56 PM
> >To: Sunil Kumar Kori <skori@marvell.com>
> >Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev <dev@dpdk.org>
> >Subject: [EXT] Re: [dpdk-dev] [PATCH] eal/trace: fix coverity issues
> >
> >External Email
> >
> >----------------------------------------------------------------------
> >On Mon, Apr 27, 2020 at 2:04 PM Sunil Kumar Kori <skori@marvell.com>
> >wrote:
> >>
> >> Pointer was being dereferenced without NULL checking.
> >>
> >> Coverity issue: 357768
> >>
> >> Fixes: 8c8066ea6a7b ("trace: add trace mode configuration parameter")
> >>
> >> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
> >> ---
> >> lib/librte_eal/common/eal_common_trace_utils.c | 3 ++-
> >> 1 file changed, 2 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 fce8892c3..119e97119 100644
> >> --- a/lib/librte_eal/common/eal_common_trace_utils.c
> >> +++ b/lib/librte_eal/common/eal_common_trace_utils.c
> >> @@ -227,15 +227,16 @@ int
> >> eal_trace_mode_args_save(const char *optarg) {
> >> struct trace *trace = trace_obj_get();
> >> - size_t len = strlen(optarg);
> >> unsigned long tmp;
> >> char *pattern;
> >> + size_t len;
> >>
> >> if (optarg == NULL) {
> >> trace_err("no optarg is passed");
> >> return -EINVAL;
> >> }
> >>
> >> + len = strlen(optarg);
> >> if (len == 0) {
> >> trace_err("value is not provided with option");
> >> return -EINVAL;
> >
> >I was looking at some gcc 10 complaints on string manipulation later
> >in eal_trace_dir_args_save().
> >
> >https://urldefense.proofpoint.com/v2/url?u=https-
> >3A__build.opensuse.org_package_live-5Fbuild-5Flog_home-3Admarchan-
> >3Abranches-3Ahome-3Abluca-3Adpdk_dpdk_Fedora-5FRawhide_x86-
> >5F64&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHMy
> >aF1_d9IIuq6vHQO6NrIPjaE&m=NZ72Sr2OMEYZD7PIY59lshlAxZJJJepF5oxbHv0j
> >5Zg&s=yOCA3PfhZojqJv0iVKlzeqM7tYGVv0jjrnVcajUx_qA&e=
> >
> >[ 126s] CC rte_malloc.o
> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
> >1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:
> >In function 'eal_trace_dir_args_save':
> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
> >1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:29
> >0:24:
> >error: 'sprintf' may write a terminating nul past the end of the
> >destination [-Werror=format-overflow=]
> >[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
> >[ 127s] | ^
> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
> >1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:29
> >0:2:
> >note: 'sprintf' output between 2 and 4097 bytes into a destination of
> >size 4096
> >[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
> >[ 127s] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >[ 127s] cc1: all warnings being treated as errors
> >
> >
> >Could we use asprintf in all this code and avoid malloc + sprintf ?
> >
> As I understood from above warnings/errors, real problem is writing beyond destination i.e. dir_path.
> If this is the case then it can be simply handled using snprintf(); with correct "size" information.
> Suggested code changes are correct. I am just trying to achieve this with lesser code changes.
>
> Also I think, fix for this should be a separate patch.
> Suggestions please ?
If the code was not combining this malloc + sprintf + wrong checks
(caught by coverity), we would avoid both issues and it would be more
consistent.
No more suggestion from me.
--
David Marchand
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH] eal/trace: fix coverity issues
2020-04-27 13:52 ` David Marchand
@ 2020-04-27 14:00 ` Sunil Kumar Kori
2020-04-30 13:59 ` Sunil Kumar Kori
1 sibling, 0 replies; 6+ messages in thread
From: Sunil Kumar Kori @ 2020-04-27 14:00 UTC (permalink / raw)
To: David Marchand; +Cc: Jerin Jacob Kollanukkaran, dev
>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Monday, April 27, 2020 7:22 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev <dev@dpdk.org>
>Subject: Re: [EXT] Re: [dpdk-dev] [PATCH] eal/trace: fix coverity issues
>
>On Mon, Apr 27, 2020 at 3:46 PM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>>
>> >-----Original Message-----
>> >From: David Marchand <david.marchand@redhat.com>
>> >Sent: Monday, April 27, 2020 5:56 PM
>> >To: Sunil Kumar Kori <skori@marvell.com>
>> >Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev
>> ><dev@dpdk.org>
>> >Subject: [EXT] Re: [dpdk-dev] [PATCH] eal/trace: fix coverity issues
>> >
>> >External Email
>> >
>> >---------------------------------------------------------------------
>> >- On Mon, Apr 27, 2020 at 2:04 PM Sunil Kumar Kori
>> ><skori@marvell.com>
>> >wrote:
>> >>
>> >> Pointer was being dereferenced without NULL checking.
>> >>
>> >> Coverity issue: 357768
>> >>
>> >> Fixes: 8c8066ea6a7b ("trace: add trace mode configuration
>> >> parameter")
>> >>
>> >> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
>> >> ---
>> >> lib/librte_eal/common/eal_common_trace_utils.c | 3 ++-
>> >> 1 file changed, 2 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 fce8892c3..119e97119 100644
>> >> --- a/lib/librte_eal/common/eal_common_trace_utils.c
>> >> +++ b/lib/librte_eal/common/eal_common_trace_utils.c
>> >> @@ -227,15 +227,16 @@ int
>> >> eal_trace_mode_args_save(const char *optarg) {
>> >> struct trace *trace = trace_obj_get();
>> >> - size_t len = strlen(optarg);
>> >> unsigned long tmp;
>> >> char *pattern;
>> >> + size_t len;
>> >>
>> >> if (optarg == NULL) {
>> >> trace_err("no optarg is passed");
>> >> return -EINVAL;
>> >> }
>> >>
>> >> + len = strlen(optarg);
>> >> if (len == 0) {
>> >> trace_err("value is not provided with option");
>> >> return -EINVAL;
>> >
>> >I was looking at some gcc 10 complaints on string manipulation later
>> >in eal_trace_dir_args_save().
>> >
>> >https://urldefense.proofpoint.com/v2/url?u=https-
>> >3A__build.opensuse.org_package_live-5Fbuild-5Flog_home-3Admarchan-
>> >3Abranches-3Ahome-3Abluca-3Adpdk_dpdk_Fedora-5FRawhide_x86-
>>
>>5F64&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHM
>y
>>
>>aF1_d9IIuq6vHQO6NrIPjaE&m=NZ72Sr2OMEYZD7PIY59lshlAxZJJJepF5oxbHv0
>j
>> >5Zg&s=yOCA3PfhZojqJv0iVKlzeqM7tYGVv0jjrnVcajUx_qA&e=
>> >
>> >[ 126s] CC rte_malloc.o
>> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>> >1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:
>> >In function 'eal_trace_dir_args_save':
>> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>>
>>1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:2
>> >9
>> >0:24:
>> >error: 'sprintf' may write a terminating nul past the end of the
>> >destination [-Werror=format-overflow=]
>> >[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
>> >[ 127s] | ^
>> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>>
>>1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:2
>> >9
>> >0:2:
>> >note: 'sprintf' output between 2 and 4097 bytes into a destination of
>> >size 4096
>> >[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
>> >[ 127s] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >[ 127s] cc1: all warnings being treated as errors
>> >
>> >
>> >Could we use asprintf in all this code and avoid malloc + sprintf ?
>> >
>> As I understood from above warnings/errors, real problem is writing beyond
>destination i.e. dir_path.
>> If this is the case then it can be simply handled using snprintf(); with correct
>"size" information.
>> Suggested code changes are correct. I am just trying to achieve this with
>lesser code changes.
>>
>> Also I think, fix for this should be a separate patch.
>> Suggestions please ?
>
>If the code was not combining this malloc + sprintf + wrong checks (caught by
>coverity), we would avoid both issues and it would be more consistent.
>No more suggestion from me.
Agreed but It must be a separate patch because there could be other warnings too
thrown by GCC 10.x which may require handling.
>
>
>--
>David Marchand
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [EXT] Re: [PATCH] eal/trace: fix coverity issues
2020-04-27 13:52 ` David Marchand
2020-04-27 14:00 ` Sunil Kumar Kori
@ 2020-04-30 13:59 ` Sunil Kumar Kori
1 sibling, 0 replies; 6+ messages in thread
From: Sunil Kumar Kori @ 2020-04-30 13:59 UTC (permalink / raw)
To: David Marchand; +Cc: Jerin Jacob Kollanukkaran, dev
Hello David,
Mentioned patch (http://patches.dpdk.org/patch/69467/ ) takes care of your input to resolve GCC 10 build.
Now I think, this patch is good to go. Please take care of this.
Regards
Sunil Kumar Kori
>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Monday, April 27, 2020 7:22 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev <dev@dpdk.org>
>Subject: Re: [EXT] Re: [dpdk-dev] [PATCH] eal/trace: fix coverity issues
>
>On Mon, Apr 27, 2020 at 3:46 PM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>>
>> >-----Original Message-----
>> >From: David Marchand <david.marchand@redhat.com>
>> >Sent: Monday, April 27, 2020 5:56 PM
>> >To: Sunil Kumar Kori <skori@marvell.com>
>> >Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev
>> ><dev@dpdk.org>
>> >Subject: [EXT] Re: [dpdk-dev] [PATCH] eal/trace: fix coverity issues
>> >
>> >External Email
>> >
>> >---------------------------------------------------------------------
>> >- On Mon, Apr 27, 2020 at 2:04 PM Sunil Kumar Kori
>> ><skori@marvell.com>
>> >wrote:
>> >>
>> >> Pointer was being dereferenced without NULL checking.
>> >>
>> >> Coverity issue: 357768
>> >>
>> >> Fixes: 8c8066ea6a7b ("trace: add trace mode configuration
>> >> parameter")
>> >>
>> >> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
>> >> ---
>> >> lib/librte_eal/common/eal_common_trace_utils.c | 3 ++-
>> >> 1 file changed, 2 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 fce8892c3..119e97119 100644
>> >> --- a/lib/librte_eal/common/eal_common_trace_utils.c
>> >> +++ b/lib/librte_eal/common/eal_common_trace_utils.c
>> >> @@ -227,15 +227,16 @@ int
>> >> eal_trace_mode_args_save(const char *optarg) {
>> >> struct trace *trace = trace_obj_get();
>> >> - size_t len = strlen(optarg);
>> >> unsigned long tmp;
>> >> char *pattern;
>> >> + size_t len;
>> >>
>> >> if (optarg == NULL) {
>> >> trace_err("no optarg is passed");
>> >> return -EINVAL;
>> >> }
>> >>
>> >> + len = strlen(optarg);
>> >> if (len == 0) {
>> >> trace_err("value is not provided with option");
>> >> return -EINVAL;
>> >
>> >I was looking at some gcc 10 complaints on string manipulation later
>> >in eal_trace_dir_args_save().
>> >
>> >https://urldefense.proofpoint.com/v2/url?u=https-
>> >3A__build.opensuse.org_package_live-5Fbuild-5Flog_home-3Admarchan-
>> >3Abranches-3Ahome-3Abluca-3Adpdk_dpdk_Fedora-5FRawhide_x86-
>>
>>5F64&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHM
>y
>>
>>aF1_d9IIuq6vHQO6NrIPjaE&m=NZ72Sr2OMEYZD7PIY59lshlAxZJJJepF5oxbHv0
>j
>> >5Zg&s=yOCA3PfhZojqJv0iVKlzeqM7tYGVv0jjrnVcajUx_qA&e=
>> >
>> >[ 126s] CC rte_malloc.o
>> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>> >1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:
>> >In function 'eal_trace_dir_args_save':
>> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>>
>>1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:2
>> >9
>> >0:24:
>> >error: 'sprintf' may write a terminating nul past the end of the
>> >destination [-Werror=format-overflow=]
>> >[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
>> >[ 127s] | ^
>> >[ 127s] /home/abuild/rpmbuild/BUILD/dpdk-
>>
>>1587835122.b13ace300/lib/librte_eal/common/eal_common_trace_utils.c:2
>> >9
>> >0:2:
>> >note: 'sprintf' output between 2 and 4097 bytes into a destination of
>> >size 4096
>> >[ 127s] 290 | sprintf(dir_path, "%s/", optarg);
>> >[ 127s] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >[ 127s] cc1: all warnings being treated as errors
>> >
>> >
>> >Could we use asprintf in all this code and avoid malloc + sprintf ?
>> >
>> As I understood from above warnings/errors, real problem is writing beyond
>destination i.e. dir_path.
>> If this is the case then it can be simply handled using snprintf(); with correct
>"size" information.
>> Suggested code changes are correct. I am just trying to achieve this with
>lesser code changes.
>>
>> Also I think, fix for this should be a separate patch.
>> Suggestions please ?
>
>If the code was not combining this malloc + sprintf + wrong checks (caught by
>coverity), we would avoid both issues and it would be more consistent.
>No more suggestion from me.
>
>
>--
>David Marchand
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-30 13:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-27 12:04 [dpdk-dev] [PATCH] eal/trace: fix coverity issues Sunil Kumar Kori
2020-04-27 12:26 ` David Marchand
2020-04-27 13:46 ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-04-27 13:52 ` David Marchand
2020-04-27 14:00 ` Sunil Kumar Kori
2020-04-30 13:59 ` Sunil Kumar Kori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).