[PATCH] [comiler-rt/ubsan] getVtablePrefix must not sanity-check on Prefix->Offset > 0
Alexey Samsonov
vonosmas at gmail.com
Thu Jul 16 15:43:29 PDT 2015
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.
Thanks for tracking this down!
On Thu, Jul 16, 2015 at 2:59 PM, Duncan P. N. Exon Smith <
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> 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
>
>
--
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150716/4c94a486/attachment.html>
More information about the llvm-commits
mailing list