* [PATCH] test/argparse: fix out of bound memcpy
@ 2025-06-27 16:22 Stephen Hemminger
2025-06-27 18:56 ` Bruce Richardson
0 siblings, 1 reply; 2+ messages in thread
From: Stephen Hemminger @ 2025-06-27 16:22 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, fengchengwen
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
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH] test/argparse: fix out of bound memcpy
2025-06-27 16:22 [PATCH] test/argparse: fix out of bound memcpy Stephen Hemminger
@ 2025-06-27 18:56 ` Bruce Richardson
0 siblings, 0 replies; 2+ messages in thread
From: Bruce Richardson @ 2025-06-27 18:56 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, fengchengwen
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-06-27 18:57 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-27 16:22 [PATCH] test/argparse: fix out of bound memcpy Stephen Hemminger
2025-06-27 18:56 ` Bruce Richardson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).