[dpdk-dev] [PATCH 0/4] net/virtio-user: fix coverity issues

[dpdk-dev] [PATCH 0/4] net/virtio-user: fix coverity issues

[Jianfeng Tan]

Patch 1: fix return value not checked, Coverity issue: 127344, 127478
Patch 2: fix string overflow, Coverity issue: 127484
Patch 3: fix resource leaks, Coverity issue: 127482
Patch 4: fix string unterminated, Coverity issue: 127476

Jianfeng Tan (4):
  net/virtio-user: fix return value not checked
  net/virtio-user: fix string overflow
  net/virtio-user: fix resource leaks
  net/virtio-user: fix string unterminated

 drivers/net/virtio/virtio_user/vhost_user.c      |  5 +-
 drivers/net/virtio/virtio_user/virtio_user_dev.c |  2 +-
 drivers/net/virtio/virtio_user_ethdev.c          | 61 ++++++++++++++++++------
 3 files changed, 50 insertions(+), 18 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH 1/4] net/virtio-user: fix return value not checked

[Jianfeng Tan]

When return values of function calls are not checked, Coverity will
report errors like:

    if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_PATH) == 1)
    >>>     CID 127477:    (CHECKED_RETURN)
    >>>     Calling "rte_kvargs_process" without checking return value
            (as is done elsewhere 25 out of 30 times).
         		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PATH,
         				   &get_string_arg, &path);

Coverity issue: 127344, 127478

Fixes: ce2eabdd43ec ("net/virtio-user: add virtual device")
Fixes: 6a84c37e3975 ("net/virtio-user: add vhost-user adapter layer")

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user/vhost_user.c |  3 +-
 drivers/net/virtio/virtio_user_ethdev.c     | 57 ++++++++++++++++++++++-------
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index a2b0687..a159ece 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -392,7 +392,8 @@ vhost_user_setup(const char *path)
 	}
 
 	flag = fcntl(fd, F_GETFD);
-	fcntl(fd, F_SETFD, flag | FD_CLOEXEC);
+	if (fcntl(fd, F_SETFD, flag | FD_CLOEXEC) < 0)
+		PMD_DRV_LOG(WARNING, "fcntl failed, %s", strerror(errno));
 
 	memset(&un, 0, sizeof(un));
 	un.sun_family = AF_UNIX;
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 5ab2471..8429b2e 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -343,29 +343,58 @@ virtio_user_pmd_devinit(const char *name, const char *params)
 	}
 
 	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_PATH) == 1)
-		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PATH,
-				   &get_string_arg, &path);
+		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PATH,
+					 &get_string_arg, &path);
+		if (ret < 0) {
+			PMD_INIT_LOG(ERR, "error to parse %s",
+				     VIRTIO_USER_ARG_PATH);
+			goto end;
+		}
 	else {
 		PMD_INIT_LOG(ERR, "arg %s is mandatory for virtio-user\n",
 			  VIRTIO_USER_ARG_QUEUE_SIZE);
 		goto end;
 	}
 
-	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_MAC) == 1)
-		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_MAC,
-				   &get_string_arg, &mac_addr);
+	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_MAC) == 1) {
+		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_MAC,
+					 &get_string_arg, &mac_addr);
+		if (ret < 0) {
+			PMD_INIT_LOG(ERR, "error to parse %s",
+				     VIRTIO_USER_ARG_MAC);
+			goto end;
+		}
+	}
 
-	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE) == 1)
-		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE,
-				   &get_integer_arg, &queue_size);
+	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE) == 1) {
+		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE,
+					 &get_integer_arg, &queue_size);
+		if (ret < 0) {
+			PMD_INIT_LOG(ERR, "error to parse %s",
+				     VIRTIO_USER_ARG_QUEUE_SIZE);
+			goto end;
+		}
+	}
 
-	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_QUEUES_NUM) == 1)
-		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUES_NUM,
-				   &get_integer_arg, &queues);
+	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_QUEUES_NUM) == 1) {
+		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUES_NUM,
+					 &get_integer_arg, &queues);
+		if (ret < 0) {
+			PMD_INIT_LOG(ERR, "error to parse %s",
+				     VIRTIO_USER_ARG_QUEUES_NUM);
+			goto end;
+		}
+	}
 
-	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1)
-		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
-				   &get_integer_arg, &cq);
+	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1) {
+		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
+					 &get_integer_arg, &cq);
+		if (ret < 0) {
+			PMD_INIT_LOG(ERR, "error to parse %s",
+				     VIRTIO_USER_ARG_CQ_NUM);
+			goto end;
+		}
+	}
 	else if (queues > 1)
 		cq = 1;
 
-- 
2.1.4

[dpdk-dev] [PATCH 2/4] net/virtio-user: fix string overflow

[Jianfeng Tan]

When parsing /proc/self/maps to get hugepage information, the string
was being copied with strcpy(), which could, theoretically but in fact
not possiblly, overflow the destination buffer. Anyway, to avoid the
false alarm, we replaced strncpy with snprintf for safely copying the
strings.

Coverity issue: 127484

Fixes: 6a84c37e3975 ("net/virtio-user: add vhost-user adapter layer")

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user/vhost_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index a159ece..082e821 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -181,7 +181,7 @@ get_hugepage_file_info(struct hugepage_file_info huges[], int max)
 		}
 		huges[idx].addr = v_start;
 		huges[idx].size = v_end - v_start;
-		strcpy(huges[idx].path, tmp);
+		snprintf(huges[idx].path, PATH_MAX, "%s", tmp);
 		idx++;
 	}
 
-- 
2.1.4

[dpdk-dev] [PATCH 3/4] net/virtio-user: fix resource leaks

[Jianfeng Tan]

The return value by rte_kvargs_parse is not free(d), which leads
to memory leak.

Coverity issue: 127482

Fixes: ce2eabdd43ec ("net/virtio-user: add virtual device")

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 8429b2e..8e39adf 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -320,7 +320,7 @@ virtio_user_eth_dev_alloc(const char *name)
 static int
 virtio_user_pmd_devinit(const char *name, const char *params)
 {
-	struct rte_kvargs *kvlist;
+	struct rte_kvargs *kvlist = NULL;
 	struct rte_eth_dev *eth_dev;
 	struct virtio_hw *hw;
 	uint64_t queues = VIRTIO_USER_DEF_Q_NUM;
@@ -422,6 +422,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
 	ret = 0;
 
 end:
+	if (kvlist)
+		rte_kvargs_free(kvlist);
 	if (path)
 		free(path);
 	if (mac_addr)
-- 
2.1.4

[dpdk-dev] [PATCH 4/4] net/virtio-user: fix string unterminated

[Jianfeng Tan]

When use strcpy() to copy string with length exceeding the last
parameter of strcpy(), it may lead to the destination string
unterminated.

We replaced strncpy with snprintf to make sure it's NULL terminated.

Coverity issue: 127476

Fixes: ce2eabdd43ec ("net/virtio-user: add virtual device")

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 1b1e5bf..376c9cf 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -181,7 +181,7 @@ int
 virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		     int cq, int queue_size, const char *mac)
 {
-	strncpy(dev->path, path, PATH_MAX);
+	snprintf(dev->path, PATH_MAX, "%s", path);
 	dev->max_queue_pairs = queues;
 	dev->queue_pairs = 1; /* mq disabled by default */
 	dev->queue_size = queue_size;
-- 
2.1.4

Re: [dpdk-dev] [PATCH 1/4] net/virtio-user: fix return value not checked

[Yuanhan Liu]

On Wed, Jun 29, 2016 at 09:05:33AM +0000, Jianfeng Tan wrote:
> -	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1)
> -		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
> -				   &get_integer_arg, &cq);
> +	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1) {
> +		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
> +					 &get_integer_arg, &cq);
> +		if (ret < 0) {
> +			PMD_INIT_LOG(ERR, "error to parse %s",
> +				     VIRTIO_USER_ARG_CQ_NUM);
> +			goto end;
> +		}
> +	}
>  	else if (queues > 1)

Above 2 lines should be in one line. Fixed, and series applied to
dpdk-next-virtio.

Thanks.

	--yliu

Re: [dpdk-dev] [PATCH 1/4] net/virtio-user: fix return value not checked

[Tan, Jianfeng]

Hi Yuanhan,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Friday, July 1, 2016 10:16 AM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Xie, Huawei; Mcnamara, John
> Subject: Re: [PATCH 1/4] net/virtio-user: fix return value not checked
> 
> On Wed, Jun 29, 2016 at 09:05:33AM +0000, Jianfeng Tan wrote:
> > -	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1)
> > -		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
> > -				   &get_integer_arg, &cq);
> > +	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1) {
> > +		ret = rte_kvargs_process(kvlist,
> VIRTIO_USER_ARG_CQ_NUM,
> > +					 &get_integer_arg, &cq);
> > +		if (ret < 0) {
> > +			PMD_INIT_LOG(ERR, "error to parse %s",
> > +				     VIRTIO_USER_ARG_CQ_NUM);
> > +			goto end;
> > +		}
> > +	}
> >  	else if (queues > 1)
> 
> Above 2 lines should be in one line. Fixed, and series applied to
> dpdk-next-virtio.

Thanks.

But there's another problem in the commit log: the Coverity defect ID should be "127477, 127478"  instead of "127344, 127478". Can you please help make it right?

Thanks,
Jianfeng

> 
> Thanks.
> 
> 	--yliu

Re: [dpdk-dev] [PATCH 1/4] net/virtio-user: fix return value not checked

[Yuanhan Liu]

On Tue, Jul 05, 2016 at 10:14:28AM +0000, Tan, Jianfeng wrote:
> > On Wed, Jun 29, 2016 at 09:05:33AM +0000, Jianfeng Tan wrote:
> > > -	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1)
> > > -		rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
> > > -				   &get_integer_arg, &cq);
> > > +	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1) {
> > > +		ret = rte_kvargs_process(kvlist,
> > VIRTIO_USER_ARG_CQ_NUM,
> > > +					 &get_integer_arg, &cq);
> > > +		if (ret < 0) {
> > > +			PMD_INIT_LOG(ERR, "error to parse %s",
> > > +				     VIRTIO_USER_ARG_CQ_NUM);
> > > +			goto end;
> > > +		}
> > > +	}
> > >  	else if (queues > 1)
> > 
> > Above 2 lines should be in one line. Fixed, and series applied to
> > dpdk-next-virtio.
> 
> Thanks.
> 
> But there's another problem in the commit log: the Coverity defect ID should be "127477, 127478"  instead of "127344, 127478". Can you please help make it right?

Done.

	--yliu