[dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC

[dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC

[Anatoly Burakov]

This patchset fixes a few Coverity issues introduced
when various parts of DPDK IPC were added, and explains
away other reported issues.

Coverity issues fixed:
- 272595 - return without mutex unlock
- 272609 - fd leak

Coverity issues intentionally not fixed:
- 260407 - strcpy into fixed size buffer
  - Both src and dst strings are fixed size, so this is false
    positive
  - Hopefully will be silenced by replacing strcpy with strlcpy
- 272565 - strcpy into fixed size buffer
  - Same as above
- 272582 - strcpy into fixed size buffer
  - Same as above
- 268321 - tainted string
  - Not an issue, we handle errors correctly
- 272593 - tainted string
  - Same as above
- 272604 - tainted string
  - Same as above
- 260410 - not checking return value of rte_thread_setname
  - We intentionally don't care if it fails
- 272583 - return without mutex unlock
  - Independently discovered and fixed [1]

[1] http://dpdk.org/dev/patchwork/patch/38042/

Anatoly Burakov (3):
  ipc: use strlcpy where applicable
  ipc: fix return without mutex unlock
  ipc: fix resource leak

 lib/librte_eal/common/eal_common_proc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/3] ipc: use strlcpy where applicable

[Anatoly Burakov]

This also silences (or should silence) a few Coverity false
positives where we used strcpy before (Coverity complained
about not checking buffer size, but source buffers were
always known to be sized correctly).

Coverity issue: 260407
Coverity issue: 272565
Coverity issue: 272582

Fixes: bacaa2754017 ("eal: add channel for multi-process communication")
Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: anatoly.burakov@intel.com
Cc: jianfeng.tan@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index 2179f3d..74bc300 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -129,7 +129,7 @@ create_socket_path(const char *name, char *buf, int len)
 	if (strlen(name) > 0)
 		snprintf(buf, len, "%s_%s", prefix, name);
 	else
-		snprintf(buf, len, "%s", prefix);
+		strlcpy(buf, prefix, len);
 }
 
 int
@@ -200,7 +200,7 @@ rte_mp_action_register(const char *name, rte_mp_t action)
 		rte_errno = ENOMEM;
 		return -1;
 	}
-	strcpy(entry->action_name, name);
+	strlcpy(entry->action_name, name, sizeof(entry->action_name));
 	entry->action = action;
 
 	pthread_mutex_lock(&mp_mutex_action);
@@ -323,8 +323,7 @@ process_msg(struct mp_msg_internal *m, struct sockaddr_un *s)
 			 */
 			struct rte_mp_msg dummy;
 			memset(&dummy, 0, sizeof(dummy));
-			snprintf(dummy.name, sizeof(dummy.name),
-					"%s", msg->name);
+			strlcpy(dummy.name, msg->name, sizeof(dummy.name));
 			mp_send(&dummy, s->sun_path, MP_IGN);
 		} else {
 			RTE_LOG(ERR, EAL, "Cannot find action: %s\n",
@@ -621,11 +620,11 @@ rte_mp_channel_init(void)
 
 	/* create filter path */
 	create_socket_path("*", path, sizeof(path));
-	snprintf(mp_filter, sizeof(mp_filter), "%s", basename(path));
+	strlcpy(mp_filter, basename(path), sizeof(mp_filter));
 
 	/* path may have been modified, so recreate it */
 	create_socket_path("*", path, sizeof(path));
-	snprintf(mp_dir_path, sizeof(mp_dir_path), "%s", dirname(path));
+	strlcpy(mp_dir_path, dirname(path), sizeof(mp_dir_path));
 
 	/* lock the directory */
 	dir_fd = open(mp_dir_path, O_RDONLY);
@@ -673,11 +672,11 @@ rte_mp_channel_init(void)
 	}
 
 	/* try best to set thread name */
-	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "rte_mp_handle");
+	strlcpy(thread_name, "rte_mp_handle", RTE_MAX_THREAD_NAME_LEN);
 	rte_thread_setname(mp_handle_tid, thread_name);
 
 	/* try best to set thread name */
-	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "rte_mp_async_handle");
+	strlcpy(thread_name, "rte_mp_async_handle", RTE_MAX_THREAD_NAME_LEN);
 	rte_thread_setname(async_reply_handle_tid, thread_name);
 
 	/* unlock the directory */
@@ -710,7 +709,7 @@ send_msg(const char *dst_path, struct rte_mp_msg *msg, int type)
 
 	memset(&dst, 0, sizeof(dst));
 	dst.sun_family = AF_UNIX;
-	snprintf(dst.sun_path, sizeof(dst.sun_path), "%s", dst_path);
+	strlcpy(dst.sun_path, dst_path, sizeof(dst.sun_path));
 
 	memset(&msgh, 0, sizeof(msgh));
 	memset(control, 0, sizeof(control));
@@ -870,7 +869,7 @@ mp_request_async(const char *dst, struct rte_mp_msg *req,
 	memset(reply_msg, 0, sizeof(*reply_msg));
 
 	sync_req->type = REQUEST_TYPE_ASYNC;
-	strcpy(sync_req->dst, dst);
+	strlcpy(sync_req->dst, dst, sizeof(sync_req->dst));
 	sync_req->request = req;
 	sync_req->reply = reply_msg;
 	sync_req->async.param = param;
@@ -916,7 +915,7 @@ mp_request_sync(const char *dst, struct rte_mp_msg *req,
 
 	sync_req.type = REQUEST_TYPE_SYNC;
 	sync_req.reply_received = 0;
-	strcpy(sync_req.dst, dst);
+	strlcpy(sync_req.dst, dst, sizeof(sync_req.dst));
 	sync_req.request = req;
 	sync_req.reply = &msg;
 	pthread_cond_init(&sync_req.sync.cond, NULL);
-- 
2.7.4

[dpdk-dev] [PATCH 2/3] ipc: fix return without mutex unlock

[Anatoly Burakov]

gettimeofday() returning a negative value is highly unlikely,
but if it ever happens, we will exit without unlocking the mutex.
Arguably at that point we'll have bigger problems, but fix this
issue anyway.

Coverity issue: 272595

Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index 74bc300..ad02c00 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -525,6 +525,7 @@ async_reply_handle(void *arg __rte_unused)
 		wait_for_async_messages();
 
 		if (gettimeofday(&now, NULL) < 0) {
+			pthread_mutex_unlock(&pending_requests.lock);
 			RTE_LOG(ERR, EAL, "Cannot get current time\n");
 			break;
 		}
-- 
2.7.4

[dpdk-dev] [PATCH 3/3] ipc: fix resource leak

[Anatoly Burakov]

Coverity issue: 272609

Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index ad02c00..a329708 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -658,6 +658,7 @@ rte_mp_channel_init(void)
 		RTE_LOG(ERR, EAL, "failed to create mp thead: %s\n",
 			strerror(errno));
 		close(mp_fd);
+		close(dir_fd);
 		mp_fd = -1;
 		return -1;
 	}
-- 
2.7.4

Re: [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC

[Tan, Jianfeng]

On 4/17/2018 11:46 PM, Anatoly Burakov wrote:
> This patchset fixes a few Coverity issues introduced
> when various parts of DPDK IPC were added, and explains
> away other reported issues.
>
> Coverity issues fixed:
> - 272595 - return without mutex unlock
> - 272609 - fd leak
>
> Coverity issues intentionally not fixed:
> - 260407 - strcpy into fixed size buffer
>    - Both src and dst strings are fixed size, so this is false
>      positive
>    - Hopefully will be silenced by replacing strcpy with strlcpy
> - 272565 - strcpy into fixed size buffer
>    - Same as above
> - 272582 - strcpy into fixed size buffer
>    - Same as above
> - 268321 - tainted string
>    - Not an issue, we handle errors correctly
> - 272593 - tainted string
>    - Same as above
> - 272604 - tainted string
>    - Same as above
> - 260410 - not checking return value of rte_thread_setname
>    - We intentionally don't care if it fails
> - 272583 - return without mutex unlock
>    - Independently discovered and fixed [1]
>
> [1] http://dpdk.org/dev/patchwork/patch/38042/
>
> Anatoly Burakov (3):
>    ipc: use strlcpy where applicable
>    ipc: fix return without mutex unlock
>    ipc: fix resource leak
>
>   lib/librte_eal/common/eal_common_proc.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
>

For this series,

Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thanks for fixing these issues.

Re: [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC

[Thomas Monjalon]

> > Anatoly Burakov (3):
> >    ipc: use strlcpy where applicable
> >    ipc: fix return without mutex unlock
> >    ipc: fix resource leak
> >
> >   lib/librte_eal/common/eal_common_proc.c | 23 ++++++++++++-----------
> >   1 file changed, 12 insertions(+), 11 deletions(-)
> >
> 
> For this series,
> 
> Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Applied, thanks