Closed Bug 1453030 Opened 6 years ago Closed 6 years ago

Crash [@ mozilla::WebrtcVideoConduit::ConfigureRecvMediaCodecs]

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Version:
59 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: jkratzer, Assigned: dminor)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files)

trigger.html
630 bytes, text/html
Details
Bug 1453030 - Only create RTCDTMFSender on audio RTCRtpSender senders;
59 bytes, text/x-review-board-request
bwc
: review+
jcristau
: approval-mozilla-beta-
Details
Bug 1453030 - Add crashtest;
59 bytes, text/x-review-board-request
bwc
: review+
Details
Attached file trigger.html — 
Testcase found while fuzzing mozilla-central rev a8061a09cd70.

==29995==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f7a4b73a4bd bp 0x7f7a3fced240 sp 0x7f7a3fcecf20 T11)
==29995==The signal is caused by a READ memory access.
==29995==Hint: address points to the zero page.
    #0 0x7f7a4b73a4bc in mozilla::WebrtcVideoConduit::ConfigureRecvMediaCodecs(std::vector<mozilla::VideoCodecConfig*, std::allocator<mozilla::VideoCodecConfig*> > const&) /builds/worker/workspace/build/src/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1318
    #1 0x7f7a4b855828 in operator() /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:580:18
    #2 0x7f7a4b855828 in apply<(lambda at /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:577:39)> /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:62
    #3 0x7f7a4b855828 in mozilla::runnable_args_func<mozilla::TransceiverImpl::InsertDTMFTone(int, unsigned int)::$_3>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:102
    #4 0x7f7a49bda4f8 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1096:14
    #5 0x7f7a49bf6930 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #6 0x7f7a49e346f4 in mozilla::net::nsSocketTransportService::Run() /builds/worker/workspace/build/src/netwerk/base/nsSocketTransportService2.cpp:1007:21
    #7 0x7f7a49e36b4c in non-virtual thunk to mozilla::net::nsSocketTransportService::Run() /builds/worker/workspace/build/src/netwerk/base/nsSocketTransportService2.cpp
    #8 0x7f7a49bda4f8 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1096:14
    #9 0x7f7a49bf6930 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #10 0x7f7a4aacbfab in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:334:20
    #11 0x7f7a4aa1ad89 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #12 0x7f7a4aa1ad89 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #13 0x7f7a4aa1ad89 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #14 0x7f7a49bd4948 in nsThread::ThreadFunc(void*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:425:11
    #15 0x7f7a6752547e in _pt_root /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #16 0x7f7a6ab266b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
    #17 0x7f7a69ba841c in clone /build/glibc-Cl5G7W/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1318 in mozilla::WebrtcVideoConduit::ConfigureRecvMediaCodecs(std::vector<mozilla::VideoCodecConfig*, std::allocator<mozilla::VideoCodecConfig*> > const&)
Thread T11 (Socket Thread) created by T0 (file:// Content) here:
    #0 0x4ae80d in __interceptor_pthread_create /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:204:3
    #1 0x7f7a675221cf in _PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:433:14
    #2 0x7f7a67521dbe in PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:518:12
    #3 0x7f7a49bd68c3 in nsThread::Init(nsTSubstring<char> const&) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:612:8
    #4 0x7f7a49be04aa in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp:471:22
    #5 0x7f7a49bf0a34 in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:143:45
    #6 0x7f7a49e314a8 in NS_NewNamedThread<14> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:72:10
    #7 0x7f7a49e314a8 in mozilla::net::nsSocketTransportService::Init() /builds/worker/workspace/build/src/netwerk/base/nsSocketTransportService2.cpp:590
    #8 0x7f7a4a9f5f8c in nsSocketTransportServiceConstructor(nsISupports*, nsID const&, void**) /builds/worker/workspace/build/src/netwerk/build/nsNetModule.cpp:75:1
    #9 0x7f7a49b8a265 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:1049:19
    #10 0x7f7a49b8182d in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:1409:10
    #11 0x7f7a49b90275 in CallGetService /builds/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp:67:43
    #12 0x7f7a49b90275 in nsGetServiceByContractIDWithError::operator()(nsID const&, void**) const /builds/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp:292
    #13 0x7f7a49a4b74a in nsCOMPtr_base::assign_from_gs_contractid_with_error(nsGetServiceByContractIDWithError const&, nsID const&) /builds/worker/workspace/build/src/xpcom/base/nsCOMPtr.cpp:106:7
    #14 0x7f7a49d8a650 in operator= /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:704:5
    #15 0x7f7a49d8a650 in InitializeSocketTransportService /builds/worker/workspace/build/src/netwerk/base/nsIOService.cpp:293
    #16 0x7f7a49d8a650 in mozilla::net::nsIOService::SetOffline(bool) /builds/worker/workspace/build/src/netwerk/base/nsIOService.cpp:1136
    #17 0x7f7a49d8975c in mozilla::net::nsIOService::Init() /builds/worker/workspace/build/src/netwerk/base/nsIOService.cpp:257:5
    #18 0x7f7a49d8bd39 in mozilla::net::nsIOService::GetInstance() /builds/worker/workspace/build/src/netwerk/base/nsIOService.cpp:354:13
    #19 0x7f7a4a9f5d27 in nsIOServiceConstructor(nsISupports*, nsID const&, void**) /builds/worker/workspace/build/src/netwerk/build/nsNetModule.cpp:57:1
    #20 0x7f7a49b8a265 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:1049:19
    #21 0x7f7a49b8182d in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:1409:10
    #22 0x7f7a4baf5744 in CallGetService<nsIIOService> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsServiceManagerUtils.h:89:10
    #23 0x7f7a4baf5744 in nsScriptSecurityManager::Init() /builds/worker/workspace/build/src/caps/nsScriptSecurityManager.cpp:1332
    #24 0x7f7a4baf6330 in nsScriptSecurityManager::InitStatics() /builds/worker/workspace/build/src/caps/nsScriptSecurityManager.cpp:1406:30
    #25 0x7f7a4b528855 in nsXPConnect::InitStatics() /builds/worker/workspace/build/src/js/xpconnect/src/nsXPConnect.cpp:138:5
    #26 0x7f7a4b4c2c28 in xpcModuleCtor() /builds/worker/workspace/build/src/js/xpconnect/src/XPCModule.cpp:13:5
    #27 0x7f7a527e5517 in Initialize() /builds/worker/workspace/build/src/layout/build/nsLayoutModule.cpp:269:8
    #28 0x7f7a49b893f2 in Load /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:726:21
    #29 0x7f7a49b893f2 in nsFactoryEntry::GetFactory() /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:1748
    #30 0x7f7a49b8a21a in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:1046:41
    #31 0x7f7a49b8fcab in CallCreateInstance /builds/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp:149:38
    #32 0x7f7a49b8fcab in nsCreateInstanceByContractID::operator()(nsID const&, void**) const /builds/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp:197
    #33 0x7f7a49a4baed in nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) /builds/worker/workspace/build/src/xpcom/base/nsCOMPtr.cpp:128:7
    #34 0x7f7a49b728a1 in nsCOMPtr /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:605:5
    #35 0x7f7a49b728a1 in LogMessageWithContext(mozilla::FileLocation&, unsigned int, char const*, ...) /builds/worker/workspace/build/src/xpcom/components/ManifestParser.cpp:173
    #36 0x7f7a49b78a6e in nsComponentManagerImpl::ManifestContract(nsComponentManagerImpl::ManifestProcessingContext&, int, char* const*) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:658:5
    #37 0x7f7a49b75ecc in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool) /builds/worker/workspace/build/src/xpcom/components/ManifestParser.cpp:738:9
    #38 0x7f7a49b85eb9 in DoRegisterManifest /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:541:5
    #39 0x7f7a49b85eb9 in nsComponentManagerImpl::RegisterManifest(NSLocationType, mozilla::FileLocation&, bool) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:554
    #40 0x7f7a49b86177 in nsComponentManagerImpl::ManifestManifest(nsComponentManagerImpl::ManifestProcessingContext&, int, char* const*) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:563:3
    #41 0x7f7a49b75ecc in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool) /builds/worker/workspace/build/src/xpcom/components/ManifestParser.cpp:738:9
    #42 0x7f7a49b85eb9 in DoRegisterManifest /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:541:5
    #43 0x7f7a49b85eb9 in nsComponentManagerImpl::RegisterManifest(NSLocationType, mozilla::FileLocation&, bool) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:554
    #44 0x7f7a49b849e6 in nsComponentManagerImpl::RereadChromeManifests(bool) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:684:5
    #45 0x7f7a49b83271 in nsComponentManagerImpl::Init() /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:349:5
    #46 0x7f7a49c34e6e in NS_InitXPCOM2 /builds/worker/workspace/build/src/xpcom/build/XPCOMInit.cpp:664:51
    #47 0x7f7a559fad43 in XRE_InitEmbedding2(nsIFile*, nsIFile*, nsIDirectoryServiceProvider*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:190:8
    #48 0x7f7a4aad657e in mozilla::ipc::ScopedXREEmbed::Start() /builds/worker/workspace/build/src/ipc/glue/ScopedXREEmbed.cpp
    #49 0x7f7a50fc97c1 in mozilla::dom::ContentProcess::Init(int, char**) /builds/worker/workspace/build/src/dom/ipc/ContentProcess.cpp:223:13
    #50 0x7f7a559fb911 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:695:21
    #51 0x4f50dc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #52 0x4f50dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #53 0x7f7a69ac182f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
Flags: in-testsuite?
Assignee: nobody → dminor
Rank: 15
Priority: -- → P2
Comment on attachment 8966691 [details]
Bug 1453030 - Only create RTCDTMFSender on audio RTCRtpSender senders;

https://reviewboard.mozilla.org/r/235396/#review241088

Video senders probably shouldn't have a dtmf sender in the first place. It would be reasonable to put a MOZ_CRASH or MOZ_ASSERT in TransceiverImpl::InsertDTMFTone though. We probably need to make our modification in PeerConnection.js
Attachment #8966691 - Flags: review?(docfaraday) → review-
^ Does that sound right, jib?
Flags: needinfo?(jib)
It looks like the spec now has a canInsertDTMF attribute on the RTCDTMFSender which presumably should be false for video senders [1].

[1] https://w3c.github.io/webrtc-pc/#dom-rtcdtmfsender-caninsertdtmf
See Also: → 1376947
sender.dtmf MUST be null for video senders. From [1]:

  "4. Let sender have a [[Dtmf]] internal slot initialized to null.
   5. If sender is of kind "audio" then create an RTCDTMFSender dtmf and set the [[Dtmf]] internal slot to dtmf. "

The canInsertDTMF attribute is separate, and when it transitions is described in [2].

[1] https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
[2] https://github.com/w3c/webrtc-pc/issues/1619#issuecomment-366963505
Flags: needinfo?(jib)
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #7)
> sender.dtmf MUST be null for video senders. From [1]:
> 
>   "4. Let sender have a [[Dtmf]] internal slot initialized to null.
>    5. If sender is of kind "audio" then create an RTCDTMFSender dtmf and set
> the [[Dtmf]] internal slot to dtmf. "
> 
> The canInsertDTMF attribute is separate, and when it transitions is
> described in [2].
> 
> [1] https://w3c.github.io/webrtc-pc/#rtcrtpsender-interface
> [2] https://github.com/w3c/webrtc-pc/issues/1619#issuecomment-366963505

Thanks! I'm obviously not very good at searching the spec.
Comment on attachment 8966691 [details]
Bug 1453030 - Only create RTCDTMFSender on audio RTCRtpSender senders;

https://reviewboard.mozilla.org/r/235396/#review241402
Attachment #8966691 - Flags: review?(docfaraday) → review+
Comment on attachment 8966692 [details]
Bug 1453030 - Add crashtest;

https://reviewboard.mozilla.org/r/235398/#review241404

::: dom/media/tests/crashtests/1453030.html:9
(Diff revision 2)
> +      function getDTMF () {
> +        try { o4 = o2.dtmf } catch(e) { }
> +        try { o4.insertDTMF("1BC1D55", new Uint32Array([3538134876])[0], new Uint32Array([666182017])[0]) } catch (e) { }
> +        document.documentElement.removeAttribute("class");
> +      }
> +      

Nit: Extra whitespace.
Attachment #8966692 - Flags: review?(docfaraday) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b5119957214
Only create RTCDTMFSender on audio RTCRtpSender senders; r=bwc
https://hg.mozilla.org/integration/autoland/rev/0c41b40807b8
Add crashtest; r=bwc
https://hg.mozilla.org/mozilla-central/rev/8b5119957214
https://hg.mozilla.org/mozilla-central/rev/0c41b40807b8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is there a user impact here which warrants uplift consideration for 60? AFAICT, this crash doesn't appear to be hitting in the wild.
Crash Signature: [@ mozilla::WebrtcVideoConduit::ConfigureRecvMediaCodecs]
status-firefox59: --- → wontfix
status-firefox60: --- → fix-optional
status-firefox-esr52: --- → unaffected
Flags: needinfo?(dminor)
Flags: in-testsuite?
Flags: in-testsuite+
Comment on attachment 8966691 [details]
Bug 1453030 - Only create RTCDTMFSender on audio RTCRtpSender senders;

Approval Request Comment
[Feature/Bug causing the regression]:
The problematic code has existed since DTMF was implemented (Bug 1291715), but I don't think it caused crashes until the changes associated with Transceivers (Bug 1290948).
[User impact if declined]:
This allows a malicious website to crash Firefox by sending DTMF on a video track.
[Is this code covered by automated tests?]:
Yes.
[Has the fix been verified in Nightly?]:
Yes (by automated tests).
[Needs manual test from QE? If yes, steps to reproduce]: 
No.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
Low risk.
[Why is the change risky/not risky?]:
This should only impact DTMF on a video track, which is not something allowed by the spec anyway. I've done some quick tests (and run the automated tests) and DTMF on audio tracks seems to be working fine.
[String changes made/needed]:
None.
Flags: needinfo?(dminor)
Attachment #8966691 - Flags: approval-mozilla-beta?
Comment on attachment 8966691 [details]
Bug 1453030 - Only create RTCDTMFSender on audio RTCRtpSender senders;

Seems like this can ride to 61.
Attachment #8966691 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: