[iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[Phil Sutter]

Gcc-10 doesn't like the use of zero-length arrays as last struct member
to denote variable sized objects. The suggested alternative, namely to
use a flexible array member as defined by C99, is problematic as that
doesn't allow for said struct to be embedded into others. With the
relevant structs being part of kernel UAPI, this can't be precluded
though.

The call to memcpy() which triggers the warning copies data from one
struct xt_counters to another. Since this struct is flat and merely
contains two u64 fields, One can use direct assignment instead which
avoids the warning.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 libiptc/libiptc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index 5888201525213..ceeb017b39400 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -1169,7 +1169,7 @@ static int iptcc_compile_chain(struct xtc_handle *h, STRUCT_REPLACE *repl, struc
 	else
 		foot->target.verdict = RETURN;
 	/* set policy-counters */
-	memcpy(&foot->e.counters, &c->counters, sizeof(STRUCT_COUNTERS));
+	foot->e.counters = c->counters;
 
 	return 0;
 }
-- 
2.28.0

Re: [iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[Jan Engelhardt]

On Thursday 2020-10-08 15:01, Phil Sutter wrote:

>Gcc-10 doesn't like the use of zero-length arrays as last struct member
>to denote variable sized objects.

On the contrary. It does not complain about the declaration of T x[0],
but about the write beyond the end of an array of size 0 (literally).
Such past-end writes are Undefined Behavior by the standard, and tools
like cov-scan (based on llvm I believe) report it just the same.

That's why C99 probably had to come up with "T y[]"; the array is now
of unspecified size rather than size 0. cov-scan agrees.

Since GCC has retained "T x[0]" as an extension, it would be a gcc bug
to warn about its own extension in conjunction with memcpy.


>The suggested alternative, namely to
>use a flexible array member as defined by C99, is problematic as that
>doesn't allow for said struct to be embedded into others.

Both T x[0] and T y[], in gcc, invoke the flexible scheme.

Both share the "embedding problem" - i.e. by using the flexible array,
you can accidentally access members of the surrounding struct.

gcc only complains about this when combining "T y[]" with C++ mode.
I find this to be an unusual choice and would suggest opening a bug.

The reason T x[0], when embedded, in C++, does not elicit a warning
is probably because it is already considered UB to access past-end-of-array
(see above); and I raise the hypothesis that the GNU extension of T x[0] is
actually only specified for C.

Anyway, gcc-10/C mode does not reject embedding (and a number of maintainers
have made it abundantly clear in the past they don't care if UAPI uses anti-C++
"features"). Nonreject example:

» cat x.c
struct a2 {
        int x;
        int z[];
};
struct m {
        struct a2 y;
        long q;
};
» gcc -Wall -std=c11 -c -w -v x.c
GNU C11 (SUSE Linux) version 10.2.1 20200825 [revision c0746a1beb1ba073c7981eb09f55b3d993b32e5c] (x86_64-suse-linux)
»

Re: [iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[Phil Sutter]

Hi Jan,

On Thu, Oct 08, 2020 at 04:33:04PM +0200, Jan Engelhardt wrote:
[...]
> Anyway, gcc-10/C mode does not reject embedding (and a number of maintainers
> have made it abundantly clear in the past they don't care if UAPI uses anti-C++
> "features"). Nonreject example:
> 
> » cat x.c
> struct a2 {
>         int x;
>         int z[];
> };
> struct m {
>         struct a2 y;
>         long q;
> };
> » gcc -Wall -std=c11 -c -w -v x.c
> GNU C11 (SUSE Linux) version 10.2.1 20200825 [revision c0746a1beb1ba073c7981eb09f55b3d993b32e5c] (x86_64-suse-linux)

No need to pass '-w', gcc seems to accept the above unless one passes
'-pedantic' and thereby enables strict ISO C mode:

| % gcc -Wall -pedantic -std=c11 -c x.c   
| x.c:6:19: warning: invalid use of structure with flexible array member [-Wpedantic]
|     6 |         struct a2 y;
|       |                   ^

While the question of whether kernel UAPI headers should adhere to
strict ISO C or not may be debatable, my motivation for working around
the situation in user space comes from Gustavo's complaints when I tried
to convert the relevant struct members into flexible arrays. He
apparently is a burnt child looking at commit 1e6e9d0f4859e ("uapi:
revert flexible-array conversions").

Cheers, Phil

Re: [iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[Jan Engelhardt]

On Thursday 2020-10-08 16:58, Phil Sutter wrote:
>
>While the question of whether kernel UAPI headers should adhere to
>strict ISO C or not may be debatable, my motivation for working around
>the situation in user space comes from Gustavo's complaints when I tried
>to convert the relevant struct members into flexible arrays. He
>apparently is a burnt child looking at commit 1e6e9d0f4859e ("uapi:
>revert flexible-array conversions").

Ugh... RDMA.

iptables does not rely or even do such embedding nonsense. When we
have a flexible array member T x[0] or T x[] somewhere, we really do
mean that Ts follow, not some Us like in the RDMA case.

It's probably fair to restore [] for our headers.

Re: [iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[Phil Sutter]

On Thu, Oct 08, 2020 at 05:31:03PM +0200, Jan Engelhardt wrote:
> 
> On Thursday 2020-10-08 16:58, Phil Sutter wrote:
> >
> >While the question of whether kernel UAPI headers should adhere to
> >strict ISO C or not may be debatable, my motivation for working around
> >the situation in user space comes from Gustavo's complaints when I tried
> >to convert the relevant struct members into flexible arrays. He
> >apparently is a burnt child looking at commit 1e6e9d0f4859e ("uapi:
> >revert flexible-array conversions").
> 
> Ugh... RDMA.
> 
> iptables does not rely or even do such embedding nonsense. When we
> have a flexible array member T x[0] or T x[] somewhere, we really do
> mean that Ts follow, not some Us like in the RDMA case.

In fact, struct ipt_replace has a zero-length array as last field of
type struct ipt_entry which in turn has a zero-length array as last
field. :)

Embedding is allowed as a gcc-extension. So while my initial approach at
getting rid of the warning in iptables compile-output worked, it didn't
make the header ISO C compatible.

> It's probably fair to restore [] for our headers.

Since gcc in pedantic mode neither accepts zero length arrays nor
embedded structs with flexible member arrays, doing so won't break ISO C
compatibility at least. ;)

Cheers, Phil

Re: [iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[Jan Engelhardt]

On Thursday 2020-10-08 18:07, Phil Sutter wrote:
>> iptables does not rely or even do such embedding nonsense. When we
>> have a flexible array member T x[0] or T x[] somewhere, we really do
>> mean that Ts follow, not some Us like in the RDMA case.
>
>In fact, struct ipt_replace has a zero-length array as last field of
>type struct ipt_entry which in turn has a zero-length array as last
>field. :)

In the RDMA thread, I was informed that the trailing members' only
purpose is to serve as something of a shorthand:

Shortcut:
	struct ipt_entry *e = replace->elements;
The long way:
	struct ipt_entry *e = (void *)((char *)replace + sizeof(*replace));

But such gritty detail is often stowed away in some nice accessor
functions or macros. That's what's currently missing in spots
apprently.

	struct ipt_entry *next = get_next_blah(replace);

Then the get_next can do that arithmetic, we won't need
ipt_replace::elements, and could do away with the flexible array
member altogether, especially when it's not used with equal-sized
elements, and ipt_entry is of variadic size.

Re: [iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[Phil Sutter]

On Thu, Oct 08, 2020 at 06:46:40PM +0200, Jan Engelhardt wrote:
> On Thursday 2020-10-08 18:07, Phil Sutter wrote:
> >> iptables does not rely or even do such embedding nonsense. When we
> >> have a flexible array member T x[0] or T x[] somewhere, we really do
> >> mean that Ts follow, not some Us like in the RDMA case.
> >
> >In fact, struct ipt_replace has a zero-length array as last field of
> >type struct ipt_entry which in turn has a zero-length array as last
> >field. :)
> 
> In the RDMA thread, I was informed that the trailing members' only
> purpose is to serve as something of a shorthand:
> 
> Shortcut:
> 	struct ipt_entry *e = replace->elements;
> The long way:
> 	struct ipt_entry *e = (void *)((char *)replace + sizeof(*replace));
> 
> But such gritty detail is often stowed away in some nice accessor
> functions or macros. That's what's currently missing in spots
> apprently.
> 
> 	struct ipt_entry *next = get_next_blah(replace);
> 
> Then the get_next can do that arithmetic, we won't need
> ipt_replace::elements, and could do away with the flexible array
> member altogether, especially when it's not used with equal-sized
> elements, and ipt_entry is of variadic size.

Since this is UAPI though, we can't get rid of the problematic fields,
no?

Cheers, Phil

Re: [iptables PATCH] libiptc: Avoid gcc-10 zero-length array warning

[Jan Engelhardt]

On Friday 2020-10-09 17:16, Phil Sutter wrote:
>> But such gritty detail is often stowed away in some nice accessor
>> functions or macros. That's what's currently missing in spots
>> apprently.
>> 
>> 	struct ipt_entry *next = get_next_blah(replace);
>> 
>> Then the get_next can do that arithmetic, we won't need
>> ipt_replace::elements, and could do away with the flexible array
>> member altogether, especially when it's not used with equal-sized
>> elements, and ipt_entry is of variadic size.
>
>Since this is UAPI though, we can't get rid of the problematic fields,
>no?

The kernel proclaims a stable ABI.
About the C API, I am not certain, but I presume there are no restriction --
old netfilter headers have been removed in the past (and userspace was to make
a copy if it wanted to build the byte streams required by the ABI
by way of a few "struct"s rather than pushing individual uint32_t fields into a
buffer).
A zero-size member does not impact the ABI at least.