Re: [PATCH] set range for strlen(array) to avoid spurious -Wstringop-overflow (PR 83373 , PR 78450)

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Dec 13, 2017 at 4:47 AM, Martin Sebor <msebor@gmail.com> wrote:
> On 12/12/2017 05:35 PM, Jeff Law wrote:
>>
>> On 12/12/2017 01:15 PM, Martin Sebor wrote:
>>>
>>> Bug 83373 - False positive reported by -Wstringop-overflow, is
>>> another example of warning triggered by a missed optimization
>>> opportunity, this time in the strlen pass.  The optimization
>>> is discussed in pr78450 - strlen(s) return value can be assumed
>>> to be less than the size of s.  The gist of it is that the result
>>> of strlen(array) can be assumed to be less than the size of
>>> the array (except in the corner case of last struct members).
>>>
>>> To avoid the false positive the attached patch adds this
>>> optimization to the strlen pass.  Although the patch passes
>>> bootstrap and regression tests for all front-ends I'm not sure
>>> the way it determines the upper bound of the range is 100%
>>> correct for languages with arrays with a non-zero lower bound.
>>> Maybe it's just not as tight as it could be.
>>
>> What about something hideous like
>>
>> struct fu {
>>   char x1[10];
>>   char x2[10];
>>   int avoid_trailing_array;
>> }
>>
>> Where objects stored in x1 are not null terminated.  Are we in the realm
>> of undefined behavior at that point (I hope so)?
>
>
> Yes, this is undefined.  Pointer arithmetic (either direct or
> via standard library functions) is only defined for pointers
> to the same object or subobject.  So even something like
>
>  memcpy (pfu->x1, pfu->x1 + 10, 10);
>
> is undefined.

There's nothing undefined here - computing the pointer pointing
to one-after-the-last element of an array is valid (you are just
not allowed to dereference it).

>  _FORTIFY_SOURCE allows it for raw memory functions
> because some low level code copies regions of the same object that
> span two or more members (I've seen an example or two in the Linux
> kernel but, IIRC, nowhere else).  With the patch, using memchr
> would be the only way to get away with it.
>
> Other than that, the new -Wstringop-truncation warning is designed
> to prevent creating unterminated character arrays and the manual
> suggests using attribute nonstring when it's necessary.  The
> -Wrestrict warning I'm about to check in also complains about
> forming invalid pointers by built-ins with restrict-qualified
> arguments (although it won't diagnose the above).
>
> Although I would prefer not to, I suppose if letting strlen cross
> the boundaries of subobjects was considered an important use to
> accommodate in limited cases the optimization could be disabled
> for member arrays declared with the new nonstring attribute (while
> still issuing a warning for it as GCC does today).
>
> Another alternative (if the above use case is considered important
> enough) might be to suppress the optimization for member character
> arrays that are immediately followed by another such array.
>
> Martin