1725356 - Various Crashes in MediaTransportHandler due to Null Pointers in IPC Parent

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, I am seeing a lot of different crashes around mozilla::PeerConnectionMedia that look quite weird, like the one above (mozilla-central revision 160071ad9de0+). The crash trace itself does not immediately make sense:

=================================================================
==1686==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7fffe4aeca06 bp 0x7fffc50e96a0 sp 0x7fffc50e9480 T26)
==1686==The signal is caused by a WRITE memory access.
==1686==Hint: address points to the zero page.
    #0 0x7fffe4aeca06 in nsTString<char16_t>::nsTString(nsTString<char16_t>&&) objdir/dist/include/nsTString.h:91:11
    #1 0x7fffe4aeca06 in void mozilla::Maybe<nsTString<char16_t> >::emplace<NS_ConvertASCIItoUTF16>(NS_ConvertASCIItoUTF16&&) objdir/dist/include/mozilla/Maybe.h:838:39
    #2 0x7fffe4aeca06 in nsTString<char16_t>& mozilla::dom::Optional_base<nsTString<char16_t>, nsTString<char16_t> >::Construct<NS_ConvertASCIItoUTF16>(NS_ConvertASCIItoUTF16&&) objdir/dist/include/mozilla/dom/BindingDeclarations.h:164:11
    #3 0x7fffe4aeca06 in mozilla::PeerConnectionImpl::GetStats(mozilla::dom::MediaStreamTrack*, bool) dom/media/webrtc/jsapi/PeerConnectionImpl.cpp:2849:26
    #4 0x7fffe4b152d1 in nsTString<char>::nsTString(char const*, unsigned int) objdir/dist/include/nsTString.h:71:11
    #5 0x7fffe4b152d1 in mozilla::PeerConnectionMedia::StartIceChecks(mozilla::JsepSession const&) dom/media/webrtc/jsapi/PeerConnectionMedia.cpp:336:11
    #6 0x7fffdd7a56dd in nsTArray_Impl<mozilla::dom::MessageData, nsTArrayInfallibleAllocator>::~nsTArray_Impl() objdir/dist/include/nsTArray.h:1037:7
    #7 0x7fffde29c7ce in nsTArray_Impl<mozilla::dom::indexedDB::PBackgroundIDBFactoryParent*, nsTArrayInfallibleAllocator>::end() const objdir/dist/include/mozilla/ipc/ProtocolUtils.h:0:0
    #8 0x7fffde29c7ce in mozilla::ManagedContainer<mozilla::dom::indexedDB::PBackgroundIDBFactoryParent>::end() const objdir/dist/include/mozilla/ipc/ProtocolUtils.h:767:40
    #9 0x7fffde29c7ce in mozilla::ipc::PBackgroundParent::ClearSubtree() objdir/ipc/ipdl/PBackgroundParent.cpp:6696:20
    #10 0x7fffdc943dac in mozilla::ipc::NodeChannel::SendMessage(mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message> >) ipc/glue/NodeChannel.cpp:189:5
    #11 0x7fffdc93fccd in nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) xpcom/base/nsCOMPtr.h:377:7
    #12 0x7fffdc93fccd in nsCOMPtr<nsITimer>::assign_assuming_AddRef(nsITimer*) xpcom/base/nsCOMPtr.h:400:20
    #13 0x7fffdc93fccd in nsCOMPtr<nsITimer>& nsCOMPtr<nsITimer>::operator=<nsITimer>(already_AddRefed<nsITimer>&&) xpcom/base/nsCOMPtr.h:715:5
    #14 0x7fffdc93fccd in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:279:21
    #15 0x7fffdc942209 in mozilla::UniquePtr<IPC::Channel, mozilla::DefaultDelete<IPC::Channel> >::get() const objdir/dist/include/mozilla/UniquePtr.h:290:32
    #16 0x7fffdc942209 in mozilla::UniquePtr<IPC::Channel, mozilla::DefaultDelete<IPC::Channel> >::operator->() const objdir/dist/include/mozilla/UniquePtr.h:285:12
    #17 0x7fffdc942209 in mozilla::ipc::NodeChannel::Start(bool) ipc/glue/NodeChannel.cpp:87:23
    #18 0x7fffdc942cbc in mozilla::ipc::NodeChannel::OnMessageReceived(IPC::Message&&) ipc/glue/NodeChannel.cpp:213:0
    [...]

However, I was able to reproduce these crashes and debug them locally. I believe at least some of them are caused by null pointer derefs in MediaTransportHandlerSTS. In particular, there is an mInitPromise pointer being initialized in MediaTransportHandlerSTS::CreateIceCtx but many other methods use mInitPromise->Then without checking that the pointer was ever set. The weird crash traces could be caused by optimizations / reordering, I suggest adding release asserts to check the pointers.

This bug itself is likely not security-sensitive, if the only reason here is the null-derefs. However, I would like to keep this bug hidden until we have progressed further with our IPC fuzzing efforts to not draw unwanted attention.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Bug 1725356 - Check MediaTransportHandlerSTS::mInitPromise before use. r?bwc

Comment 3

[Byron Campen [:bwc]]

I've looked at the asan output, and I cannot make any sense of it either. It looks like stack corruption to me, although shouldn't asan notice that before things go completely off the rails? Maybe it is bad symbols or something? Small bits of it are at least plausible, but there are some pretty glaring "impossible" jumps.

This stack is apparently from the IPDL Background thread, and it seems to be calling nsThreadManager::Shutdown. Shouldn't that be a main-thread only kind of thing?

The asan stack trace for the creation of the IPDL Background thread also looks bogus.

Comment 4

[Christian Holler (:decoder)]

Yea, there could also be some sort of symbolizing issue at work here. I am in the process of moving this setup to a new server and will see if I hit the issue again with the patch attached. I could confirm that the fuzzer indeed hits the release asserts added here (this is what I was basing my patch on).

Comment 5

[Christian Holler (:decoder)]

I think I found the problem, there was a build mismatch in the experimental setup. This stack should be accurate (I simplified some of the STL mess in the trace for readability):

==1495==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7f70f14d47b7 bp 0x7f705d7a66a0 sp 0x7f705d7a6540 T24)
==1495==The signal is caused by a WRITE memory access.
==1495==Hint: address points to the zero page.
    #0 0x7f70f14d47b7 in std::__atomic_base<unsigned long>::fetch_add(unsigned long, std::memory_order) bits/atomic_base.h:514:16
    #1 0x7f70f14d47b7 in mozilla::ThreadSafeAutoRefCnt::operator++() objdir-ff-asan/dist/include/nsISupportsImpl.h:354:19
    #2 0x7f70f14d47b7 in mozilla::MozPromiseRefcountable::AddRef() objdir-ff-asan/dist/include/mozilla/MozPromise.h:151:3
    #3 0x7f70f14d47b7 in mozilla::RefPtrTraits<mozilla::MozPromise<bool, ...>, false> >::AddRef(mozilla::MozPromise<bool, ...>, false>*) objdir-ff-asan/dist/include/mozilla/RefPtr.h:49:39
    #4 0x7f70f14d47b7 in RefPtr<mozilla::MozPromise<bool, ...>, false> >::ConstRemovingRefPtrTraits<mozilla::MozPromise<bool, ...>, false> >::AddRef(mozilla::MozPromise<bool, ...>, false>*) objdir-ff-asan/dist/include/mozilla/RefPtr.h:380:35
    #5 0x7f70f14d47b7 in RefPtr<mozilla::MozPromise<bool, ...>, false> >::RefPtr(mozilla::MozPromise<bool, ...>, false>*) objdir-ff-asan/dist/include/mozilla/RefPtr.h:109:7
    #6 0x7f70f14d47b7 in mozilla::MozPromise<bool, ...>, false>*) objdir-ff-asan/dist/include/mozilla/MozPromise.h:975:57
    #7 0x7f70f14d47b7 in mozilla::MozPromise<bool, ...> const&)::$_11&&) objdir-ff-asan/dist/include/mozilla/MozPromise.h:1046:12
    #8 0x7f70f14d47b7 in mozilla::MediaTransportHandlerSTS::StartIceChecks(bool, std::vector<...> const&) dom/media/webrtc/jsapi/MediaTransportHandler.cpp:895:17
    #9 0x7f70f14f889e in mozilla::MediaTransportParent::RecvStartIceChecks(bool const&, std::vector<...> const&) dom/media/webrtc/jsapi/MediaTransportParent.cpp:188:20
    #10 0x7f70ea196281 in mozilla::dom::PMediaTransportParent::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PMediaTransportParent.cpp:970:64
    #11 0x7f70eac83291 in mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PBackgroundParent.cpp:3478:32
    #12 0x7f70e9346dac in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) ipc/glue/MessageChannel.cpp:2099:25
    #13 0x7f70e9342ced in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) ipc/glue/MessageChannel.cpp:2023:9
    #14 0x7f70e9345209 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) ipc/glue/MessageChannel.cpp:1861:3
    #15 0x7f70e9345cd0 in mozilla::ipc::MessageChannel::MessageTask::Run() ipc/glue/MessageChannel.cpp:1899:13
    [...]

Comment 6

[Byron Campen [:bwc]]

How is StartIceChecks being called before CreateIceCtx here? Are you fuzzing the IPC API directly or something?

Comment 7

[Christian Holler (:decoder)]

(In reply to Byron Campen [:bwc] from comment #6)

How is StartIceChecks being called before CreateIceCtx here? Are you fuzzing the IPC API directly or something?

Yes, that's exactly what I am doing :)

Comment 8

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

Check MediaTransportHandlerSTS::mInitPromise before use. r=bwc
https://hg.mozilla.org/integration/autoland/rev/b7bddce8fe3b4b709d7616ee947a8e9974b20049
https://hg.mozilla.org/mozilla-central/rev/b7bddce8fe3b