This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] enhance -Warray-bounds to handle strings and excessive indices

On 11/07/2017 03:18 AM, Richard Biener wrote:

On Tue, Nov 7, 2017 at 4:23 AM, Martin Sebor <msebor@gmail.com> wrote:

On 11/06/2017 11:41 AM, Jeff Law wrote:


On 10/29/2017 10:15 AM, Martin Sebor wrote:


Ping -- please see my reply below.

On 10/20/2017 09:57 AM, Richard Biener wrote:


get_addr_base_and_unit_offset will return NULL if there's any


variable


component in 'ref'.  So as written it seems to be dead code (you
want to pass 'arg'?)




Sorry, I'm not sure I understand what you mean.  What do you think
is dead code?  The call to get_addr_base_and_unit_offset() is also
made for an array of unspecified bound (up_bound is null) and for
an array at the end of a struct.  For those the function returns
non-null, and for the others (arrays of runtime bound) it returns
null.  (I passed arg instead of ref but I see no difference in
my tests.)



If you pass a.b.c[i] it will return NULL, if you pass a.b.c ('arg')


it will


return the offset of 'c'.  If you pass a.b[j].c it will still return


NULL.


You could use get_ref_base_and_extent which will return the offset
of a.b[0].c in this case and sets max_size != size - but you are only
interested in offset.  The disadvantage of get_ref_base_and_extent
is it returns offset in bits thus if the offset is too large for a


HWI


you'll instead get offset == 0 and max_size == -1.

Thus I'm saying this is dead code for variable array accesses
(even for the array you are warning about).  Yes, for constant index
and at-struct-end you'll get sth, but the warning is in VRP because
of variable indexes.

So I suggest to pass 'arg' and use get_ref_base_and_extent
for some extra precision (and possible lossage for very very large
structures).



Computing bit offsets defeats the out-of-bounds flexible array
index detection because the computation overflows (the function
sets the offset to zero).



It shouldn't if you pass arg rather than ref.



I suspect you missed my reply in IRC where I confirmed that this
approach doesn't work for the reason you yourself mentioned above:


The disadvantage of get_ref_base_and_extent
is it returns offset in bits thus if the offset is too large
for a HWI you'll instead get offset == 0 and max_size == -1.



This means that using the function you suggest would either prevent
detecting the out-of-bounds indices that overflow due to the bit
offset, thus largely defeating the purpose of the patch (to detect
excessively large indices), or not printing the value of the out-of
bounds index in the warning in this case, which would in turn mean
further changes to the rest of the logic.


I think Richi's point is that it's more important (in his opinion) to
handle the varying objects within an access like a.b.c[i] than to handle
cases that would overflow when converted to bits.  Thus he'd prefer to
use get_ref_base_and_extent over get_addr_base_and_unit_offset.

I wonder if a hybrid approach here would work.

ie, use get_ref_base_and_extent per Richi's request.  When that returns
an max_size of -1, then call get_addr_base_and_unit_offset and if it
returns that the offset is huge, then warn without any deeper analysis.

Yea, this could trip a false positive if someone makes an array that is
half the address space (give or take), but that's probably not at all
common.  Whereas the additional cases handled by get_ref_base_and_extent
are perhaps more useful.

Thoughts?



I'm always open to suggestions for improvements.  I just haven't
been able to construct a test case to demonstrate the improvement
gained by using get_ref_base_and_extent.  The only test cases I've
come up with show false positives.  It could very well be because
of my lack of experience with the APIs so if there is a trade-off
here that could be compensated for by the approach you suggest I'm
happy to incorporate it into the patch.  I just need a test case
to verify that the solution works as intended.


The difficulty with a testcase like

struct { struct A { int b[1]; } a[5]; } x;

 x.a[i].b[j]

is that b is not considered an array at struct end since one of my
recent changes to array_at_struct_end (basically it disallows having
a flex array as the last member of an array).

It would still stand for non-array components with variable offset
but you can't create C testcases for that.

So yes, for the specific case within the array_at_struct_end_p condition
get_addr_base_and_unit_offset is enough.  IIRC the conditon was
a bit more than just get_addr_base_and_unit_offset.  up_bound !=
INTEGER_CST for example.  So make the above

void foo (int n, int i)
{
 struct { struct A { int b[n]; } a[5]; } x;
 return x.a[i].b[PTRDIFF_MAX/2];
}

with appropriately adjusted constant.  Does that give you the testcase
you want?


Thank you for the test case.  It is diagnosed the same way
irrespective of which of the two functions is used so it serves
to confirm my understanding that the only difference between
the two functions is bits vs bytes.

Unless you have another test case that does demonstrate that
get_ref_base_and_extent is necessary/helpful, is the last patch
okay to commit?

(Again, to be clear, I'm happy to change or enhance the patch if
I can verify that the change handles cases that the current patch
misses.)



As of "it works, catches corner-cases, ..." - yes, it does, but it
adds code that needs to be maintained, may contain bugs, is
executed even for valid code.


Understood.  I don't claim the enhancement is free of any cost
whatsoever.  But it is teeny by most standards and it doesn't
detect just excessively large indices but also negative indices
into last member arrays (bug 68325) and out-of-bounds indices
(bug 82583).  The "excessively large" part does come largely
for free with the other checks.

Martin
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]