1673525 - UndefinedBehaviorSanitizer: /builds/worker/workspace/obj-build/dist/include/SharedBuffer.h:58:30: runtime error: applying non-zero offset 16 to null pointer

Status: VERIFIED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.zip

Testcase found while fuzzing mozilla-central rev b1a74943bc51 (built with --enable-debug).

/builds/worker/workspace/obj-build/dist/include/SharedBuffer.h:58:30: runtime error: applying non-zero offset 16 to null pointer

    #0 0x7f442ba20760 in Data /builds/worker/workspace/obj-build/dist/include/SharedBuffer.h:58:30
    #1 0x7f442ba20760 in SendStreamAudio /builds/worker/checkouts/gecko/dom/media/mediasink/DecodedStream.cpp:627:69
    #2 0x7f442ba20760 in mozilla::DecodedStream::SendAudio(double, nsMainThreadPtrHandle<nsIPrincipal> const&) /builds/worker/checkouts/gecko/dom/media/mediasink/DecodedStream.cpp:663:5
    #3 0x7f442ba1d0e6 in mozilla::DecodedStream::SendData() /builds/worker/checkouts/gecko/dom/media/mediasink/DecodedStream.cpp:923:3
    #4 0x7f442ba1b2b3 in mozilla::DecodedStream::Start(mozilla::media::TimeUnit const&, mozilla::MediaInfo const&) /builds/worker/checkouts/gecko/dom/media/mediasink/DecodedStream.cpp:500:5
    #5 0x7f442ba28192 in mozilla::VideoSink::Start(mozilla::media::TimeUnit const&, mozilla::MediaInfo const&) /builds/worker/checkouts/gecko/dom/media/mediasink/VideoSink.cpp:217:29
    #6 0x7f442b43951b in mozilla::MediaDecoderStateMachine::StartMediaSink() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:3364:29
    #7 0x7f442b4235a4 in mozilla::MediaDecoderStateMachine::MaybeStartPlayback() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:2966:3
    #8 0x7f442b5c720f in mozilla::MediaDecoderStateMachine::CompletedState::Step() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:2008:16
    #9 0x7f442b5c844f in mozilla::MediaDecoderStateMachine::CompletedState::Enter() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:1991:5
    #10 0x7f442b4218a1 in CallEnterMemberFunction<mozilla::MediaDecoderStateMachine::CompletedState> /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:265:16
    #11 0x7f442b4218a1 in decltype(ReturnTypeHelper(&(mozilla::MediaDecoderStateMachine::CompletedState::Enter))) mozilla::MediaDecoderStateMachine::StateObject::SetState<mozilla::MediaDecoderStateMachine::CompletedState>() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:306:12
    #12 0x7f442b5ed0e6 in mozilla::MediaDecoderStateMachine::RequestAudioData()::$_22::operator()(mozilla::MediaResult const&) const /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp
    #13 0x7f442b5eb8c4 in InvokeMethod<(lambda at /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:3229:11), void ((lambda at /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:3229:11)::*)(const mozilla::MediaResult &) const, mozilla::MediaResult> /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:553:12
    #14 0x7f442b5eb8c4 in InvokeCallbackMethod<false, (lambda at /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:3229:11), void ((lambda at /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:3229:11)::*)(const mozilla::MediaResult &) const, mozilla::MediaResult, RefPtr<mozilla::MozPromise<RefPtr<mozilla::AudioData>, mozilla::MediaResult, true>::Private> > /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:584:5
    #15 0x7f442b5eb8c4 in mozilla::MozPromise<RefPtr<mozilla::AudioData>, mozilla::MediaResult, true>::ThenValue<mozilla::MediaDecoderStateMachine::RequestAudioData()::$_21, mozilla::MediaDecoderStateMachine::RequestAudioData()::$_22>::DoResolveOrRejectInternal(mozilla::MozPromise<RefPtr<mozilla::AudioData>, mozilla::MediaResult, true>::ResolveOrRejectValue&) /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:773:9
    #16 0x7f442b5c1c4e in mozilla::MozPromise<RefPtr<mozilla::AudioData>, mozilla::MediaResult, true>::ThenValueBase::ResolveOrRejectRunnable::Run() /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:410:21
    #17 0x7f442522b9c0 in mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() /builds/worker/workspace/obj-build/dist/include/mozilla/TaskDispatcher.h:228:35
    #18 0x7f44252348c5 in mozilla::TaskQueue::Runner::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskQueue.cpp:158:20
    #19 0x7f442525f1ca in nsThreadPool::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadPool.cpp:299:14
    #20 0x7f4425250467 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1197:14
    #21 0x7f442525affc in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:513:10
    #22 0x7f4426534f22 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:302:20
    #23 0x7f4426435641 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:334:10
    #24 0x7f4426435641 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:327:3
    #25 0x7f4426435641 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:309:3
    #26 0x7f4425249572 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:442:10
    #27 0x7f443e69842e in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #28 0x7f4441f90608 in start_thread /build/glibc-ZN95T4/glibc-2.31/nptl/pthread_create.c:477:8
    #29 0x7f4441b59292 in clone /build/glibc-ZN95T4/glibc-2.31/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Comment 1

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/Jpq_pFqYVdEZT9Emwiyg5w/index.html

Comment 2

[Jason Kratzer [:jkratzer]]

The bug appears to have been introduced in the following build range:

Start: 2a46f29c8cdd6f6269483e86c6e21e649ffa7127 (20201020234416)
End: 4c3e6fb95aa6184651f215d466e702ff7d1660d3 (20201021000031)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2a46f29c8cdd6f6269483e86c6e21e649ffa7127&tochange=4c3e6fb95aa6184651f215d466e702ff7d1660d3

Comment 3

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1595994

Comment 4

[Jim Mathies [:jimm]]

Work is currently kept in nightly.

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

When testing this it looks like the major important pref to set/action to take is to allow autoplay on the page. I tested this using debug builds which I believe trigger asserts due to null ptrs being involved.

After looking at this for a bit I re-bisected due to being unable to find the issue in bug 1595994. I believe bug 1566389 may actually be our regressor. Specifically https://phabricator.services.mozilla.com/D91773 , I verified locally via hg backout bc144d4e01d0 after which I no longer hit asserts.

Bug investigation:

When we don't have any audio data the logic in decoded stream will get a null ptr when we try and get our buffer[0] and the UB results from that.

I don't know this code well, and it makes references to mimicking other code that seems likely to have changed since this was written (e.g. this code references DecodedAudioDataSink, which no longer exists).

It looks like the code is supposed to push silence to fill gaps and then push data after that silence. I wonder if it's just missing an edge case, or if there's some greater issue here. Will keep looking.

[0] https://searchfox.org/mozilla-central/rev/23c25cd32a1e87095301273937b4ee162f41e860/dom/media/mediasink/DecodedStream.cpp#625

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Updating flags based on adjusted regressor.

Comment 7

[Jean-Yves Avenard [:jya]]

Wouldn't https://bugzilla.mozilla.org/show_bug.cgi?id=1669503 be a fix for this?

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(In reply to Jean-Yves Avenard [:jya] from comment #7)

Wouldn't https://bugzilla.mozilla.org/show_bug.cgi?id=1669503 be a fix for this?

I'll believe it. I'm not familiar with various machinery between the demux and here, but I'd expect since mp3's made this unhappy it could be fixed there too.

It looks like this code is doing something similar to https://searchfox.org/mozilla-central/rev/38ed718a101aca27db25984413c052ccd8c0ceda/dom/media/mediasink/AudioSink.cpp#320 and historically I guess it was going for something like https://searchfox.org/mozilla-central/rev/74504dc15aad2b850911040a3cd8041e4faf4c79/dom/media/mediasink/DecodedAudioDataSink.cpp#396

If the intention is to do the same thing, I wonder if we can abstract out that logic. However, since I don't know the code I don't want to spend time working on something to find out we'd prefer another solution or that his is entirely a problem elsewhere. I've got a band aid for this I'll stick up + a crashtest for this.

NI Paul for comment on the mp3 changes.

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1673525 - Add crashtest.

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1673525 - Don't try and process audio with no frames in DecodedStream.

Depends on D100399

Comment 11

[Andreas Pehrson [:pehrsons]]

(In reply to Bryce Seager van Dyk (:bryce) from comment #8)

If the intention is to do the same thing, I wonder if we can abstract out that logic. However, since I don't know the code I don't want to spend time working on something to find out we'd prefer another solution or that his is entirely a problem elsewhere. I've got a band aid for this I'll stick up + a crashtest for this.

It is. Long-term we want to route audio from playback via the graph so that we don't need two separate sinks. Intermediate-term we might want to use one sink for both audio backends, probably as part of shipping captureStream, that would also de-dupe this code.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0221fa8b967
Add crashtest. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/cf453005952e
Don't try and process audio with no frames in DecodedStream. r=pehrsons

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d0221fa8b967
https://hg.mozilla.org/mozilla-central/rev/cf453005952e

Comment 14

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210112215931-bdc025488805.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.