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

[Martin Sebor]

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?

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: 17582 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210317/9f7a8960/attachment-0001.bin>