[PATCH] bring -Warray-bounds closer to -Wstringop-overflow (PR91647, 91463, 91679)
Jeff Law
law@redhat.com
Thu Oct 31 19:30:00 GMT 2019
On 10/11/19 10:34 AM, Martin Sebor wrote:
> I've fixed the bug in the attached patch. The rest can be suppressed
> by replacing the zero-length arrays with flexible array members but
> that's just trading one misuse for another. If the code can't be
> changed to avoid this (likely not an option since the arrays are in
> visible in the public API) I think the best way to deal with them is
> to suppress them by #pragma GCC diagnostic ignored. I opened BZ 25097
> in Glibc Bugzilla to track this.
While it's trading one misuse for another, one could argue that a
flexible array is less of a misuse :-)
>
> The patch triggers 94 instances of -Warray-bounds (60 of which
> are for distinct code) in 21 .c files. I haven't looked at all
> of them but some of the patterns I noticed are:
>
> 1) Intentionally using an interior zero-length array to access
> (e.g., memset) one or more subsequent members. E.g.,
> _dbgp_external_startup in drivers/usb/early/ehci-dbgp.c and
> quite a few others. This is pretty pervasive but seems easily
> avoidable.
Yea, I saw something like this as well. I can't recall if it was in the
kernel or a package that was reading structured data of some sort.
THey'd use the zero length array to access the entire blob-o-data and
subsequent fields when they wanted to access particular chunks of data
that were at well known offsets from the start.
>
> 2) Overwriting a member array with more data (e.g., function
> cxio_rdev_open in
> drivers/infiniband/hw/cxgb3/cxio_hal.c or in function
> pk_probe in drivers/hid/hid-prodikeys.c). At first glance
> some of these look like bugs but with stuff obscured by macros
> and no comments it's hard to tell.
ACK.
>
> 3) Uses of the container_of() macro to access one member given
> the address of another. This is undefined (and again breaks
> the aliasing rules) but the macro is used all over the place
> in the kernel. I count over 15,000 references to it.
Ugh.
>
> 4) Uses of one-element arrays as members of other one-element
> arrays (in include/scsi/fc/fc_ms.h). Was this ever meant
> to be supported by GCC? (It isn't by _FORTIFY_SOURCE=2.)
>
> 5) Possible false positives due to the recent loop unrolling
> change.
I think I've worked around some #5 issues as well. It's got an
aassociated BZ and hopefully will be addressed during stage3/stage4.
>
> It will be a quite a bit of work to clean this up. To make it
> easier we would introduce a new option to control the warning
> for some of the most common idioms, such as
> -Wzero-length-array-bounds. I'm not too wild about this because
> it would just paper over the problem. A better solution would
> also involve avoiding the aliasing assumptions for overlapping
> zero-length member arrays.
>
> Anyway, attached is the updated patch with just the one fix
> I mentioned above, retested on x86_64-linux.
I think the agreement in our meeting yesterday was to give some kind of
knob, particularly for the kernel folks.
>
> Martin
>
> gcc-91647.diff
>
> PR middle-end/91679 - missing -Warray-bounds accessing a member array in a local buffer
> PR middle-end/91647 - new FAILs for Warray-bounds-8 and Wstringop-overflow-3.C
> PR middle-end/91463 - missing -Warray-bounds accessing past the end of a statically initialized flexible array member
>
> gcc/ChangeLog:
>
> PR middle-end/91679
> PR middle-end/91647
> PR middle-end/91463
> * tree-vrp.c (vrp_prop::check_array_ref): Handle trailing arrays with
> initializers.
> (vrp_prop::check_mem_ref): Handle declared struct objects.
> * tree.c (last_field): New function.
> (array_at_struct_end_p): Handle MEM_REF.
> (get_initializer_for): New helper.
> (component_ref_size): Rename locals. Call get_initializer_for instead
> of fold_ctor_reference. Correct handling of flexible array members.
> * wide-int.h (generic_wide_int <storage>::sign_mask): Assert invariant.
>
> gcc/testsuite/ChangeLog:
>
> PR middle-end/91679
> PR middle-end/91647
> PR middle-end/91463
> * c-c++-common/Warray-bounds-2.c: Disable VRP. Adjust expected messages.
> * gcc.dg/Warray-bounds-48.c: New test.
> * gcc.dg/Warray-bounds-49.c: New test.
> * gcc.dg/Wstringop-overflow-16.c: Adjust text of expected messages.
> * gcc.dg/pr36902.c: Remove xfail.
> * gcc.dg/strlenopt-57.c: Add an expected warning.
So per our meeting yesterday, add a knob for the kernel folks to be able
to control this and it's OK.
jeff
More information about the Gcc-patches
mailing list