On Mon, Apr 27, 2020 at 3:46 PM Sunil Kumar Kori <sk...@marvell.com> wrote:
>
> >-----Original Message-----
> >From: David Marchand <david.march...@redhat.com>
> >Sent: Monday, April 27, 2020 5:56 PM
> >To: Sunil Kumar Kori <sk...@marvell.com>
> >Cc: Jerin Jacob Kollanukkaran <jer...@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 <sk...@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 <sk...@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
