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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Martin Sebor-2
Recent enhancements to -Wstringop-overflow improved the warning
to the point that it detects a superset of the problems -Warray-
bounds is intended detect in character accesses.  Because both
warnings detect overlapping sets of problems, and because the IL
they work with tends to change in subtle ways from target to
targer, tests designed to verify one or the other sometimes fail
with a target where the warning isn't robust enough to detect
the problem given the IL representation.

To reduce these test suite failures the attached patch extends
-Warray-bounds to handle some of the same problems -Wstringop-
overflow does, pecifically, out-of-bounds accesses to array
members of structs, including zero-length arrays and flexible
array members of defined objects.

In the process of testing the enhancement I realized that
the recently added component_size() function doesn't work as
intended for non-character array members (see below).  The patch
corrects this by reverting back to the original implementation
of the function until the better/simpler solution can be put in
place as mentioned below.

Tested on x86_64-linux.

Martin


[*] component_size() happens to work for char arrays because those
are transformed to STRING_CSTs, but not for arrays that are not.
E.g., in

   struct S { int64_t i; int16_t j; int16_t a[]; }
     s = { 0, 0, { 1, 0 } };

unless called with type set to int16_t[2], fold_ctor_reference
will return s.a[0] rather than all of s.a.  But set type to
int16_t[2] we would need to know that s.a's initializer has two
elements, and that's just what we're using fold_ctor_reference
to find out.

I think this could probably be made to work somehow by extending
useless_type_conversion_p to handle this case as special somehow,
but it doesn't seem worth the effort given that there should be
an easier way to do it as you noted below.

Given the above, the long term solution should be to rely on
DECL_SIZE_UNIT(decl) - TYPE_SIZE_UNIT(decl_type) as Richard
suggested in the review of its initial implementation.
Unfortunately, because of bugs in both the C and C++ front ends
(I just opened PR 65403 with the details) the simple formula
doesn't give the right answers either.  So until the bugs are
fixed, the patch reverts back to the original loopy solution.
It's no more costly than the current fold_ctor_reference
approach.

gcc-91647.diff (28K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Martin Sebor-2
Just a heads up that I tested the patch with Glibc and the kernel.
It exposes some of the same "abuses" of (near) zero-length arrays
as the most recent improvement in this area.  In glibc, it
complains about code in fileops.c, iofwide.c, libc-tls.c, and
rtld.c.  The ones I looked at all look like the last one we saw.
I'll look into how to deal with them next.  In the kernel it
issues a variety of warnings that I need to investigate after
I get back from Cauldron.

On 9/6/19 1:27 PM, Martin Sebor wrote:

> Recent enhancements to -Wstringop-overflow improved the warning
> to the point that it detects a superset of the problems -Warray-
> bounds is intended detect in character accesses.  Because both
> warnings detect overlapping sets of problems, and because the IL
> they work with tends to change in subtle ways from target to
> targer, tests designed to verify one or the other sometimes fail
> with a target where the warning isn't robust enough to detect
> the problem given the IL representation.
>
> To reduce these test suite failures the attached patch extends
> -Warray-bounds to handle some of the same problems -Wstringop-
> overflow does, pecifically, out-of-bounds accesses to array
> members of structs, including zero-length arrays and flexible
> array members of defined objects.
>
> In the process of testing the enhancement I realized that
> the recently added component_size() function doesn't work as
> intended for non-character array members (see below).  The patch
> corrects this by reverting back to the original implementation
> of the function until the better/simpler solution can be put in
> place as mentioned below.
>
> Tested on x86_64-linux.
>
> Martin
>
>
> [*] component_size() happens to work for char arrays because those
> are transformed to STRING_CSTs, but not for arrays that are not.
> E.g., in
>
>    struct S { int64_t i; int16_t j; int16_t a[]; }
>      s = { 0, 0, { 1, 0 } };
>
> unless called with type set to int16_t[2], fold_ctor_reference
> will return s.a[0] rather than all of s.a.  But set type to
> int16_t[2] we would need to know that s.a's initializer has two
> elements, and that's just what we're using fold_ctor_reference
> to find out.
>
> I think this could probably be made to work somehow by extending
> useless_type_conversion_p to handle this case as special somehow,
> but it doesn't seem worth the effort given that there should be
> an easier way to do it as you noted below.
>
> Given the above, the long term solution should be to rely on
> DECL_SIZE_UNIT(decl) - TYPE_SIZE_UNIT(decl_type) as Richard
> suggested in the review of its initial implementation.
> Unfortunately, because of bugs in both the C and C++ front ends
> (I just opened PR 65403 with the details) the simple formula
> doesn't give the right answers either.  So until the bugs are
> fixed, the patch reverts back to the original loopy solution.
> It's no more costly than the current fold_ctor_reference
> approach.

Reply | Threaded
Open this post in threaded view
|

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

Jeff Law
On 9/6/19 5:26 PM, Martin Sebor wrote:
> Just a heads up that I tested the patch with Glibc and the kernel.
> It exposes some of the same "abuses" of (near) zero-length arrays
> as the most recent improvement in this area.  In glibc, it
> complains about code in fileops.c, iofwide.c, libc-tls.c, and
> rtld.c.  The ones I looked at all look like the last one we saw.
> I'll look into how to deal with them next.  In the kernel it
> issues a variety of warnings that I need to investigate after
> I get back from Cauldron.
THanks.  Good to know.

jeff
Reply | Threaded
Open this post in threaded view
|

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

Jeff Law
In reply to this post by Martin Sebor-2
On 9/6/19 1:27 PM, Martin Sebor wrote:

> Recent enhancements to -Wstringop-overflow improved the warning
> to the point that it detects a superset of the problems -Warray-
> bounds is intended detect in character accesses.  Because both
> warnings detect overlapping sets of problems, and because the IL
> they work with tends to change in subtle ways from target to
> targer, tests designed to verify one or the other sometimes fail
> with a target where the warning isn't robust enough to detect
> the problem given the IL representation.
>
> To reduce these test suite failures the attached patch extends
> -Warray-bounds to handle some of the same problems -Wstringop-
> overflow does, pecifically, out-of-bounds accesses to array
> members of structs, including zero-length arrays and flexible
> array members of defined objects.
>
> In the process of testing the enhancement I realized that
> the recently added component_size() function doesn't work as
> intended for non-character array members (see below).  The patch
> corrects this by reverting back to the original implementation
> of the function until the better/simpler solution can be put in
> place as mentioned below.
>
> Tested on x86_64-linux.
>
> Martin
>
>
> [*] component_size() happens to work for char arrays because those
> are transformed to STRING_CSTs, but not for arrays that are not.
> E.g., in
>
>   struct S { int64_t i; int16_t j; int16_t a[]; }
>     s = { 0, 0, { 1, 0 } };
>
> unless called with type set to int16_t[2], fold_ctor_reference
> will return s.a[0] rather than all of s.a.  But set type to
> int16_t[2] we would need to know that s.a's initializer has two
> elements, and that's just what we're using fold_ctor_reference
> to find out.
>
> I think this could probably be made to work somehow by extending
> useless_type_conversion_p to handle this case as special somehow,
> but it doesn't seem worth the effort given that there should be
> an easier way to do it as you noted below.
>
> Given the above, the long term solution should be to rely on
> DECL_SIZE_UNIT(decl) - TYPE_SIZE_UNIT(decl_type) as Richard
> suggested in the review of its initial implementation.
> Unfortunately, because of bugs in both the C and C++ front ends
> (I just opened PR 65403 with the details) the simple formula
> doesn't give the right answers either.  So until the bugs are
> fixed, the patch reverts back to the original loopy solution.
> It's no more costly than the current fold_ctor_reference
> approach.
>
> 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
> * builtins.c (component_size): Rename to component_ref_size and move...
> (compute_objsize): Adjust to callee name change.
> * 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, get_flexarray_size): New helpers.
> (component_ref_size): ...move here from builtins.c.  Make extern.
> Use get_flexarray_size instead of fold_ctor_reference.
> * tree.h (component_ref_size): Declare.
> * 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-44.c: New test.
> * gcc.dg/Warray-bounds-45.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.
>

> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c (revision 275387)
> +++ gcc/tree.c (working copy)
> @@ -13860,6 +13902,134 @@ component_ref_field_offset (tree exp)
[ ... ]
> +  /* If the flexible array member has an known size use the greater
> +     of it and the tail padding in the enclosing struct.
> +     Otherwise, when the size of the flexible array member is unknown
> +     and the referenced object is not a struct, use the size of its
> +     type when known.  This detects sizes of array buffers when cast
> +     to struct tyoes with flexible array members.  */
s/tyoes/types/

So no concerns with the patch itself, just the fallout you mentioned in
a follow-up message.  Ideally we'd have glibc and the kernel fixed
before this goes in, but I'd settle for just getting glibc fixed since
we have more influence there.

Out of curiosity are the kernel issues you raised due to flexible arrays
or just cases where we're doing a better job on normal objects?  I'd be
a bit surprised to find flexible arrays in the kernel.

jeff
Reply | Threaded
Open this post in threaded view
|

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

Martin Sebor-2
On 9/10/19 4:35 PM, Jeff Law wrote:

> On 9/6/19 1:27 PM, Martin Sebor wrote:
>> Recent enhancements to -Wstringop-overflow improved the warning
>> to the point that it detects a superset of the problems -Warray-
>> bounds is intended detect in character accesses.  Because both
>> warnings detect overlapping sets of problems, and because the IL
>> they work with tends to change in subtle ways from target to
>> targer, tests designed to verify one or the other sometimes fail
>> with a target where the warning isn't robust enough to detect
>> the problem given the IL representation.
>>
>> To reduce these test suite failures the attached patch extends
>> -Warray-bounds to handle some of the same problems -Wstringop-
>> overflow does, pecifically, out-of-bounds accesses to array
>> members of structs, including zero-length arrays and flexible
>> array members of defined objects.
>>
>> In the process of testing the enhancement I realized that
>> the recently added component_size() function doesn't work as
>> intended for non-character array members (see below).  The patch
>> corrects this by reverting back to the original implementation
>> of the function until the better/simpler solution can be put in
>> place as mentioned below.
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>>
>> [*] component_size() happens to work for char arrays because those
>> are transformed to STRING_CSTs, but not for arrays that are not.
>> E.g., in
>>
>>    struct S { int64_t i; int16_t j; int16_t a[]; }
>>      s = { 0, 0, { 1, 0 } };
>>
>> unless called with type set to int16_t[2], fold_ctor_reference
>> will return s.a[0] rather than all of s.a.  But set type to
>> int16_t[2] we would need to know that s.a's initializer has two
>> elements, and that's just what we're using fold_ctor_reference
>> to find out.
>>
>> I think this could probably be made to work somehow by extending
>> useless_type_conversion_p to handle this case as special somehow,
>> but it doesn't seem worth the effort given that there should be
>> an easier way to do it as you noted below.
>>
>> Given the above, the long term solution should be to rely on
>> DECL_SIZE_UNIT(decl) - TYPE_SIZE_UNIT(decl_type) as Richard
>> suggested in the review of its initial implementation.
>> Unfortunately, because of bugs in both the C and C++ front ends
>> (I just opened PR 65403 with the details) the simple formula
>> doesn't give the right answers either.  So until the bugs are
>> fixed, the patch reverts back to the original loopy solution.
>> It's no more costly than the current fold_ctor_reference
>> approach.
...
>
> So no concerns with the patch itself, just the fallout you mentioned in
> a follow-up message.  Ideally we'd have glibc and the kernel fixed
> before this goes in, but I'd settle for just getting glibc fixed since
> we have more influence there.

Half of the issues there were due to a bug in the warning.  The rest
are caused by Glibc's use of interior zero-length arrays to access
subsequent members.  It works in simple cases but it's very brittle
because GCC assumes that even such members don't alias. If it's meant
to be a supported feature then aliasing would have to be changed to
take it into account.  But I'd rather encourage projects to move away
from these dangerous hacks and towards cleaner, safer code.

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.

> Out of curiosity are the kernel issues you raised due to flexible arrays
> or just cases where we're doing a better job on normal objects?  I'd be
> a bit surprised to find flexible arrays in the kernel.

I don't think I've come across any flexible arrays in the kernel.

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.

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.

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.

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.

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.

Martin

gcc-91647.diff (31K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Martin Sebor-2
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg00860.html

Should I add something like the -Wzero-length-array-bounds option
to allow some of the questionable idioms seen in the kernel?

On 10/11/2019 10:34 AM, Martin Sebor wrote:

> On 9/10/19 4:35 PM, Jeff Law wrote:
>> On 9/6/19 1:27 PM, Martin Sebor wrote:
>>> Recent enhancements to -Wstringop-overflow improved the warning
>>> to the point that it detects a superset of the problems -Warray-
>>> bounds is intended detect in character accesses.  Because both
>>> warnings detect overlapping sets of problems, and because the IL
>>> they work with tends to change in subtle ways from target to
>>> targer, tests designed to verify one or the other sometimes fail
>>> with a target where the warning isn't robust enough to detect
>>> the problem given the IL representation.
>>>
>>> To reduce these test suite failures the attached patch extends
>>> -Warray-bounds to handle some of the same problems -Wstringop-
>>> overflow does, pecifically, out-of-bounds accesses to array
>>> members of structs, including zero-length arrays and flexible
>>> array members of defined objects.
>>>
>>> In the process of testing the enhancement I realized that
>>> the recently added component_size() function doesn't work as
>>> intended for non-character array members (see below).  The patch
>>> corrects this by reverting back to the original implementation
>>> of the function until the better/simpler solution can be put in
>>> place as mentioned below.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Martin
>>>
>>>
>>> [*] component_size() happens to work for char arrays because those
>>> are transformed to STRING_CSTs, but not for arrays that are not.
>>> E.g., in
>>>
>>>    struct S { int64_t i; int16_t j; int16_t a[]; }
>>>      s = { 0, 0, { 1, 0 } };
>>>
>>> unless called with type set to int16_t[2], fold_ctor_reference
>>> will return s.a[0] rather than all of s.a.  But set type to
>>> int16_t[2] we would need to know that s.a's initializer has two
>>> elements, and that's just what we're using fold_ctor_reference
>>> to find out.
>>>
>>> I think this could probably be made to work somehow by extending
>>> useless_type_conversion_p to handle this case as special somehow,
>>> but it doesn't seem worth the effort given that there should be
>>> an easier way to do it as you noted below.
>>>
>>> Given the above, the long term solution should be to rely on
>>> DECL_SIZE_UNIT(decl) - TYPE_SIZE_UNIT(decl_type) as Richard
>>> suggested in the review of its initial implementation.
>>> Unfortunately, because of bugs in both the C and C++ front ends
>>> (I just opened PR 65403 with the details) the simple formula
>>> doesn't give the right answers either.  So until the bugs are
>>> fixed, the patch reverts back to the original loopy solution.
>>> It's no more costly than the current fold_ctor_reference
>>> approach.
> ...
>>
>> So no concerns with the patch itself, just the fallout you mentioned in
>> a follow-up message.  Ideally we'd have glibc and the kernel fixed
>> before this goes in, but I'd settle for just getting glibc fixed since
>> we have more influence there.
>
> Half of the issues there were due to a bug in the warning.  The rest
> are caused by Glibc's use of interior zero-length arrays to access
> subsequent members.  It works in simple cases but it's very brittle
> because GCC assumes that even such members don't alias. If it's meant
> to be a supported feature then aliasing would have to be changed to
> take it into account.  But I'd rather encourage projects to move away
> from these dangerous hacks and towards cleaner, safer code.
>
> 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.
>
>> Out of curiosity are the kernel issues you raised due to flexible arrays
>> or just cases where we're doing a better job on normal objects?  I'd be
>> a bit surprised to find flexible arrays in the kernel.
>
> I don't think I've come across any flexible arrays in the kernel.
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>
> Martin
Reply | Threaded
Open this post in threaded view
|

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

Jeff Law
In reply to this post by Martin Sebor-2
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

Reply | Threaded
Open this post in threaded view
|

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

Martin Sebor-2
On 10/31/19 12:54 PM, Jeff Law wrote:

> 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.
I've added a new option, -Wzero-length-bounds to control this.
It complicates the get_component_size function a bit but the rest
of the changes weren't intrusive.

To make a clear distinction between zero-length arrays and flexible
array members in dianostics I tweaked how the former are shown and
added the zero.  With that, a zero-length array is now rendered as
char[0] and not as char[] as it was before, and as flexible arrays
still are.

I also had to work around by using memcpy instead of a hand rolled
loop an apparent deficiency in loop unrolling (PR 92323) that
triggered a -Warray-bounds false positive in gimple-match-head.c.

Rebuilding the kernel with the updated patch results in the following
breakdown of the two warnings (the numbers are total instances of each,
unique instances, and files they come from):

   -Wzero-length-bounds                 49       46       13
   -Warray-bounds                       45       14        8

The -Warray-bounds instances I checked look legitimate even though
the code is some of them still looks benign.  I'm not sure there's
a good way to relax the warning to sanction some of these abuses
without also missing some bugs.  It might be worth looking into
some more in stage 3, depending on the fallout during mass rebuild.

After bootstrapping on x86_64 and i385 and regtesting I committed
the attached patch in r277728.

Martin

gcc-91647.diff (50K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Jakub Jelinek
On Fri, Nov 01, 2019 at 03:09:39PM -0600, Martin Sebor wrote:
> * gcc.dg/pr36902.c: Remove xfail.

> --- a/gcc/testsuite/gcc.dg/pr36902.c
> +++ b/gcc/testsuite/gcc.dg/pr36902.c
> @@ -44,7 +44,7 @@ foo2(unsigned char * to, const unsigned char * from, int n)
>        *to = *from;
>        break;
>      case 5:
> -      to[4] = from [4]; /* { dg-warning "array subscript is above array bounds" "" { xfail *-*-* } } */
> +      to[4] = from [4]; /* { dg-warning "\\\[-Warray-bounds } */
>        break;
>      }
>    return to;

This FAILs:
+ERROR: gcc.dg/pr36902.c: missing " for " dg-warning 47 "\\\\\\[-Warray-bounds "
+ERROR: gcc.dg/pr36902.c: missing " for " dg-warning 47 "\\\\\\[-Warray-bounds "
Fixed thusly, regtested on x86_64-linux and i686-linux, committed to trunk
as obvious:

2019-11-02  Jakub Jelinek  <[hidden email]>

        * gcc.dg/pr36902.c: Terminate dg-warning regexp string.

--- gcc/testsuite/gcc.dg/pr36902.c.jj 2019-11-01 22:19:48.757844885 +0100
+++ gcc/testsuite/gcc.dg/pr36902.c 2019-11-02 00:21:00.556117852 +0100
@@ -44,7 +44,7 @@ foo2(unsigned char * to, const unsigned
       *to = *from;
       break;
     case 5:
-      to[4] = from [4]; /* { dg-warning "\\\[-Warray-bounds } */
+      to[4] = from [4]; /* { dg-warning "\\\[-Warray-bounds" } */
       break;
     }
   return to;


        Jakub

Reply | Threaded
Open this post in threaded view
|

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

Maciej W. Rozycki-5
In reply to this post by Martin Sebor-2
On Fri, 1 Nov 2019, Martin Sebor wrote:

> Rebuilding the kernel with the updated patch results in the following
> breakdown of the two warnings (the numbers are total instances of each,
> unique instances, and files they come from):
>
>    -Wzero-length-bounds                 49       46       13
>    -Warray-bounds                       45       14        8
>
> The -Warray-bounds instances I checked look legitimate even though
> the code is some of them still looks benign.  I'm not sure there's
> a good way to relax the warning to sanction some of these abuses
> without also missing some bugs.  It might be worth looking into
> some more in stage 3, depending on the fallout during mass rebuild.
>
> After bootstrapping on x86_64 and i385 and regtesting I committed
> the attached patch in r277728.

 It is what I believe has also broken glibc:

In file included from ../sysdeps/riscv/libc-tls.c:19:
../csu/libc-tls.c: In function '__libc_setup_tls':
../csu/libc-tls.c:209:30: error: array subscript 1 is outside the bounds of an interior zero-length array 'struct dtv_slotinfo[0]' [-Werror=zero-length-bounds]
  209 |   static_slotinfo.si.slotinfo[1].map = main_map;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
In file included from ../sysdeps/riscv/ldsodefs.h:46,
                 from ../sysdeps/gnu/ldsodefs.h:46,
                 from ../sysdeps/unix/sysv/linux/ldsodefs.h:25,
                 from ../sysdeps/unix/sysv/linux/riscv/ldsodefs.h:22,
                 from ../csu/libc-tls.c:21,
                 from ../sysdeps/riscv/libc-tls.c:19:
../sysdeps/generic/ldsodefs.h:423:7: note: while referencing 'slotinfo'
  423 |     } slotinfo[0];
      |       ^~~~~~~~
cc1: all warnings being treated as errors

(here in a RISC-V build).

 Has anybody looked yet into how the breakage could possibly be addressed?

  Maciej
Reply | Threaded
Open this post in threaded view
|

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

Jeff Law
On 11/6/19 4:09 PM, Maciej W. Rozycki wrote:

> On Fri, 1 Nov 2019, Martin Sebor wrote:
>
>> Rebuilding the kernel with the updated patch results in the following
>> breakdown of the two warnings (the numbers are total instances of each,
>> unique instances, and files they come from):
>>
>>    -Wzero-length-bounds                 49       46       13
>>    -Warray-bounds                       45       14        8
>>
>> The -Warray-bounds instances I checked look legitimate even though
>> the code is some of them still looks benign.  I'm not sure there's
>> a good way to relax the warning to sanction some of these abuses
>> without also missing some bugs.  It might be worth looking into
>> some more in stage 3, depending on the fallout during mass rebuild.
>>
>> After bootstrapping on x86_64 and i385 and regtesting I committed
>> the attached patch in r277728.
>
>  It is what I believe has also broken glibc:
>
> In file included from ../sysdeps/riscv/libc-tls.c:19:
> ../csu/libc-tls.c: In function '__libc_setup_tls':
> ../csu/libc-tls.c:209:30: error: array subscript 1 is outside the bounds of an interior zero-length array 'struct dtv_slotinfo[0]' [-Werror=zero-length-bounds]
>   209 |   static_slotinfo.si.slotinfo[1].map = main_map;
>       |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from ../sysdeps/riscv/ldsodefs.h:46,
>                  from ../sysdeps/gnu/ldsodefs.h:46,
>                  from ../sysdeps/unix/sysv/linux/ldsodefs.h:25,
>                  from ../sysdeps/unix/sysv/linux/riscv/ldsodefs.h:22,
>                  from ../csu/libc-tls.c:21,
>                  from ../sysdeps/riscv/libc-tls.c:19:
> ../sysdeps/generic/ldsodefs.h:423:7: note: while referencing 'slotinfo'
>   423 |     } slotinfo[0];
>       |       ^~~~~~~~
> cc1: all warnings being treated as errors
>
> (here in a RISC-V build).
>
>  Has anybody looked yet into how the breakage could possibly be addressed?
Yea, Florian posted patches over the weekend to fix glibc.  They're
still going through the review/update cycle.

jeff

Reply | Threaded
Open this post in threaded view
|

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

Maciej W. Rozycki-5
On Wed, 6 Nov 2019, Jeff Law wrote:

> >  It is what I believe has also broken glibc:
> >
> > In file included from ../sysdeps/riscv/libc-tls.c:19:
> > ../csu/libc-tls.c: In function '__libc_setup_tls':
> > ../csu/libc-tls.c:209:30: error: array subscript 1 is outside the bounds of an interior zero-length array 'struct dtv_slotinfo[0]' [-Werror=zero-length-bounds]
> >   209 |   static_slotinfo.si.slotinfo[1].map = main_map;
> >       |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
> > In file included from ../sysdeps/riscv/ldsodefs.h:46,
> >                  from ../sysdeps/gnu/ldsodefs.h:46,
> >                  from ../sysdeps/unix/sysv/linux/ldsodefs.h:25,
> >                  from ../sysdeps/unix/sysv/linux/riscv/ldsodefs.h:22,
> >                  from ../csu/libc-tls.c:21,
> >                  from ../sysdeps/riscv/libc-tls.c:19:
> > ../sysdeps/generic/ldsodefs.h:423:7: note: while referencing 'slotinfo'
> >   423 |     } slotinfo[0];
> >       |       ^~~~~~~~
> > cc1: all warnings being treated as errors
> >
> > (here in a RISC-V build).
> >
> >  Has anybody looked yet into how the breakage could possibly be addressed?
> Yea, Florian posted patches over the weekend to fix glibc.  They're
> still going through the review/update cycle.

 Thanks, I have found them now, now that I knew what to look for and in
what time frame.

 Unfortunately there's no mention of the error message or at least the
name of the `-Wzero-length-bounds' option (which is how I found the GCC
patch) in the respective glibc change descriptions so my mailing list
searches returned nothing.  I think it would be good to try and have
keywords potentially looked for in change descriptions, and verbatim error
messages are certainly good candidates IMO.

 So I went for `-Wno-zero-length-bounds' for my glibc build for the time
being, as my objective now is to get some outstanding GCC stuff in before
stage 1 ends rather than being drawn into glibc build issues.

  Maciej