[PATCH] [comiler-rt/ubsan] getVtablePrefix must not sanity-check on Prefix->Offset > 0
Duncan P. N. Exon Smith
dexonsmith at apple.com
Thu Jul 16 14:59:40 PDT 2015
I just looked at the Itanium C++ ABI to confirm that positive
offsets are legal. It looks like the are explicitly called out in
the "virtual table components and order" section, under the "i"
bubble of the third bullet point.
https://mentorembedded.github.io/cxx-abi/abi.html#vtable-components
> - The offset to top holds the displacement to the top of the object from the location within the object of the virtual table pointer that addresses this virtual table, as a ptrdiff_t. It is always present. The offset provides a way to find the top of the object from any base subobject with a virtual table pointer. This is necessary for dynamic_cast<void*> in particular.
>
> (i) In a complete object virtual table, and therefore in all of its primary base virtual tables, the value of this offset will be zero. For the secondary virtual tables of other non-virtual bases, and of many virtual bases, it will be negative. Only in some construction virtual tables will some virtual base virtual tables have positive offsets, due to a different ordering of the virtual bases in the full object than in the subobject's standalone layout.
(The last sentence is the key one.)
If that's what the review is waiting on, I'm comfortable LGTM'ing
the semantic change. It's clearly a false positive match. If the
runtime has some way of knowing whether we're inside construction
or destruction, I imagine a more limited form of the check could be
reintroduced.
I don't know UBSan at all, so maybe Alexei wants to confirm whether
the patch itself is fine (looks okay to me).
> On 2015-Jul-16, at 02:02, Stephan Bergmann <sbergman at redhat.com> wrote:
>
> ping^8
>
> On 07/14/2015 08:45 AM, Stephan Bergmann wrote:
>> ping^7
>>
>> It would be great if somebody could get this patch pushed. (It unbreaks
>> UBSan builds of LibreOffice, and it is somewhat painful to maintain as a
>> local patch on all the machines I use for such builds.)
>>
>> I updated the attached getVtablesPrefix.patch to current trunk, and also
>> added the test case from my original mail to it now.
>>
>> Thanks,
>> Stephan
>>
>> On 07/03/2015 09:22 AM, Stephan Bergmann wrote:
>>> ping^6
>>>
>>> On 06/26/2015 08:57 AM, Stephan Bergmann wrote:
>>>> ping1^5
>>>>
>>>> On 06/19/2015 02:02 PM, Stephan Bergmann wrote:
>>>>> ping^4
>>>>>
>>>>> On 06/05/2015 07:06 PM, David Blaikie wrote:
>>>>>> On Fri, Jun 5, 2015 at 10:02 AM, Alexey Samsonov <vonosmas at gmail.com
>>>>>> <mailto:vonosmas at gmail.com>> wrote:
>>>>>>
>>>>>> I referred to David Majnemer, who touched this code a while ago.
>>>>>> But
>>>>>> thanks for suggesting help :)
>>>>>>
>>>>>>
>>>>>> Ah, +Majnemer.
>>>>>>
>>>>>> - Dave
>>>>>>
>>>>>>
>>>>>> On Fri, Jun 5, 2015 at 8:26 AM, David Blaikie <dblaikie at gmail.com
>>>>>> <mailto:dblaikie at gmail.com>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, Jun 4, 2015 at 5:52 PM, Alexey Samsonov
>>>>>> <vonosmas at gmail.com <mailto:vonosmas at gmail.com>> wrote:
>>>>>>
>>>>>> Richard or David, do you want to look into this, or you'd
>>>>>> prefer to leave this for me?
>>>>>>
>>>>>>
>>>>>> I can't say I have much more context in vtable layout & C++
>>>>>> ABI
>>>>>> than you do, most likely. Happy to bounce some the ideas
>>>>>> around
>>>>>> in person if that's helpful.
>>>>>>
>>>>>> - David
>>>>>>
>>>>>>
>>>>>> On Tue, Jun 2, 2015 at 3:32 AM, Stephan Bergmann
>>>>>> <sbergman at redhat.com <mailto:sbergman at redhat.com>> wrote:
>>>>>>
>>>>>> ping^3
>>>>>>
>>>>>>
>>>>>> On 12/16/2014 10:52 AM, Stephan Bergmann wrote:
>>>>>>
>>>>>> ping
>>>>>> On 12/05/2014 09:33 AM, Stephan Bergmann wrote:
>>>>>>
>>>>>> ping
>>>>>> On 08/12/2014 09:10 PM, Alexey Samsonov wrote:
>>>>>>
>>>>>> +Richard
>>>>>>
>>>>>>
>>>>>> On Tue, Aug 12, 2014 at 3:51 AM, Stephan
>>>>>> Bergmann <sbergman at redhat.com
>>>>>> <mailto:sbergman at redhat.com>
>>>>>> <mailto:sbergman at redhat.com
>>>>>> <mailto:sbergman at redhat.com>>> wrote:
>>>>>>
>>>>>> On 08/11/2014 10:19 PM, Alexey
>>>>>> Samsonov
>>>>>> wrote:
>>>>>>
>>>>>> +Richard
>>>>>>
>>>>>> Note, that you'd also have to
>>>>>> update comment for
>>>>>> VtablePrefix::Offset field.
>>>>>>
>>>>>>
>>>>>> ah, right; updated patch
>>>>>>
>>>>>> Stephan
>>>>>>
>>>>>> On Mon, Aug 11, 2014 at 6:30 AM,
>>>>>> Stephan Bergmann
>>>>>> <sbergman at redhat.com
>>>>>> <mailto:sbergman at redhat.com>
>>>>>> <mailto:sbergman at redhat.com
>>>>>> <mailto:sbergman at redhat.com>>
>>>>>> <mailto:sbergman at redhat.com
>>>>>> <mailto:sbergman at redhat.com>
>>>>>> <mailto:sbergman at redhat.com
>>>>>> <mailto:sbergman at redhat.com>>>>
>>>>>> wrote:
>>>>>>
>>>>>> At least with recent Clang
>>>>>> trunk on Linux x86_64:
>>>>>>
>>>>>> $ cat test.cc
>>>>>> #include <iostream>
>>>>>> struct A { virtual ~A()
>>>>>> {} };
>>>>>> struct B: virtual A {};
>>>>>> struct C: virtual A {
>>>>>> ~C()
>>>>>> { std::cout << '\n'; } };
>>>>>> struct D: virtual B,
>>>>>> virtual C {};
>>>>>> int main() { delete new
>>>>>> D; }
>>>>>>
>>>>>> $ clang++
>>>>>> -fsanitize=undefined test.cc
>>>>>>
>>>>>> $ ./a.out
>>>>>> <unknown>: runtime
>>>>>> error:
>>>>>> member call on address
>>>>>> 0x000002a35010
>>>>>> which does not point
>>>>>> to an
>>>>>> object of type 'A'
>>>>>> 0x000002a35010: note:
>>>>>> object has invalid vptr
>>>>>> 00 00 00 00 58 0e
>>>>>> 43 00
>>>>>> 00 00 00 00 30 0e 43 00 00
>>>>>> 00 00 00
>>>>>> 00 00 00 00 00 00
>>>>>> 00 00
>>>>>> e1 0f 02 00
>>>>>>
>>>>>> ^~~~~~~~~~~~~~~~~~~~~~~
>>>>>> invalid
>>>>>> vptr
>>>>>>
>>>>>>
>>>>>> The problem is that
>>>>>> getVtablePrefix
>>>>>> (lib/ubsan/ubsan_type_hash.cc)
>>>>>> rejects any VtablePrefix
>>>>>> with
>>>>>> Offset > 0 as "This can't
>>>>>> possibly be
>>>>>> a valid vtable" but,
>>>>>> according
>>>>>> to the Itanium ABI, "in some
>>>>>> construction virtual tables
>>>>>> will some virtual base virtual
>>>>>> tables
>>>>>> have positive offsets."
>>>>>>
>>>>>> The apparent fix is to
>>>>>> remove
>>>>>> the check, see the attached
>>>>>> getVtablePrefix.patch.
> <getVtablesPrefix.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list