[PATCH 0/4] fix crash on device args with no driver options

[PATCH 0/4] fix crash on device args with no driver options

[Stephen Hemminger]

A device given in the global syntax with no driver layer, such as
"bus=vdev,name=net_af_packet0", leaves devargs args NULL because
drv_str shares a union with it and is never set.  The legacy syntax
always sets args, to an empty string when there are no options, and
drivers rely on that.  Passing the NULL to rte_kvargs_parse() crashes
in strdup().

The first patch makes the global syntax match the legacy one by
defaulting to an empty string.  The second hardens rte_kvargs_parse()
so a NULL gives an empty list rather than a crash; NULL is not
excluded by the API documentation, and an empty list is the sensible
reading of "no arguments".

The last two patches add the missing coverage.  There was no test for
a device string without driver arguments, which is how this went
unnoticed.  Converting the devargs test to the unit test suite runner
comes first so the new case is reported on its own rather than
short-circuiting the rest of the file.

Verified both new tests fail without the fixes: the devargs case trips
its args-NULL assertion, and the kvargs case faults in strdup().

Bugzilla ID: 2049

Stephen Hemminger (4):
  devargs: fix NULL args with global device syntax
  kvargs: harden rte_kvargs_parse
  test/devargs: use unit test suite runner
  test/devargs: add tests for missing device arguments

 app/test/test_devargs.c             | 101 ++++++++++++++++++++++------
 app/test/test_kvargs.c              |  16 +++++
 lib/eal/common/eal_common_devargs.c |   4 ++
 lib/kvargs/rte_kvargs.c             |   6 +-
 4 files changed, 106 insertions(+), 21 deletions(-)

-- 
2.53.0

[PATCH 1/4] devargs: fix NULL args with global device syntax

[Stephen Hemminger]

A device given in the global syntax without a driver layer,
e.g. "bus=vdev,name=net_af_packet0", leaves drv_str,
and so args which shares its union, set to NULL.
The legacy syntax always sets args, to an empty string when there
are no options, and drivers rely on that.
Passing NULL to rte_kvargs_parse() crashes in strdup().

Default drv_str to an empty string when there is no driver layer.

Bugzilla ID: 2049
Fixes: b344eb5d941a ("devargs: parse global device syntax")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/common/eal_common_devargs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index c523429d67..8bbff9cd15 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -154,6 +154,10 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		}
 	}
 
+	/* No driver layer: match the legacy syntax, empty args. */
+	if (devargs->drv_str == NULL)
+		devargs->drv_str = "";
+
 	/* Resolve devargs name. */
 	if (devargs->bus != NULL && devargs->bus->devargs_parse != NULL)
 		ret = devargs->bus->devargs_parse(devargs);
-- 
2.53.0

[PATCH 2/4] kvargs: harden rte_kvargs_parse

[Stephen Hemminger]

Make rte_kvargs_parse(NULL, ...) return an empty list rather than crash.
NULL is not excluded by the API doc, and an empty list matches
"no arguments" so safer.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/kvargs/rte_kvargs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/kvargs/rte_kvargs.c b/lib/kvargs/rte_kvargs.c
index 4e3198b33f..89c1b74308 100644
--- a/lib/kvargs/rte_kvargs.c
+++ b/lib/kvargs/rte_kvargs.c
@@ -272,6 +272,10 @@ rte_kvargs_parse(const char *args, const char * const valid_keys[])
 		return NULL;
 	memset(kvlist, 0, sizeof(*kvlist));
 
+	/* Treat NULL as empty string */
+	if (args == NULL)
+		return kvlist;
+
 	if (rte_kvargs_tokenize(kvlist, args) < 0) {
 		rte_kvargs_free(kvlist);
 		return NULL;
@@ -294,7 +298,7 @@ rte_kvargs_parse_delim(const char *args, const char * const valid_keys[],
 	char *copy;
 	size_t len;
 
-	if (valid_ends == NULL)
+	if (args == NULL || valid_ends == NULL)
 		return rte_kvargs_parse(args, valid_keys);
 
 	copy = strdup(args);
-- 
2.53.0

[PATCH 3/4] test/devargs: use unit test suite runner

[Stephen Hemminger]

The devargs test ran its four subtests from a single function, so a
failure reported only one overall result and later subtests were
skipped once an earlier one failed.

Convert to unit_test_suite_runner() so each subtest is a named case
that runs and reports independently.

Two subtests also recorded failure by assigning the parse return
value to the result, which hid a failure when that value was 0.
Use TEST_FAILED instead.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_devargs.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index 571f57500c..de0dd2364d 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -179,14 +179,14 @@ test_invalid_devargs(void)
 	struct rte_devargs da;
 	uint32_t i;
 	int ret;
-	int fail = 0;
+	int fail = TEST_SUCCESS;
 
 	for (i = 0; i < RTE_DIM(list); i++) {
 		ret = rte_devargs_parse(&da, list[i]);
 		if (ret >= 0) {
 			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
 			       list[i], ret);
-			fail = ret;
+			fail = TEST_FAILED;
 		}
 		rte_devargs_reset(&da);
 	}
@@ -233,7 +233,7 @@ test_valid_devargs_parsing(void)
 	struct rte_eth_devargs eth_da[RTE_MAX_ETHPORTS];
 	uint32_t i;
 	int ret;
-	int fail = 0;
+	int fail = TEST_SUCCESS;
 
 	for (i = 0; i < RTE_DIM(list); i++) {
 		memset(eth_da, 0, RTE_MAX_ETHPORTS * sizeof(*eth_da));
@@ -241,7 +241,7 @@ test_valid_devargs_parsing(void)
 		if (ret <= 0) {
 			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
 			       list[i].devargs, ret);
-			fail = ret;
+			fail = TEST_FAILED;
 			break;
 		}
 
@@ -249,7 +249,7 @@ test_valid_devargs_parsing(void)
 		if (ret != list[i].devargs_count) {
 			printf("Devargs returned count %d != expected count %d\n", ret,
 			       list[i].devargs_count);
-			fail = -1;
+			fail = TEST_FAILED;
 			break;
 		}
 	}
@@ -278,7 +278,7 @@ test_invalid_devargs_parsing(void)
 	struct rte_eth_devargs eth_da[RTE_MAX_ETHPORTS];
 	uint32_t i;
 	int ret;
-	int fail = 0;
+	int fail = TEST_SUCCESS;
 
 	for (i = 0; i < RTE_DIM(list); i++) {
 		memset(eth_da, 0, RTE_MAX_ETHPORTS * sizeof(*eth_da));
@@ -286,29 +286,30 @@ test_invalid_devargs_parsing(void)
 		if (ret > 0) {
 			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
 			       list[i], ret);
-			fail = ret;
+			fail = TEST_FAILED;
 			break;
 		}
 	}
 	return fail;
 }
 
+static struct unit_test_suite devargs_test_suite = {
+	.suite_name = "Devargs Unit Test Suite",
+	.setup = NULL,
+	.teardown = NULL,
+	.unit_test_cases = {
+		TEST_CASE(test_valid_devargs),
+		TEST_CASE(test_invalid_devargs),
+		TEST_CASE(test_valid_devargs_parsing),
+		TEST_CASE(test_invalid_devargs_parsing),
+		TEST_CASES_END() /**< NULL terminate unit test array */
+	}
+};
+
 static int
 test_devargs(void)
 {
-	printf("== test valid case ==\n");
-	if (test_valid_devargs() < 0)
-		return -1;
-	printf("== test invalid case ==\n");
-	if (test_invalid_devargs() < 0)
-		return -1;
-	printf("== test devargs parsing valid case ==\n");
-	if (test_valid_devargs_parsing() < 0)
-		return -1;
-	printf("== test devargs parsing invalid case ==\n");
-	if (test_invalid_devargs_parsing() < 0)
-		return -1;
-	return 0;
+	return unit_test_suite_runner(&devargs_test_suite);
 }
 
 REGISTER_FAST_TEST(devargs_autotest, NOHUGE_OK, ASAN_OK, test_devargs);
-- 
2.53.0

[PATCH 4/4] test/devargs: add tests for missing device arguments

[Stephen Hemminger]

There was no coverage for a device string with no driver arguments,
which is how Bugzilla 2049 went unnoticed: global device syntax with
no driver layer left args NULL and drivers crashed passing it to
rte_kvargs_parse().

Add a devargs case asserting args is non-NULL and empty for global
syntax without a driver layer, and that it parses as kvargs the way a
driver would use it.  Add a kvargs case for rte_kvargs_parse(NULL),
which must give an empty list.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_devargs.c | 60 +++++++++++++++++++++++++++++++++++++++++
 app/test/test_kvargs.c  | 16 +++++++++++
 2 files changed, 76 insertions(+)

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index de0dd2364d..693bf362d4 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -193,6 +193,65 @@ test_invalid_devargs(void)
 	return fail;
 }
 
+/*
+ * Global device syntax without a driver layer must still leave args set,
+ * as the legacy syntax does.  Drivers pass it straight to
+ * rte_kvargs_parse(), which used to crash on NULL.
+ */
+static int
+test_devargs_no_driver_layer(void)
+{
+	static const char * const list[] = {
+		"bus=vdev,name=net_null0",
+		"bus=vdev,name=net_null0/class=eth",
+		"class=eth",
+	};
+	struct rte_kvargs *kvlist;
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = TEST_SUCCESS;
+
+	if (rte_bus_find_by_name("vdev") == NULL ||
+	    rte_class_find_by_name("eth") == NULL) {
+		printf("vdev bus or eth class missing, skipping\n");
+		return TEST_SKIPPED;
+	}
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(&da, 0, sizeof(da));
+		ret = rte_devargs_parse(&da, list[i]);
+		if (ret < 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = TEST_FAILED;
+			goto cleanup;
+		}
+		if (da.args == NULL) {
+			printf("rte_devargs_parse(%s) left args NULL\n", list[i]);
+			fail = TEST_FAILED;
+			goto cleanup;
+		}
+		if (da.args[0] != '\0') {
+			printf("rte_devargs_parse(%s) args (%s) not empty\n",
+			       list[i], da.args);
+			fail = TEST_FAILED;
+			goto cleanup;
+		}
+		/* What a driver does with it. */
+		kvlist = rte_kvargs_parse(da.args, NULL);
+		if (kvlist == NULL) {
+			printf("rte_kvargs_parse(%s args) failed\n", list[i]);
+			fail = TEST_FAILED;
+			goto cleanup;
+		}
+		rte_kvargs_free(kvlist);
+cleanup:
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
 struct devargs_parse_case {
 	const char *devargs;
 	uint8_t devargs_count;
@@ -300,6 +359,7 @@ static struct unit_test_suite devargs_test_suite = {
 	.unit_test_cases = {
 		TEST_CASE(test_valid_devargs),
 		TEST_CASE(test_invalid_devargs),
+		TEST_CASE(test_devargs_no_driver_layer),
 		TEST_CASE(test_valid_devargs_parsing),
 		TEST_CASE(test_invalid_devargs_parsing),
 		TEST_CASES_END() /**< NULL terminate unit test array */
diff --git a/app/test/test_kvargs.c b/app/test/test_kvargs.c
index a14b75948a..5dd74bec49 100644
--- a/app/test/test_kvargs.c
+++ b/app/test/test_kvargs.c
@@ -328,6 +328,21 @@ static int test_invalid_kvargs(void)
 	return -1;
 }
 
+/* NULL means no arguments, and must give an empty list rather than crash. */
+static int
+test_parse_null_args(void)
+{
+	struct rte_kvargs *kvlist;
+
+	kvlist = rte_kvargs_parse(NULL, NULL);
+	TEST_ASSERT_NOT_NULL(kvlist, "rte_kvargs_parse(NULL) returned NULL");
+	TEST_ASSERT_EQUAL(kvlist->count, 0, "rte_kvargs_parse(NULL) count %u, not 0",
+			  kvlist->count);
+	rte_kvargs_free(kvlist);
+
+	return 0;
+}
+
 static struct unit_test_suite kvargs_test_suite  = {
 	.suite_name = "Kvargs Unit Test Suite",
 	.setup = NULL,
@@ -353,6 +368,7 @@ static struct unit_test_suite kvargs_test_suite  = {
 		TEST_CASE(test_parse_list_value),
 		TEST_CASE(test_parse_empty_elements),
 		TEST_CASE(test_parse_with_only_key),
+		TEST_CASE(test_parse_null_args),
 		TEST_CASE(test_invalid_kvargs),
 		TEST_CASES_END() /**< NULL terminate unit test array */
 	}
-- 
2.53.0