Re: [PATCH] bring -Warray-bounds closer to -Wstringop-overflow (PR91647, 91463, 91679)

[Jeff Law <law@redhat.com>]

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