dgibson / dtc

Device Tree Compiler
218 stars 130 forks source link

libfdt: Replace deprecated 0-length arrays with proper flexible arrays #76

Closed kees closed 1 year ago

kees commented 1 year ago

Replace the 0-length arrays in structures with proper flexible arrays. This will avoid warnings when building under GCC 13 with -fstrict-flex-arrays, which the Linux kernel will be doing soon:

In file included from ../lib/fdt_ro.c:2: ../lib/../scripts/dtc/libfdt/fdt_ro.c: In function 'fdt_get_name': ../lib/../scripts/dtc/libfdt/fdt_ro.c:319:24: warning: 'strrchr' reading 1 or more bytes from a region of size 0 [-Wstringop-overread] 319 | leaf = strrchr(nameptr, '/'); | ^~~~~

Signed-off-by: Kees Cook keescook@chromium.org

kees commented 1 year ago

It seems like the freebsd tests have been failing for a while, looking at the recent commits.

dgibson commented 1 year ago

Replace the 0-length arrays in structures with proper flexible arrays. This will avoid warnings when building under GCC 13 with -fstrict-flex-arrays, which the Linux kernel will be doing soon:

Hrm, so, using standard constructs is good. It's particularly good for libfdt.h which is supposed to cover the format itself, rather than be bound strictly to libfdt.

I had avoided this, because I was under the impression that flexible array elements counted as length 1, rather than 0 for the purposes of sizeof which will break a number of places in libfdt. However, I've just had another look, and it seems like I was mistaken.

If you can double check that with a definitive source, that would be most helpful.

dgibson commented 1 year ago

Thanks. Applied.

robherring commented 1 year ago

This change is breaking the build of pylibfdt for me: /home/rob/proj/dtc/./pylibfdt/libfdt_wrap.c: In function ‘_wrap_fdt_node_header_name_set’: /home/rob/proj/dtc/./pylibfdt/libfdt_wrap.c:4350:18: error: cast specifies array type 4350 | arg1->name = (char [])(char )memcpy(malloc((size)sizeof(char)), (const char )(arg2), sizeof(char)(size)); | ^ /home/rob/proj/dtc/./pylibfdt/libfdt_wrap.c:4350:16: error: invalid use of flexible array member 4350 | arg1->name = (char [])(char )memcpy(malloc((size)sizeof(char)), (const char )(arg2), sizeof(char)(size)); | ^ /home/rob/proj/dtc/./pylibfdt/libfdt_wrap.c:4352:16: error: invalid use of flexible array member 4352 | arg1->name = 0; | ^ /home/rob/proj/dtc/./pylibfdt/libfdt_wrap.c: In function ‘_wrap_fdt_property_data_set’: /home/rob/proj/dtc/./pylibfdt/libfdt_wrap.c:4613:18: error: cast specifies array type 4613 | arg1->data = (char [])(char )memcpy(malloc((size)sizeof(char)), (const char )(arg2), sizeof(char)(size)); | ^ /home/rob/proj/dtc/./pylibfdt/libfdt_wrap.c:4613:16: error: invalid use of flexible array member 4613 | arg1->data = (char [])(char )memcpy(malloc((size)sizeof(char)), (const char )(arg2), sizeof(char)(size)); | ^ /home/rob/proj/dtc/./pylibfdt/libfdt_wrap.c:4615:16: error: invalid use of flexible array member 4615 | arg1->data = 0; | ^

I'm using gcc 12.2.0.

kees commented 1 year ago

Uhm, is that some kind of SWIG binding? We've seen issues with non-C scrapers falling over in the face of flex arrays before: https://www.spinics.net/lists/fedora-devel/msg297996.html

robherring commented 1 year ago

Yes, it's SWIG. Paging @sjg20 ...

Looks like the distro fixes are either reverting the change or avoiding the struct for SWIG. Will see if we can do the latter.

sjg20 commented 1 year ago

Yes it breaks for me too...will take a quick look

sjg20 commented 1 year ago

https://stackoverflow.com/questions/29781102/swig-handling-tlvzero-length-array

Hmm I think reverting is the best..

Kees are you able to figure out the swig stuff and resubmit?

sjg20 commented 1 year ago

(and how did this pass for you?)

robherring commented 1 year ago

I have a fix: diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i index f9f7e7e66d13..efbdee8c22d8 100644 --- a/pylibfdt/libfdt.i +++ b/pylibfdt/libfdt.i @@ -1036,6 +1036,9 @@ class NodeAdder():

%rename(fdt_property) fdt_property_func;

+%ignore fdt_property::data; +%ignore fdt_node_header::name; + /*

sjg20 commented 1 year ago

Seems right. We don't need to set these through the swig interface anyway

robherring commented 1 year ago

Patch on the list.

Note that it passed CI tests, so I guess Python is skipped for those.

sjg20 commented 1 year ago

Funny that that one went to spam

dgibson commented 1 year ago

@robherring @sjg20 the reason I didn't detect this breakage is that the build of pylibfdt has been broken for me for months. It's cleearly not broken for everyone, so it must be something specific to my build environment (Fedora 37), but I don't know how to fix it.

In any case I've now applied the fix.