[PATCH] test/argparse: fix out of bound memcpy

[PATCH] test/argparse: fix out of bound memcpy

[Stephen Hemminger]

The rte_argparse API use variable length arrays for the args.
But the test was only putting space on stack for the argparse
part, not the args. This can lead to out of bounds writes.

The bug only gets detected if DPDK is compiled with LTO.
In function ‘test_argparse_copy’,
    inlined from ‘test_argparse_init_obj’ at ../app/test/test_argparse.c:108:2,
    inlined from ‘test_argparse_opt_callback_parse_int_of_no_val’ at ../app/test/test_argparse.c:490:8:
../app/test/test_argparse.c:96:17: warning: ‘memcpy’ writing 56 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
   96 |                 memcpy(&dst->args[i], &src->args[i], sizeof(src->args[i]));

Fixes: 6c5c6571601c ("argparse: verify argument config")
Cc: fengchengwen@huawei.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_argparse.c | 56 ++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 34 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index 0a229752fa..f4b33e2726 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -70,43 +70,31 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
 	return 0;
 }
 
-/* valid templater, must contain at least two args. */
-#define argparse_templater() { \
-	.prog_name = "test_argparse", \
-	.usage = "-a xx -b yy", \
-	.descriptor = NULL, \
-	.epilog = NULL, \
-	.exit_on_error = false, \
-	.callback = test_argparse_callback, \
-	.args = { \
-		{ "--abc", "-a", "abc argument", (void *)1, (void *)1, \
-			RTE_ARGPARSE_VALUE_NONE, RTE_ARGPARSE_VALUE_TYPE_NONE }, \
-		{ "--xyz", "-x", "xyz argument", (void *)1, (void *)2, \
-			RTE_ARGPARSE_VALUE_NONE, RTE_ARGPARSE_VALUE_TYPE_NONE }, \
-		ARGPARSE_ARG_END(), \
-	}, \
-}
-
-static void
-test_argparse_copy(struct rte_argparse *dst, struct rte_argparse *src)
-{
-	uint32_t i;
-	memcpy(dst, src, sizeof(*src));
-	for (i = 0; /* NULL */; i++) {
-		memcpy(&dst->args[i], &src->args[i], sizeof(src->args[i]));
-		if (src->args[i].name_long == NULL)
-			break;
-	}
-}
-
 static struct rte_argparse *
 test_argparse_init_obj(void)
 {
-	static struct rte_argparse backup = argparse_templater();
-	static struct rte_argparse obj = argparse_templater();
-	/* Because obj may be overwritten, do a deep copy. */
-	test_argparse_copy(&obj, &backup);
-	return &obj;
+	static struct {
+		struct rte_argparse cmd;
+		struct rte_argparse_arg args[3];
+	} obj;
+
+	obj.cmd = (struct rte_argparse) {
+		.prog_name = "test_argparse",
+		.usage = "-a xx -b yy",
+		.exit_on_error = false,
+		.callback = test_argparse_callback,
+	};
+	obj.args[0] = (struct rte_argparse_arg)
+		{ "--abc", "-a", "abc argument", (void *)1, (void *)1,
+			RTE_ARGPARSE_VALUE_NONE, RTE_ARGPARSE_VALUE_TYPE_NONE
+		};
+	obj.args[1] = (struct rte_argparse_arg)
+		{ "--xyz", "-x", "xyz argument", (void *)1, (void *)2,
+			RTE_ARGPARSE_VALUE_NONE, RTE_ARGPARSE_VALUE_TYPE_NONE
+		};
+	obj.args[2] = (struct rte_argparse_arg) ARGPARSE_ARG_END();
+
+	return &obj.cmd;
 }
 
 static int
-- 
2.47.2

Re: [PATCH] test/argparse: fix out of bound memcpy

[Bruce Richardson]

On Fri, Jun 27, 2025 at 09:22:35AM -0700, Stephen Hemminger wrote:
> The rte_argparse API use variable length arrays for the args.
> But the test was only putting space on stack for the argparse
> part, not the args. This can lead to out of bounds writes.
> 
> The bug only gets detected if DPDK is compiled with LTO.
> In function ‘test_argparse_copy’,
>     inlined from ‘test_argparse_init_obj’ at ../app/test/test_argparse.c:108:2,
>     inlined from ‘test_argparse_opt_callback_parse_int_of_no_val’ at ../app/test/test_argparse.c:490:8:
> ../app/test/test_argparse.c:96:17: warning: ‘memcpy’ writing 56 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
>    96 |                 memcpy(&dst->args[i], &src->args[i], sizeof(src->args[i]));
> 
> Fixes: 6c5c6571601c ("argparse: verify argument config")
> Cc: fengchengwen@huawei.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

It looks to me like this is a false positive. If it's not, then the whole
method of declaring argparse arguments is broken, and the library is not
really usable.

See below for what I see in gdb for a regular (non-LTO) debug build. Looks
to me like the compiler is doing the right thing.

/Bruce

>  app/test/test_argparse.c | 56 ++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 34 deletions(-)
> 
> diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
> index 0a229752fa..f4b33e2726 100644
> --- a/app/test/test_argparse.c
> +++ b/app/test/test_argparse.c
> @@ -70,43 +70,31 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
>  	return 0;
>  }
>  
> -/* valid templater, must contain at least two args. */
> -#define argparse_templater() { \
> -	.prog_name = "test_argparse", \
> -	.usage = "-a xx -b yy", \
> -	.descriptor = NULL, \
> -	.epilog = NULL, \
> -	.exit_on_error = false, \
> -	.callback = test_argparse_callback, \
> -	.args = { \
> -		{ "--abc", "-a", "abc argument", (void *)1, (void *)1, \
> -			RTE_ARGPARSE_VALUE_NONE, RTE_ARGPARSE_VALUE_TYPE_NONE }, \
> -		{ "--xyz", "-x", "xyz argument", (void *)1, (void *)2, \
> -			RTE_ARGPARSE_VALUE_NONE, RTE_ARGPARSE_VALUE_TYPE_NONE }, \
> -		ARGPARSE_ARG_END(), \
> -	}, \
> -}
> -
> -static void
> -test_argparse_copy(struct rte_argparse *dst, struct rte_argparse *src)
> -{
> -	uint32_t i;
> -	memcpy(dst, src, sizeof(*src));
> -	for (i = 0; /* NULL */; i++) {
> -		memcpy(&dst->args[i], &src->args[i], sizeof(src->args[i]));
> -		if (src->args[i].name_long == NULL)
> -			break;
> -	}
> -}
> -
>  static struct rte_argparse *
>  test_argparse_init_obj(void)
>  {
> -	static struct rte_argparse backup = argparse_templater();
> -	static struct rte_argparse obj = argparse_templater();
> -	/* Because obj may be overwritten, do a deep copy. */

Running gdb and querying the layout of items in this function I get:

Thread 1 "dpdk-test" hit Breakpoint 1, test_argparse_init_obj () at ../app/test/test_argparse.c:108
108		test_argparse_copy(&obj, &backup);
(gdb) print &backup
$1 = (struct rte_argparse *) 0x555556d2b8a0 <backup>
(gdb) print &obj
$2 = (struct rte_argparse *) 0x555556d2b740 <obj>
(gdb) print 0xb8a0-0xb740
$8 = 352
(gdb) print sizeof(backup)
$9 = 184
(gdb) print sizeof(backup->args[0])
$10 = 56
(gdb) print sizeof(backup->args[0])*3 + sizeof(backup)
$11 = 352
(gdb) 

So we have the space available and allocated for the full structure plus
the 3 args. This means that the memcpy is not going to overflow.

Now, the separate question arises as to whether there are better methods to
initialize things in this test. That's a different issue, and I suspect
that we don't need the memcpy at all, but for me the key thing is that the
syntax used in the templater macro is good for defining argparse arguments.


> -	test_argparse_copy(&obj, &backup);
> -	return &obj;
> +	static struct {
> +		struct rte_argparse cmd;
> +		struct rte_argparse_arg args[3];
> +	} obj;
> +
> +	obj.cmd = (struct rte_argparse) {
> +		.prog_name = "test_argparse",
> +		.usage = "-a xx -b yy",
> +		.exit_on_error = false,
> +		.callback = test_argparse_callback,
> +	};
> +	obj.args[0] = (struct rte_argparse_arg)
> +		{ "--abc", "-a", "abc argument", (void *)1, (void *)1,
> +			RTE_ARGPARSE_VALUE_NONE, RTE_ARGPARSE_VALUE_TYPE_NONE
> +		};
> +	obj.args[1] = (struct rte_argparse_arg)
> +		{ "--xyz", "-x", "xyz argument", (void *)1, (void *)2,
> +			RTE_ARGPARSE_VALUE_NONE, RTE_ARGPARSE_VALUE_TYPE_NONE
> +		};
> +	obj.args[2] = (struct rte_argparse_arg) ARGPARSE_ARG_END();
> +
> +	return &obj.cmd;
>  }
>  
>  static int
> -- 
> 2.47.2
>