DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan
@ 2014-12-16 15:03 Bruce Richardson
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 1/5] test: after NULL check, don't free the NULL pointer Bruce Richardson
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Bruce Richardson @ 2014-12-16 15:03 UTC (permalink / raw)
  To: dev

This patch set fixes 5 issues found during a static analysis scan of the latest
DPDK code. These fixes are for possible NULL pointer references and array 
overflow/underflow.

Bruce Richardson (5):
  test: after NULL check, don't free the NULL pointer
  test: check for mbuf alloc failure
  examples: set correct limit for length of unix socket path
  examples: fix check for null before de-reference
  cfgfile: prevent error when reading an empty file

 app/test/test_kvargs.c                      | 3 ---
 app/test/test_table_acl.c                   | 5 +++++
 examples/vm_power_manager/channel_manager.c | 4 ++--
 examples/vm_power_manager/channel_manager.h | 8 +++++++-
 examples/vm_power_manager/vm_power_cli.c    | 4 ++--
 lib/librte_cfgfile/rte_cfgfile.c            | 4 +++-
 6 files changed, 19 insertions(+), 9 deletions(-)

-- 
1.9.3

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

* [dpdk-dev] [PATCH 1/5] test: after NULL check, don't free the NULL pointer
  2014-12-16 15:03 [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Bruce Richardson
@ 2014-12-16 15:03 ` Bruce Richardson
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 2/5] test: check for mbuf alloc failure Bruce Richardson
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Bruce Richardson @ 2014-12-16 15:03 UTC (permalink / raw)
  To: dev

In the kvargs test cases, we were checking for errors by checking if the
returned pointer value was NULL. In the error handling, we then tried to
free back the NULL pointer, which would cause a crash.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_kvargs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/app/test/test_kvargs.c b/app/test/test_kvargs.c
index b8f5e5c..6be8512 100644
--- a/app/test/test_kvargs.c
+++ b/app/test/test_kvargs.c
@@ -78,7 +78,6 @@ static int test_valid_kvargs(void)
 	kvlist = rte_kvargs_parse(args, valid_keys);
 	if (kvlist == NULL) {
 		printf("rte_kvargs_parse() error");
-		rte_kvargs_free(kvlist);
 		goto fail;
 	}
 	rte_kvargs_free(kvlist);
@@ -89,7 +88,6 @@ static int test_valid_kvargs(void)
 	kvlist = rte_kvargs_parse(args, valid_keys);
 	if (kvlist == NULL) {
 		printf("rte_kvargs_parse() error");
-		rte_kvargs_free(kvlist);
 		goto fail;
 	}
 	/* call check_handler() for all entries with key="check" */
@@ -150,7 +148,6 @@ static int test_valid_kvargs(void)
 	kvlist = rte_kvargs_parse(args, valid_keys);
 	if (kvlist == NULL) {
 		printf("rte_kvargs_parse() error");
-		rte_kvargs_free(kvlist);
 		goto fail;
 	}
 	/* call check_handler() on all entries with key="check", it
-- 
1.9.3

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

* [dpdk-dev] [PATCH 2/5] test: check for mbuf alloc failure
  2014-12-16 15:03 [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Bruce Richardson
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 1/5] test: after NULL check, don't free the NULL pointer Bruce Richardson
@ 2014-12-16 15:03 ` Bruce Richardson
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 3/5] examples: set correct limit for length of unix socket path Bruce Richardson
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Bruce Richardson @ 2014-12-16 15:03 UTC (permalink / raw)
  To: dev

If mbuf allocation failed for whatever reason, we would get a NULL
pointer exception in test_table_acl.c:test_pipeline_single_filter test
case.
We fix this by causing an early break out of the application loop. If we
quit the test immediately we would leak any existing allocated mbufs,
but by breaking instead, we allow the test to continue and clean up the
mbufs already in the pipeline, while still having a test failure as the
mbuf counts should not match.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_table_acl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/app/test/test_table_acl.c b/app/test/test_table_acl.c
index 0f2b57e..e4e9b9c 100644
--- a/app/test/test_table_acl.c
+++ b/app/test/test_table_acl.c
@@ -513,6 +513,11 @@ test_pipeline_single_filter(int expected_count)
 			struct rte_mbuf *mbuf;
 
 			mbuf = rte_pktmbuf_alloc(pool);
+			if (mbuf == NULL)
+				/* this will cause test failure after cleanup
+				 * of already enqueued mbufs, as the mbuf
+				 * counts won't match */
+				break;
 			memset(rte_pktmbuf_mtod(mbuf, char *), 0x00,
 				sizeof(struct ipv4_5tuple));
 
-- 
1.9.3

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

* [dpdk-dev] [PATCH 3/5] examples: set correct limit for length of unix socket path
  2014-12-16 15:03 [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Bruce Richardson
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 1/5] test: after NULL check, don't free the NULL pointer Bruce Richardson
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 2/5] test: check for mbuf alloc failure Bruce Richardson
@ 2014-12-16 15:03 ` Bruce Richardson
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 4/5] examples: fix check for null before de-reference Bruce Richardson
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Bruce Richardson @ 2014-12-16 15:03 UTC (permalink / raw)
  To: dev

The length of the path to a unix socket is not PATH_MAX but instead is
UNIX_PATH_MAX which is generally just over 100 bytes in size. It's not
actually defined in sys/un.h on linux - despite the man page referencing
it, so calculate the size in the case where it's not defined.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 examples/vm_power_manager/channel_manager.c | 2 +-
 examples/vm_power_manager/channel_manager.h | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
index 7828be7..34a395d 100644
--- a/examples/vm_power_manager/channel_manager.c
+++ b/examples/vm_power_manager/channel_manager.c
@@ -597,7 +597,7 @@ get_info_vm(const char *vm_name, struct vm_info *info)
 	ITERATIVE_BITMASK_CHECK_64(mask, i) {
 		info->channels[channel_num].channel_num = i;
 		memcpy(info->channels[channel_num].channel_path,
-				vm_info->channels[i]->channel_path, PATH_MAX);
+				vm_info->channels[i]->channel_path, UNIX_PATH_MAX);
 		info->channels[channel_num].status = vm_info->channels[i]->status;
 		info->channels[channel_num].fd = vm_info->channels[i]->fd;
 		channel_num++;
diff --git a/examples/vm_power_manager/channel_manager.h b/examples/vm_power_manager/channel_manager.h
index 12c29c3..67e26ec 100644
--- a/examples/vm_power_manager/channel_manager.h
+++ b/examples/vm_power_manager/channel_manager.h
@@ -39,6 +39,7 @@ extern "C" {
 #endif
 
 #include <linux/limits.h>
+#include <sys/un.h>
 #include <rte_atomic.h>
 #include "channel_commands.h"
 
@@ -54,6 +55,11 @@ extern "C" {
 /* File socket directory */
 #define CHANNEL_MGR_SOCKET_PATH     "/tmp/powermonitor/"
 
+#ifndef UNIX_PATH_MAX
+struct sockaddr_un _sockaddr_un;
+#define UNIX_PATH_MAX sizeof(_sockaddr_un.sun_path)
+#endif
+
 /* Communication Channel Status */
 enum channel_status { CHANNEL_MGR_CHANNEL_DISCONNECTED = 0,
 	CHANNEL_MGR_CHANNEL_CONNECTED,
@@ -68,7 +74,7 @@ enum vm_status { CHANNEL_MGR_VM_INACTIVE = 0, CHANNEL_MGR_VM_ACTIVE};
  *  the host.
  */
 struct channel_info {
-	char channel_path[PATH_MAX]; /**< Path to host socket */
+	char channel_path[UNIX_PATH_MAX]; /**< Path to host socket */
 	volatile uint32_t status;    /**< Connection status(enum channel_status) */
 	int fd;                      /**< AF_UNIX socket fd */
 	unsigned channel_num;        /**< CHANNEL_MGR_SOCKET_PATH/<vm_name>.channel_num */
-- 
1.9.3

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

* [dpdk-dev] [PATCH 4/5] examples: fix check for null before de-reference
  2014-12-16 15:03 [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Bruce Richardson
                   ` (2 preceding siblings ...)
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 3/5] examples: set correct limit for length of unix socket path Bruce Richardson
@ 2014-12-16 15:03 ` Bruce Richardson
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 5/5] cfgfile: prevent error when reading an empty file Bruce Richardson
  2014-12-16 23:53 ` [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Thomas Monjalon
  5 siblings, 0 replies; 7+ messages in thread
From: Bruce Richardson @ 2014-12-16 15:03 UTC (permalink / raw)
  To: dev

The check for NULL is in the wrong position in the "if" error leg. The
pointer should be checked for NULL before checking what the value of
what the pointer points to is.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 examples/vm_power_manager/channel_manager.c | 2 +-
 examples/vm_power_manager/vm_power_cli.c    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
index 34a395d..04344ae 100644
--- a/examples/vm_power_manager/channel_manager.c
+++ b/examples/vm_power_manager/channel_manager.c
@@ -389,7 +389,7 @@ add_all_channels(const char *vm_name)
 		errno = 0;
 		channel_num = (unsigned)strtol(remaining, &tail_ptr, 0);
 		if ((errno != 0) || (remaining[0] == '\0') ||
-				(*tail_ptr != '\0') || tail_ptr == NULL) {
+				tail_ptr == NULL || (*tail_ptr != '\0')) {
 			RTE_LOG(WARNING, CHANNEL_MANAGER, "Malformed channel name"
 					"'%s' found it should be in the form of "
 					"'<guest_name>.<channel_num>(decimal)'\n",
diff --git a/examples/vm_power_manager/vm_power_cli.c b/examples/vm_power_manager/vm_power_cli.c
index e7f4469..bd685fd 100644
--- a/examples/vm_power_manager/vm_power_cli.c
+++ b/examples/vm_power_manager/vm_power_cli.c
@@ -323,7 +323,7 @@ cmd_channels_op_parsed(void *parsed_result, struct cmdline *cl,
 			break;
 		errno = 0;
 		channel_num = (unsigned)strtol(token, &tail_ptr, 10);
-		if ((errno != 0) || (*tail_ptr != '\0') || tail_ptr == NULL)
+		if ((errno != 0) || tail_ptr == NULL || (*tail_ptr != '\0'))
 			break;
 
 		if (channel_num == CHANNEL_CMDS_MAX_VM_CHANNELS) {
@@ -408,7 +408,7 @@ cmd_channels_status_op_parsed(void *parsed_result, struct cmdline *cl,
 			break;
 		errno = 0;
 		channel_num = (unsigned)strtol(token, &tail_ptr, 10);
-		if ((errno != 0) || (*tail_ptr != '\0') || tail_ptr == NULL)
+		if ((errno != 0) || tail_ptr == NULL || (*tail_ptr != '\0'))
 			break;
 
 		if (channel_num == CHANNEL_CMDS_MAX_VM_CHANNELS) {
-- 
1.9.3

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

* [dpdk-dev] [PATCH 5/5] cfgfile: prevent error when reading an empty file
  2014-12-16 15:03 [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Bruce Richardson
                   ` (3 preceding siblings ...)
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 4/5] examples: fix check for null before de-reference Bruce Richardson
@ 2014-12-16 15:03 ` Bruce Richardson
  2014-12-16 23:53 ` [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Thomas Monjalon
  5 siblings, 0 replies; 7+ messages in thread
From: Bruce Richardson @ 2014-12-16 15:03 UTC (permalink / raw)
  To: dev

If the file to be read by the cfgfile is empty, i.e. no configuration
data, but possibly comments present, the cfgfile should not mark the
last processed section (curr_section) as having N entries, since there
is no last processed section.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index f2bc2cc..b81c273 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -225,8 +225,10 @@ rte_cfgfile_load(const char *filename, int flags)
 	}
 	fclose(f);
 	cfg->flags = flags;
-	cfg->sections[curr_section]->num_entries = curr_entry + 1;
 	cfg->num_sections = curr_section + 1;
+	/* curr_section will still be -1 if we have an empty file */
+	if (curr_section >= 0)
+		cfg->sections[curr_section]->num_entries = curr_entry + 1;
 	return cfg;
 
 error1:
-- 
1.9.3

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

* Re: [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan
  2014-12-16 15:03 [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Bruce Richardson
                   ` (4 preceding siblings ...)
  2014-12-16 15:03 ` [dpdk-dev] [PATCH 5/5] cfgfile: prevent error when reading an empty file Bruce Richardson
@ 2014-12-16 23:53 ` Thomas Monjalon
  5 siblings, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2014-12-16 23:53 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

> This patch set fixes 5 issues found during a static analysis scan of the latest
> DPDK code. These fixes are for possible NULL pointer references and array 
> overflow/underflow.
> 
> Bruce Richardson (5):
>   test: after NULL check, don't free the NULL pointer
>   test: check for mbuf alloc failure
>   examples: set correct limit for length of unix socket path
>   examples: fix check for null before de-reference
>   cfgfile: prevent error when reading an empty file

Applied

Thanks
-- 
Thomas

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

end of thread, other threads:[~2014-12-16 23:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-16 15:03 [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan Bruce Richardson
2014-12-16 15:03 ` [dpdk-dev] [PATCH 1/5] test: after NULL check, don't free the NULL pointer Bruce Richardson
2014-12-16 15:03 ` [dpdk-dev] [PATCH 2/5] test: check for mbuf alloc failure Bruce Richardson
2014-12-16 15:03 ` [dpdk-dev] [PATCH 3/5] examples: set correct limit for length of unix socket path Bruce Richardson
2014-12-16 15:03 ` [dpdk-dev] [PATCH 4/5] examples: fix check for null before de-reference Bruce Richardson
2014-12-16 15:03 ` [dpdk-dev] [PATCH 5/5] cfgfile: prevent error when reading an empty file Bruce Richardson
2014-12-16 23:53 ` [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan 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).