1442819 - UBSan: downcast of address which does not point to an object of type 'const mozilla::MediaDecoder'

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Tyson Smith [:tsmith]]

This was triggered while loading a video on twitch.tv when built with -fsanitize=vptr

Found with mozilla-central changeset: 406399:f4e33c42faa7

mozilla-central/objdir-ff-vptr/dist/include/DecoderDoctorLogger.h:483:42: runtime error: downcast of address 0x615000655700 which does not point to an object of type 'const mozilla::MediaDecoder'
0x615000655700: note: object has invalid vptr
 dc 04 80 73  e4 e4 e4 e4 e4 e4 e4 e4  e4 e4 e4 e4 e4 e4 e4 e4  e4 e4 e4 e4 e4 e4 e4 e4  e4 e4 e4 e4
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr
    #0 0x7fe1e4256fe7 in DecoderDoctorLifeLogger mozilla-central/objdir-ff-vptr/dist/include/DecoderDoctorLogger.h:483:42
    #1 0x7fe1e4256fe7 in mozilla::MediaDecoder::MediaDecoder(mozilla::MediaDecoderInit&) mozilla-central/dom/media/MediaDecoder.cpp:211
    #2 0x7fe1e48d7d48 in mozilla::MediaSourceDecoder::MediaSourceDecoder(mozilla::MediaDecoderInit&) mozilla-central/dom/media/mediasource/MediaSourceDecoder.cpp:39:5
    #3 0x7fe1e3e4fc73 in mozilla::dom::HTMLMediaElement::LoadResource() mozilla-central/dom/html/HTMLMediaElement.cpp:2598:46
    #4 0x7fe1e3e4b823 in mozilla::dom::HTMLMediaElement::SelectResource() mozilla-central/dom/html/HTMLMediaElement.cpp:2101:12
    #5 0x7fe1e3e471ab in mozilla::dom::HTMLMediaElement::SelectResourceWrapper() mozilla-central/dom/html/HTMLMediaElement.cpp:2044:3
    #6 0x7fe1e3f06047 in applyImpl<mozilla::dom::HTMLMediaElement, void (mozilla::dom::HTMLMediaElement::*)()> mozilla-central/objdir-ff-vptr/dist/include/nsThreadUtils.h:1149:12
    #7 0x7fe1e3f06047 in apply<mozilla::dom::HTMLMediaElement, void (mozilla::dom::HTMLMediaElement::*)()> mozilla-central/objdir-ff-vptr/dist/include/nsThreadUtils.h:1155
    #8 0x7fe1e3f06047 in mozilla::detail::RunnableMethodImpl<mozilla::dom::HTMLMediaElement*, void (mozilla::dom::HTMLMediaElement::*)(), true, (mozilla::RunnableKind)0>::Run() mozilla-central/objdir-ff-vptr/dist/include/nsThreadUtils.h:1200
    #9 0x7fe1e3eeb6a6 in mozilla::dom::nsSyncSection::Run() mozilla-central/dom/html/HTMLMediaElement.cpp:1888:16
    #10 0x7fe1db4088c3 in mozilla::CycleCollectedJSContext::ProcessStableStateQueue() mozilla-central/xpcom/base/CycleCollectedJSContext.cpp:307:12
    #11 0x7fe1db40a9c4 in mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) mozilla-central/xpcom/base/CycleCollectedJSContext.cpp:367:3
    #12 0x7fe1de79ac33 in XPCJSContext::AfterProcessTask(unsigned int) mozilla-central/js/xpconnect/src/XPCJSContext.cpp:1261:30
    #13 0x7fe1db658e6e in nsThread::ProcessNextEvent(bool, bool*) mozilla-central/xpcom/threads/nsThread.cpp:1056:24
    #14 0x7fe1db699d3e in NS_ProcessNextEvent(nsIThread*, bool) mozilla-central/xpcom/threads/nsThreadUtils.cpp:517:10
    #15 0x7fe1dcfc1478 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) mozilla-central/ipc/glue/MessagePump.cpp:97:21
    #16 0x7fe1dce4745d in RunHandler mozilla-central/ipc/chromium/src/base/message_loop.cc:319:3
    #17 0x7fe1dce4745d in MessageLoop::Run() mozilla-central/ipc/chromium/src/base/message_loop.cc:299
    #18 0x7fe1e62c7246 in nsBaseAppShell::Run() mozilla-central/widget/nsBaseAppShell.cpp:157:27
    #19 0x7fe1eb945ab4 in XRE_RunAppShell() mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:892:22
    #20 0x7fe1dce4745d in RunHandler mozilla-central/ipc/chromium/src/base/message_loop.cc:319:3
    #21 0x7fe1dce4745d in MessageLoop::Run() mozilla-central/ipc/chromium/src/base/message_loop.cc:299
    #22 0x7fe1eb944bf7 in XRE_InitChildProcess(int, char**, XREChildData const*) mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:718:34
    #23 0x516234 in content_process_main(mozilla::Bootstrap*, int, char**) mozilla-central/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #24 0x516a50 in main mozilla-central/browser/app/nsBrowserApp.cpp:280:18
    #25 0x7fe2019401c0 in __libc_start_main /build/glibc-itYbWN/glibc-2.26/csu/../csu/libc-start.c:308
    #26 0x41eef9 in _start (mozilla-central/objdir-ff-vptr/dist/bin/firefox+0x41eef9)

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I see what's happening:
During the construction of a MediaDecoder object, the base class DecoderDoctorLifeLogger<MediaDecoder> is constructed first, and it does a static_cast<MediaDecoder*>(this), but at this point the MediaDecoder derived object is not fully constructed yet.
So the UBSan analysis is correct.

However I believe it never becomes true UB in this case:
- static_cast may be used to downcast from a (non-virtual) base class to a derived class, and there is no runtime check during this cast, i.e., the derived object is not accessed.
- DDLogger never dereferences these pointers anyway, they're used only as object identifiers in log messages.

To please UBSan we would need to move the construction-logging lines into the object constructors themselves (or will UBSan complain `this` is used when the object it not fully constructed yet?) for about 70 classes.
Obviously I'd prefer to WONTFIX this, but I'm biased and lazy. :-)

Tyson, can we tell UBSan to ignore this cast? Other suggestions?

Comment 2

[Ralph Giles (:rillian)]

Thanks for taking this on, Gerald!

Comment 3

[Nathan Froyd [:froydnj]]

Tyson asked me to look at this bug.

All that DecoderDoctorLogger wants is the actual pointer of the object being constructed--in particular, an untyped pointer to that object--not the pointer to some superclass of that object, right?  So in DecoderDoctorLifeLogger, instead of:

    DecoderDoctorLogger::LogConstruction(static_cast<const T*>(this));

You need to do something horrible, like:

    // Warning, possibly buggy code ahead!
    auto* thisObj = reinterpret_cast<const char*>(this);
    // These 0x0s may want to be something bigger to avoid potential underflow issues with pointers.
    // XPCOM QI tables use 0x1000 instead.
    const size_t offsetToSubclass = reinterpret_cast<char*>(static_cast<DecoderDoctorLifeLogger*>((T*)0x0)) -
        reinterpret_cast<char*>((T*)0x0);
    auto* subclassObj = thisObj + offsetToSubclass;
    DeceoderDoctorLogger::LogConstruction(static_cast<const void*>(subclassObj));

and manually construct the pointer to the subclass that you want, without actually calling it a pointer to T*.  Ideally UBSan will ignore the static_casts to T* when computing offsetToSubclass (presumably it has logic for those), or perhaps there's some clever way to force it to ignore them.

Does that seem like it might work?

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

It would possibly work, but... Eww :-P

If we're trying to prevent UBSan from analyzing pointers that are never dereferenced anyway, I'd much prefer using a targeted `__attribute__((no_sanitize("vptr")))` [1] than trying to hide (non-dereferencing) pointer arithmetic through sneaky type manipulations.


Reading the UBSan doc [1]:
- "-fsanitize=vptr: Use of an object whose vptr indicates that it is of the wrong dynamic type, or that its lifetime has not begun or has ended." -- Notice the words "Use" and "object".
- "UndefinedBehaviorSanitizer is not expected to produce false positives."
I would now argue that in this case, UBSan is in fact producing a false positive (or the doc needs clarifying), because the *object* is never actually *used*. I'll try and contact the UBSan devs about that...

[1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#issue-suppression

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1442819 - Suppress UBSan false positive -

Review commit: https://reviewboard.mozilla.org/r/227172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227172/

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8958229 [details]
Bug 1442819 - Suppress UBSan false positive -

Tyson, since you seem to have UBSan handy, would you be able to test this patch to see if that makes UBSan happy?

Comment 7

[Tyson Smith [:tsmith]]

(In reply to Gerald Squelart [:gerald] from comment #6)
> Comment on attachment 8958229 [details]
> Bug 1442819 - Suppress UBSan false positive -
> 
> Tyson, since you seem to have UBSan handy, would you be able to test this patch to see if that makes UBSan happy?

I am still seeing the issue with the patch applied. I would have assumed that would make UBSan happy.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you for trying. I'll see if I can spin UBSan locally and fix this...

Comment 9

[Tyson Smith [:tsmith]]

Attachment: mozconfig

Here is the mozconfig I was using

Comment 10

[Tyson Smith [:tsmith]]

Let me retry your patch Gerald.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thanks Tyson.
As discussed, I tried on Fedora, I saw the vptr issues in DecoderDoctorLogger.h without the patch, and they were gone with the patch.

But if the patch is still not working for you, I can try Nathan's trick instead.

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

In case it helps, some more details:
Fedora 27 (with all updates)
clang 5.0.1
central from yesterday (3d21d31141dc)
mozconfig as posted in comment 9

Comment 13

[Tyson Smith [:tsmith]]

I purged old logs and clobbered old builds.
Gerald, with your patch applied I can no longer reproduce the issue.

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8958229 [details]
Bug 1442819 - Suppress UBSan false positive -

https://reviewboard.mozilla.org/r/227172/#review236084

r=me with the followup filed.  Bonus points for writing the followup patch!

::: dom/media/doctor/DecoderDoctorLogger.h:483
(Diff revision 1)
> +  // This constructor is called before the `T` object is fully constructed, and
> +  // pointers are not dereferenced anyway, so UBSan shouldn't check vptrs.
> +  __attribute__((no_sanitize("vptr")))

Can you file a followup to add a proper attribute in `mfbt/Attributes.h` for this sort of thing?

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8958229 [details]
Bug 1442819 - Suppress UBSan false positive -

https://reviewboard.mozilla.org/r/227172/#review236084

> Can you file a followup to add a proper attribute in `mfbt/Attributes.h` for this sort of thing?

Will do, thanks for the reminder.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4e466004d34
Suppress UBSan false positive - r=froydnj

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e4e466004d34