Re: [dpdk-dev] [EXT] Re: [PATCH] eal/trace: fix coverity issues

[Sunil Kumar Kori <skori@marvell.com>]

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