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

[Bruce Richardson <bruce.richardson@intel.com>]

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
>