DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC
@ 2018-04-17 15:46 Anatoly Burakov
  2018-04-17 15:46 ` [dpdk-dev] [PATCH 1/3] ipc: use strlcpy where applicable Anatoly Burakov
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Anatoly Burakov @ 2018-04-17 15:46 UTC (permalink / raw)
  To: dev; +Cc: thomas

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-dev] [PATCH 1/3] ipc: use strlcpy where applicable
  2018-04-17 15:46 [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Anatoly Burakov
@ 2018-04-17 15:46 ` Anatoly Burakov
  2018-04-17 15:46 ` [dpdk-dev] [PATCH 2/3] ipc: fix return without mutex unlock Anatoly Burakov
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Anatoly Burakov @ 2018-04-17 15:46 UTC (permalink / raw)
  To: dev; +Cc: thomas, anatoly.burakov, jianfeng.tan

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-dev] [PATCH 2/3] ipc: fix return without mutex unlock
  2018-04-17 15:46 [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Anatoly Burakov
  2018-04-17 15:46 ` [dpdk-dev] [PATCH 1/3] ipc: use strlcpy where applicable Anatoly Burakov
@ 2018-04-17 15:46 ` Anatoly Burakov
  2018-04-17 15:46 ` [dpdk-dev] [PATCH 3/3] ipc: fix resource leak Anatoly Burakov
  2018-04-20 14:40 ` [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Tan, Jianfeng
  3 siblings, 0 replies; 6+ messages in thread
From: Anatoly Burakov @ 2018-04-17 15:46 UTC (permalink / raw)
  To: dev; +Cc: thomas, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-dev] [PATCH 3/3] ipc: fix resource leak
  2018-04-17 15:46 [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Anatoly Burakov
  2018-04-17 15:46 ` [dpdk-dev] [PATCH 1/3] ipc: use strlcpy where applicable Anatoly Burakov
  2018-04-17 15:46 ` [dpdk-dev] [PATCH 2/3] ipc: fix return without mutex unlock Anatoly Burakov
@ 2018-04-17 15:46 ` Anatoly Burakov
  2018-04-20 14:40 ` [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Tan, Jianfeng
  3 siblings, 0 replies; 6+ messages in thread
From: Anatoly Burakov @ 2018-04-17 15:46 UTC (permalink / raw)
  To: dev; +Cc: thomas, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC
  2018-04-17 15:46 [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Anatoly Burakov
                   ` (2 preceding siblings ...)
  2018-04-17 15:46 ` [dpdk-dev] [PATCH 3/3] ipc: fix resource leak Anatoly Burakov
@ 2018-04-20 14:40 ` Tan, Jianfeng
  2018-04-23 20:34   ` Thomas Monjalon
  3 siblings, 1 reply; 6+ messages in thread
From: Tan, Jianfeng @ 2018-04-20 14:40 UTC (permalink / raw)
  To: Anatoly Burakov, dev; +Cc: thomas



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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC
  2018-04-20 14:40 ` [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Tan, Jianfeng
@ 2018-04-23 20:34   ` Thomas Monjalon
  0 siblings, 0 replies; 6+ messages in thread
From: Thomas Monjalon @ 2018-04-23 20:34 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev, Tan, Jianfeng

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-04-23 20:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-17 15:46 [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Anatoly Burakov
2018-04-17 15:46 ` [dpdk-dev] [PATCH 1/3] ipc: use strlcpy where applicable Anatoly Burakov
2018-04-17 15:46 ` [dpdk-dev] [PATCH 2/3] ipc: fix return without mutex unlock Anatoly Burakov
2018-04-17 15:46 ` [dpdk-dev] [PATCH 3/3] ipc: fix resource leak Anatoly Burakov
2018-04-20 14:40 ` [dpdk-dev] [PATCH 0/3] Coverity fixes for DPDK IPC Tan, Jianfeng
2018-04-23 20:34   ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).