Coverity fixes for DPDK IPC

PATCH Applied Anatoly Burakov 3 participants 6 messages Apr 17, 2018 – Apr 23, 2018

Share & tools

Anatoly Burakov

author

2018-04-17 15:46 UTC #0 lore raw

cover Coverity fixes for DPDK IPC

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

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-17 15:46 UTC #1 lore raw

PATCH 1/3 ipc: use strlcpy where applicable

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 [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

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

lib/librte_eal/common/eal_common_proc.c +10 ~~-11~~

index 2179f3d..74bc300 100644
@@ -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);

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-17 15:46 UTC #2 lore raw

PATCH 2/3 ipc: fix return without mutex unlock

In reply to 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 [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/eal_common_proc.c | 1 +
 1 file changed, 1 insertion(+)

lib/librte_eal/common/eal_common_proc.c +1 -0

index 74bc300..ad02c00 100644
@@ -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;
 		}

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-17 15:46 UTC #3 lore raw

PATCH 3/3 ipc: fix resource leak

In reply to Anatoly Burakov

Coverity issue: 272609

Fixes f05e26051c15 ("eal: add IPC asynchronous request")

Cc [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/eal_common_proc.c | 1 +
 1 file changed, 1 insertion(+)

lib/librte_eal/common/eal_common_proc.c +1 -0

index ad02c00..a329708 100644
@@ -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;
 	}

Signature · 1 line

2.7.4

Tan, Jianfeng

Acked-by on the v1 series

2018-04-20 14:40 UTC #4 lore raw

In reply to Anatoly Burakov

Quoted 30 lines · 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/

Quoted 8 lines

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 <[email protected]>

Thanks for fixing these issues.

Thomas Monjalon

Applied

2018-04-23 20:34 UTC #5 lore raw

Quoted 5 lines

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

Quoted 7 lines

Quoted 3 lines
  lib/librte_eal/common/eal_common_proc.c | 23 ++++++++++++-----------
  1 file changed, 12 insertions(+), 11 deletions(-)
For this series,
Acked-by Jianfeng Tan <[email protected]>

Applied, thanks