[PATCH] tighten up checking for virtual bases (PR 99502)

[Martin Sebor]

On 3/17/21 1:40 PM, Jason Merrill wrote:
> On 3/17/21 3:03 PM, Martin Sebor wrote:
>> On 3/16/21 2:24 PM, Jason Merrill wrote:
>>> On 3/11/21 1:06 PM, Martin Sebor wrote:
>>>> More extensive testing of the patch I just committed in r11-7563 to
>>>> avoid the false positive -Warray-bounds on accesses to members of
>>>> virtual bases has exposed a couple of problems that cause many false
>>>> negatives for even basic bugs like:
>>>>
>>>>     typedef struct A { int i; } A;
>>>>
>>>>     void* g (void)
>>>>     {
>>>>       A *p = (A*)malloc (3);
>>>>       p->i = 0;        // missing warning in C++
>>>>       return p;
>>>>     }
>>>>
>>>> as well as a number of others, including more complex ones involving
>>>> virtual base classes that can, with some additional work, be reliably
>>>> detected.  Most of them were successfully diagnosed before the fix
>>>> for PR 98266.  Unfortunately, the tests that exercise this are all
>>>> C so the C++ only regressions due to the divergence went unnoticed.
>>>>
>>>> The root cause is twofold: a) the simplistic check for TYPE_BINFO
>>>> in the new inbounds_vbase_memaccess_p() function means we exclude
>>>> from checking any member accesses whose leading offset is in bounds,
>>>> and b) the check for the leading offset misses cases where just
>>>> the ending offset of an access is out of bounds.
>>>>
>>>> The attached patch adds code to restrict the original solution
>>>> to just the narrow subset of accesses to members of classes with
>>>> virtual bases, and also adds a check for the ending offset.
>>>
>>> Is it enough to just fix the ending offset check, and maybe remove 
>>> "vbase" from the function name?  The shortcut should be valid for 
>>> classes without vbases, as well.
>>
>> You mean check just the ending offset?  I did both in case the size
>> of the member wasn't known or constant but I don't think those cases
>> come up here.  The C++ front end rejects both flexible array members
>> and variably-modified types in base classes so the size of any such
>> member should always be constant (right?)
>>
>> Is the attached update what you're suggesting?
> 
> Ah, no, I meant dropping the has_virtual_base check.

I see.  I wanted to constrain the check just to when it's necessary
but it does seem to work either way at the expense of more checking.
I did have to restore the test for the beginning offset because
the code now lets flexible array members through.  How's the attached
revision?

> 
>> Martin
>>
>>>
>>>> Is the patch okay for trunk?
>>>>
>>>> (The code for has_virtual_base() is adapted from
>>>> polymorphic_type_binfo_p() in ipa-utils.h.  It seems like a handy
>>>> utility that could go in tree.h.)
>>>>
>>>> Martin
>>>>
>>>> PS There is another regression I discovered while working on this.
>>>> It's a false positive but unrelated to the r11-7563 fix so I'll
>>>> post a patch for separately.
>>>
>>
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-99502.diff
Type: text/x-patch
Size: 18149 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210317/b2b64538/attachment-0001.bin>