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: fengcheng...@huawei.com
> Signed-off-by: Stephen Hemminger <step...@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
>
