[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
> 

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

[Stephen Hemminger]

On Fri, 27 Jun 2025 19:56:57 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> 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

The problem is that the when structure is initialized its size gets boosted.
	https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Flexible-Array-Fields.html
	GNU C allows static initialization of flexible array fields. 
	The effect is to “make the array long enough” for the initializer.

	struct f1 { int x; int y[]; } f1
		  = { 1, { 2, 3, 4 } };

It looks like a compiler bug that the extra size info doesn't get propogated
into the copy code. 

[PATCH v2] test/argparse: change initialization to workaround LTO

[Stephen Hemminger]

When compiled with Link Time Optimization, the existing code
generated an error, because the compiler was unable to intuit
that there was space in the flexible array.

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]));

Initialiizing a structure with flexible array is special case
and compiler expands the structure to fit. But inside the copy
function it no longer knew that.

The workaround is to put the copy inside the same function
and use structure assignment. Also macro should be uppper case.

Fixes: 6c5c6571601c ("argparse: verify argument config")
Cc: fengchengwen@huawei.com

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - simpler fix is to just inline the copy

 app/test/test_argparse.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
index 0a229752fa..d5b777e321 100644
--- a/app/test/test_argparse.c
+++ b/app/test/test_argparse.c
@@ -71,7 +71,7 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
 }
 
 /* valid templater, must contain at least two args. */
-#define argparse_templater() { \
+#define ARGPARSE_TEMPLATE { \
 	.prog_name = "test_argparse", \
 	.usage = "-a xx -b yy", \
 	.descriptor = NULL, \
@@ -87,25 +87,24 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
 	}, \
 }
 
-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);
+	/* Note: initialization of structure with flexible arrary
+	 * increases the size of the variable to match.
+	 */
+	static const struct rte_argparse backup = ARGPARSE_TEMPLATE;
+	static struct rte_argparse obj = ARGPARSE_TEMPLATE;
+	unsigned int i;
+
+	obj = backup;
+	for (i = 0; ; i++) {
+		obj.args[i] = backup.args[i];
+		if (backup.args[i].name_long == NULL)
+			break;
+	}
+
 	return &obj;
 }
 
-- 
2.47.2

Re: [PATCH v2] test/argparse: change initialization to workaround LTO

[Bruce Richardson]

On Mon, Jun 30, 2025 at 07:58:49AM -0700, Stephen Hemminger wrote:
> When compiled with Link Time Optimization, the existing code
> generated an error, because the compiler was unable to intuit
> that there was space in the flexible array.
> 
> 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]));
> 
> Initialiizing a structure with flexible array is special case
> and compiler expands the structure to fit. But inside the copy
> function it no longer knew that.
> 
> The workaround is to put the copy inside the same function
> and use structure assignment. Also macro should be uppper case.
> 
> Fixes: 6c5c6571601c ("argparse: verify argument config")
> Cc: fengchengwen@huawei.com
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - simpler fix is to just inline the copy
> 
>  app/test/test_argparse.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 

LGTM. One suggestion inline, in case you feel like adjusting things
further.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
> index 0a229752fa..d5b777e321 100644
> --- a/app/test/test_argparse.c
> +++ b/app/test/test_argparse.c
> @@ -71,7 +71,7 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
>  }
>  
>  /* valid templater, must contain at least two args. */
> -#define argparse_templater() { \
> +#define ARGPARSE_TEMPLATE { \
>  	.prog_name = "test_argparse", \
>  	.usage = "-a xx -b yy", \
>  	.descriptor = NULL, \
> @@ -87,25 +87,24 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
>  	}, \
>  }
>  
> -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);
> +	/* Note: initialization of structure with flexible arrary
> +	 * increases the size of the variable to match.
> +	 */
> +	static const struct rte_argparse backup = ARGPARSE_TEMPLATE;
> +	static struct rte_argparse obj = ARGPARSE_TEMPLATE;
> +	unsigned int i;
> +
> +	obj = backup;
> +	for (i = 0; ; i++) {
> +		obj.args[i] = backup.args[i];
> +		if (backup.args[i].name_long == NULL)
> +			break;
> +	}

We should consider either making this a "do { } while" loop or adding the
termination condition to the "for" loop statement as normal. For example:

	unsigned int i = 0;

	obj = backup;
	do {
		obj.args[i] = backup.args[i];
	} while (backup.args[++i].name_long != NULL);

or else:

	obj = backup;
	for (i = 0; backup.args[i].name_long != NULL; i++)
		obj.args[i] = backup.args[i];
	obj.args[i] = ARGPARSE_ARG_END();

I'd tend toward the second, myself, but what is in your patch above is fine
as-is too.

> +
>  	return &obj;
>  }
>  
> -- 
> 2.47.2
> 

Re: [PATCH v2] test/argparse: change initialization to workaround LTO

[Stephen Hemminger]

On Mon, 30 Jun 2025 16:20:21 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> We should consider either making this a "do { } while" loop or adding the
> termination condition to the "for" loop statement as normal. For example:
> 
> 	unsigned int i = 0;
> 
> 	obj = backup;
> 	do {
> 		obj.args[i] = backup.args[i];
> 	} while (backup.args[++i].name_long != NULL);
> 
> or else:
> 
> 	obj = backup;
> 	for (i = 0; backup.args[i].name_long != NULL; i++)
> 		obj.args[i] = backup.args[i];
> 	obj.args[i] = ARGPARSE_ARG_END();
> 
> I'd tend toward the second, myself, but what is in your patch above is fine
> as-is too.

Agree with one of the two is more readable.
I kind of wanted to keep what original code was doing.

Re: [PATCH v2] test/argparse: change initialization to workaround LTO

[Stephen Hemminger]

On Mon, 30 Jun 2025 16:20:21 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Mon, Jun 30, 2025 at 07:58:49AM -0700, Stephen Hemminger wrote:
> > When compiled with Link Time Optimization, the existing code
> > generated an error, because the compiler was unable to intuit
> > that there was space in the flexible array.
> > 
> > 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]));
> > 
> > Initialiizing a structure with flexible array is special case
> > and compiler expands the structure to fit. But inside the copy
> > function it no longer knew that.
> > 
> > The workaround is to put the copy inside the same function
> > and use structure assignment. Also macro should be uppper case.
> > 
> > Fixes: 6c5c6571601c ("argparse: verify argument config")
> > Cc: fengchengwen@huawei.com
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > v2 - simpler fix is to just inline the copy
> > 
> >  app/test/test_argparse.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> >   
> 
> LGTM. One suggestion inline, in case you feel like adjusting things
> further.
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> > diff --git a/app/test/test_argparse.c b/app/test/test_argparse.c
> > index 0a229752fa..d5b777e321 100644
> > --- a/app/test/test_argparse.c
> > +++ b/app/test/test_argparse.c
> > @@ -71,7 +71,7 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
> >  }
> >  
> >  /* valid templater, must contain at least two args. */
> > -#define argparse_templater() { \
> > +#define ARGPARSE_TEMPLATE { \
> >  	.prog_name = "test_argparse", \
> >  	.usage = "-a xx -b yy", \
> >  	.descriptor = NULL, \
> > @@ -87,25 +87,24 @@ test_argparse_callback(uint32_t index, const char *value, void *opaque)
> >  	}, \
> >  }
> >  
> > -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);
> > +	/* Note: initialization of structure with flexible arrary
> > +	 * increases the size of the variable to match.
> > +	 */
> > +	static const struct rte_argparse backup = ARGPARSE_TEMPLATE;
> > +	static struct rte_argparse obj = ARGPARSE_TEMPLATE;
> > +	unsigned int i;
> > +
> > +	obj = backup;
> > +	for (i = 0; ; i++) {
> > +		obj.args[i] = backup.args[i];
> > +		if (backup.args[i].name_long == NULL)
> > +			break;
> > +	}  
> 
> We should consider either making this a "do { } while" loop or adding the
> termination condition to the "for" loop statement as normal. For example:
> 
> 	unsigned int i = 0;
> 
> 	obj = backup;
> 	do {
> 		obj.args[i] = backup.args[i];
> 	} while (backup.args[++i].name_long != NULL);
> 
> or else:
> 
> 	obj = backup;
> 	for (i = 0; backup.args[i].name_long != NULL; i++)
> 		obj.args[i] = backup.args[i];
> 	obj.args[i] = ARGPARSE_ARG_END();
> 
> I'd tend toward the second, myself, but what is in your patch above is fine
> as-is too.
> 
> > +
> >  	return &obj;
> >  }
> >  
> > -- 
> > 2.47.2
> >   

The long term goal here is to build with LTO during review.
Best if there are no outstanding warnings in that case.
LTO has found some pre-existing bugs because it has wider visibility across file boundaries.