On 3/17/21 4:47 PM, Martin Sebor wrote:
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?
- /* Consider the access if its type is a derived class. */
+ /* Consider the access only if its type is a derived polymorphic class
+ with a virtual base. */
OK without this hunk.
Jason
