[dpdk-dev] app/testpmd: fix argument cannot be negative

Message ID 20170725105954.82081-1-danielx.t.mrzyglod@intel.com (mailing list archive)
State Changes Requested, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Daniel Mrzyglod July 25, 2017, 10:59 a.m. UTC
  Coverity issue: 143454
Fixes: a92a5a2cbbff ("app/testpmd: add command for loading DDP")

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
 app/test-pmd/config.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
  

Comments

Van Haaren, Harry July 25, 2017, 12:16 p.m. UTC | #1
Hi Daniel,


> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Daniel Mrzyglod
> Sent: Tuesday, July 25, 2017 12:00 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Mrzyglod, DanielX T <danielx.t.mrzyglod@intel.com>
> Subject: [dpdk-dev] [PATCH] app/testpmd: fix argument cannot be negative

Sorry for the nit-pick review, but there should be some description
here in the commit message, describing what was fixed. I see it is
stated in the title, but a short paragraph stating the issue and
what wrong effect the patch fixes is very useful in git bisect :)

Comments on code below, -Harry

> Coverity issue: 143454
> Fixes: a92a5a2cbbff ("app/testpmd: add command for loading DDP")
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
>  app/test-pmd/config.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index ee6644d10..b77fb96e1 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3292,7 +3292,7 @@ uint8_t *
>  open_ddp_package_file(const char *file_path, uint32_t *size)
>  {
>  	FILE *fh = fopen(file_path, "rb");
> -	uint32_t pkg_size;
> +	off_t pkg_size;

I don't see a mention of  off_t  anywhere else in this file,
perhaps use a commonly used datatype that is suitable
to hold the "long int" that ftell() returns, eg: int64_t ?

>  	uint8_t *buf = NULL;
>  	int ret = 0;
> 
> @@ -3312,6 +3312,12 @@ open_ddp_package_file(const char *file_path, uint32_t *size)
>  	}
> 
>  	pkg_size = ftell(fh);
> +	if (pkg_size == -1) {
> +		fclose(fh);
> +		printf("%s: The stream specified is not a seekable stream\n"
> +				, __func__);
> +		return buf;
> +	}

Given that malloc(0) is implementation defined, we should probably check
to ensure that pkg_size is > 0 before malloc(). Perhaps not explicitly
called out by coverity in this case, but worth fixing as that code is
being modified already.

> 
>  	buf = (uint8_t *)malloc(pkg_size);
>  	if (!buf) {
> --
> 2.13.3
  

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index ee6644d10..b77fb96e1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3292,7 +3292,7 @@  uint8_t *
 open_ddp_package_file(const char *file_path, uint32_t *size)
 {
 	FILE *fh = fopen(file_path, "rb");
-	uint32_t pkg_size;
+	off_t pkg_size;
 	uint8_t *buf = NULL;
 	int ret = 0;
 
@@ -3312,6 +3312,12 @@  open_ddp_package_file(const char *file_path, uint32_t *size)
 	}
 
 	pkg_size = ftell(fh);
+	if (pkg_size == -1) {
+		fclose(fh);
+		printf("%s: The stream specified is not a seekable stream\n"
+				, __func__);
+		return buf;
+	}
 
 	buf = (uint8_t *)malloc(pkg_size);
 	if (!buf) {