* [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).