[dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan

[dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan

[Bruce Richardson]

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

[dpdk-dev] [PATCH 1/5] test: after NULL check, don't free the NULL pointer

[Bruce Richardson]

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

[dpdk-dev] [PATCH 2/5] test: check for mbuf alloc failure

[Bruce Richardson]

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

[dpdk-dev] [PATCH 3/5] examples: set correct limit for length of unix socket path

[Bruce Richardson]

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

[dpdk-dev] [PATCH 4/5] examples: fix check for null before de-reference

[Bruce Richardson]

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

[dpdk-dev] [PATCH 5/5] cfgfile: prevent error when reading an empty file

[Bruce Richardson]

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

Re: [dpdk-dev] [PATCH 0/5] Fixes for issues highlighted by static analysis scan

[Thomas Monjalon]

> 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