DPDK patches and discussions
 help / color / mirror / Atom feed
From: Sunil Kumar Kori <skori@marvell.com>
To: David Marchand <david.marchand@redhat.com>
Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>, dev <dev@dpdk.org>
Subject: Re: [dpdk-dev] [EXT] Re:  [PATCH] eal/trace: fix coverity issues
Date: Mon, 27 Apr 2020 14:00:57 +0000	[thread overview]
Message-ID: <BY5PR18MB3105C5C6EC9287336BBD8BE0B4AF0@BY5PR18MB3105.namprd18.prod.outlook.com> (raw)
In-Reply-To: <CAJFAV8yyyfkeZNt+ojCnOKhAQCB_p=QH+DhB-ic7Nh21y4mJwg@mail.gmail.com>

>-----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


  reply	other threads:[~2020-04-27 14:01 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-27 12:04 [dpdk-dev] " 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 [this message]
2020-04-30 13:59       ` Sunil Kumar Kori

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BY5PR18MB3105C5C6EC9287336BBD8BE0B4AF0@BY5PR18MB3105.namprd18.prod.outlook.com \
    --to=skori@marvell.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=jerinj@marvell.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).