[PATCH] [comiler-rt/ubsan] getVtablePrefix must not sanity-check on Prefix->Offset > 0
Stephan Bergmann
sbergman at redhat.com
Tue Aug 4 01:00:20 PDT 2015
On 07/17/2015 12:43 AM, Alexey Samsonov wrote:
> LGTM.
>
> I'm terribly sorry it took me so long to get to it =/. Semantic change
> looks fine to me as well, and I don't think there's an easy way (for
> now) to learn whether we're still constructing virtual table from UBSan
> runtime.
> I'll ask Hans to include this into 3.7. Let me know if I should submit
> it for you.
Yes, please submit (I don't have commit access).
Thanks, Stephan
> On Thu, Jul 16, 2015 at 2:59 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>
> 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 <mailto: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>
> >>>>>> <mailto: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>
> >>>>>> <mailto: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>
> <mailto: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>
> <mailto: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>>
> >>>>>> <mailto: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>>>
> >>>>>> <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
> <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 <mailto:llvm-commits at cs.uiuc.edu>
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com <mailto:vonosmas at gmail.com>
More information about the llvm-commits
mailing list