Bug 220382 – Nullptr crash in LibWebRTCDTMFSenderBackend constructor via RTCRtpSender::dtmf

• Component: WebRTC
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 417125 [details]
Test

ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000118596695 bp 0x7ffeecea1dd0 sp 0x7ffeecea1da0 T0)

    #0 0x118596695 in WebCore::LibWebRTCDTMFSenderBackend::LibWebRTCDTMFSenderBackend(rtc::scoped_refptr<webrtc::DtmfSenderInterface>&&)+0xb5 (WebCore.framework/Versions/A/WebCore:x86_64+0x514a695)
    #1 0x1185967c8 in WebCore::LibWebRTCDTMFSenderBackend::LibWebRTCDTMFSenderBackend(rtc::scoped_refptr<webrtc::DtmfSenderInterface>&&)+0x8 (WebCore.framework/Versions/A/WebCore:x86_64+0x514a7c8)
    #2 0x1136ccc2f in std::__1::__unique_if<WebCore::LibWebRTCDTMFSenderBackend>::__unique_single std::__1::make_unique<WebCore::LibWebRTCDTMFSenderBackend, rtc::scoped_refptr<webrtc::DtmfSenderInterface> >(rtc::scoped_refptr<webrtc::DtmfSenderInterface>&&)+0x2f (WebCore.framework/Versions/A/WebCore:x86_64+0x280c2f)
    #3 0x1136cbd86 in WebCore::LibWebRTCRtpSenderBackend::createDTMFBackend()+0x106 (WebCore.framework/Versions/A/WebCore:x86_64+0x27fd86)
    #4 0x115ccaf22 in WebCore::RTCRtpSender::dtmf()+0x192 (WebCore.framework/Versions/A/WebCore:x86_64+0x287ef22)
    #5 0x1149a043d in WebCore::jsRTCRtpSender_dtmfGetter(JSC::JSGlobalObject&, WebCore::JSRTCRtpSender&)+0x10d (WebCore.framework/Versions/A/WebCore:x86_64+0x155443d)
    #6 0x1148e92cc in long long WebCore::IDLAttribute<WebCore::JSRTCRtpSender>::get<&(WebCore::jsRTCRtpSender_dtmfGetter(JSC::JSGlobalObject&, WebCore::JSRTCRtpSender&)), (WebCore::CastedThisErrorBehavior)3>(JSC::JSGlobalObject&, long long, char const*)+0x2c (WebCore.framework/Versions/A/WebCore:x86_64+0x149d2cc)
    #7 0x1148e9298 in WebCore::jsRTCRtpSender_dtmf(JSC::JSGlobalObject*, long long, JSC::PropertyName)+0x8 (WebCore.framework/Versions/A/WebCore:x86_64+0x149d298)
    #8 0x135abcc56 in JSC::PropertySlot::customGetter(JSC::JSGlobalObject*, JSC::PropertyName) const+0x2a6 (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x2f60c56)
    #9 0x135243531 in JSC::LLInt::performLLIntGetByID(JSC::Instruction const*, JSC::CodeBlock*, JSC::JSGlobalObject*, JSC::JSValue, JSC::Identifier const&, JSC::GetByIdModeMetadata&)+0xe91 (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26e7531)
    #10 0x135242451 in llint_slow_path_get_by_id+0x211 (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26e6451)
    #11 0x1337797e8 in llint_entry+0x94c0 (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xc1d7e8)
    #12 0x133770128 in vmEntryToJavaScript+0xd7 (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xc14128)
    #13 0x134f52893 in JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*)+0x7193 (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x23f6893)
    #14 0x1356c7ebe in JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)+0x21e (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x2b6bebe)
    #15 0x1356c8177 in JSC::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)+0xe7 (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x2b6c177)
    #16 0x11656a7a9 in WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)+0xd9 (WebCore.framework/Versions/A/WebCore:x86_64+0x311e7a9)
    #17 0x116569fb9 in WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)+0x2e9 (WebCore.framework/Versions/A/WebCore:x86_64+0x311dfb9)
    #18 0x116569bad in WebCore::ScriptController::evaluateInWorldIgnoringException(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)+0xed (WebCore.framework/Versions/A/WebCore:x86_64+0x311dbad)
    #19 0x11656a9af in WebCore::ScriptController::evaluateIgnoringException(WebCore::ScriptSourceCode const&)+0x1f (WebCore.framework/Versions/A/WebCore:x86_64+0x311e9af)
    #20 0x116e4720c in WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)+0x3bc (WebCore.framework/Versions/A/WebCore:x86_64+0x39fb20c)
    #21 0x116e43e5e in WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport)+0xb0e (WebCore.framework/Versions/A/WebCore:x86_64+0x39f7e5e)
    #22 0x1175a8ec6 in WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&)+0x206 (WebCore.framework/Versions/A/WebCore:x86_64+0x415cec6)
    #23 0x1175a8b94 in WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::RawPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&)+0x84 (WebCore.framework/Versions/A/WebCore:x86_64+0x415cb94)
    #24 0x11757f662 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()+0x3f2 (WebCore.framework/Versions/A/WebCore:x86_64+0x4133662)
    #25 0x11757fd27 in WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&)+0x367 (WebCore.framework/Versions/A/WebCore:x86_64+0x4133d27)
    #26 0x11757ec4e in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)+0x18e (WebCore.framework/Versions/A/WebCore:x86_64+0x4132c4e)
    #27 0x11757e815 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)+0x65 (WebCore.framework/Versions/A/WebCore:x86_64+0x4132815)
    #28 0x117580d72 in WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl, WTF::RawPtrTraits<WTF::StringImpl>, WTF::DefaultRefDerefTraits<WTF::StringImpl> >&&)+0x332 (WebCore.framework/Versions/A/WebCore:x86_64+0x4134d72)
    #29 0x116bfb1ff in WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&)+0x14f (WebCore.framework/Versions/A/WebCore:x86_64+0x37af1ff)
    #30 0x117aff77b in WebCore::DocumentWriter::end()+0x14b (WebCore.framework/Versions/A/WebCore:x86_64+0x46b377b)
    #31 0x117ab018c in WebCore::DocumentLoader::finishedLoading()+0x2dc (WebCore.framework/Versions/A/WebCore:x86_64+0x466418c)
    #32 0x117aafb09 in WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&, WebCore::NetworkLoadMetrics const&)+0x2c9 (WebCore.framework/Versions/A/WebCore:x86_64+0x4663b09)
    #33 0x117c84b6f in WebCore::CachedResource::checkNotify(WebCore::NetworkLoadMetrics const&)+0x17f (WebCore.framework/Versions/A/WebCore:x86_64+0x4838b6f)
    #34 0x117c7f1be in WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)+0x4e (WebCore.framework/Versions/A/WebCore:x86_64+0x48331be)
    #35 0x117c80a78 in WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)+0x258 (WebCore.framework/Versions/A/WebCore:x86_64+0x4834a78)
    #36 0x117bf5162 in WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&)+0x732 (WebCore.framework/Versions/A/WebCore:x86_64+0x47a9162)
    #37 0x107e5dc66 in WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&)+0x286 (WebKit.framework/Versions/A/WebKit:x86_64+0x20d1c66)
    #38 0x108537121 in void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul>)+0x61 (WebKit.framework/Versions/A/WebKit:x86_64+0x27ab121)
    #39 0x1085370a8 in void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WebCore::NetworkLoadMetrics>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))+0x28 (WebKit.framework/Versions/A/WebKit:x86_64+0x27ab0a8)
    #40 0x108534b16 in void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))+0x146 (WebKit.framework/Versions/A/WebKit:x86_64+0x27a8b16)
    #41 0x108534123 in WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&)+0x1a3 (WebKit.framework/Versions/A/WebKit:x86_64+0x27a8123)
    #42 0x107e49c0a in WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)+0xfa (WebKit.framework/Versions/A/WebKit:x86_64+0x20bdc0a)
    #43 0x105e2fb93 in IPC::Connection::dispatchMessage(IPC::Decoder&)+0x293 (WebKit.framework/Versions/A/WebKit:x86_64+0xa3b93)
    #44 0x105e31507 in IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)+0x167 (WebKit.framework/Versions/A/WebKit:x86_64+0xa5507)
    #45 0x105e32026 in IPC::Connection::dispatchOneIncomingMessage()+0x196 (WebKit.framework/Versions/A/WebKit:x86_64+0xa6026)
    #46 0x105e505f5 in IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8::operator()()+0x35 (WebKit.framework/Versions/A/WebKit:x86_64+0xc45f5)
    #47 0x105e5055c in WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8, void>::call()+0xc (WebKit.framework/Versions/A/WebKit:x86_64+0xc455c)
    #48 0x132b945ce in WTF::Function<void ()>::operator()() const+0x3e (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x385ce)
    #49 0x132c2da28 in WTF::RunLoop::performWork()+0x228 (JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xd1a28)

<rdar://problem/71712462>

[youenn fablet]

This crash should not happen with normal peer connection.


We could update MockRtpSender to return a mock dtmf.
Or we could add a test in LibWebRTCRtpSenderBackend::createDTMFBackend().
But I do not see real value in doing so: AudioRtpSender::GetDtmfSender() is expected to return a valid dtmf sender.

Ryosuke, how did you hit this crash?

[Ryosuke Niwa]

(In reply to youenn fablet from comment #1)
> This crash should not happen with normal peer connection.
> 
> 
> We could update MockRtpSender to return a mock dtmf.
> Or we could add a test in LibWebRTCRtpSenderBackend::createDTMFBackend().
> But I do not see real value in doing so: AudioRtpSender::GetDtmfSender() is
> expected to return a valid dtmf sender.
> 
> Ryosuke, how did you hit this crash?

There is a test attached!

[youenn fablet]

(In reply to Ryosuke Niwa from comment #2)
> (In reply to youenn fablet from comment #1)
> > This crash should not happen with normal peer connection.
> > 
> > 
> > We could update MockRtpSender to return a mock dtmf.
> > Or we could add a test in LibWebRTCRtpSenderBackend::createDTMFBackend().
> > But I do not see real value in doing so: AudioRtpSender::GetDtmfSender() is
> > expected to return a valid dtmf sender.
> > 
> > Ryosuke, how did you hit this crash?
> 
> There is a test attached!

Right, but the test is enabling the mock connection backend.
It would not be fine to have this crash with a real connection.

The mock connection backend is not supporting DTMF and I think it is fine to consider this is configuration unsupported.
I can fix it if this is causing issues with fuzzers for instance.

[Rob Buis]

Created attachment 418851 [details]
Patch

[youenn fablet]

Comment on attachment 418851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418851&action=review

> Source/WebCore/ChangeLog:8
> +        Provide mock dtmf sender.

Patch seems fine but I wonder what the end goal is.
We do not get any additional code coverage/functionality coverage for this patch.
If the issue is for fuzzers to stop crashing, that seems fine.

> Source/WebCore/testing/MockLibWebRTCPeerConnection.h:170
> +class MockDtmfSender : public webrtc::DtmfSenderInterface {

final.

> Source/WebCore/testing/MockLibWebRTCPeerConnection.h:179
> +    int inter_tone_gap() const override { return 50; }

final an probably private as well.

> Source/WebCore/testing/MockLibWebRTCPeerConnection.h:209
> +    rtc::scoped_refptr<webrtc::DtmfSenderInterface> m_dtmfSender;

mutable to remove the const_cast.

[Rob Buis]

Created attachment 418852 [details]
Patch

[Rob Buis]

Comment on attachment 418851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418851&action=review

>> Source/WebCore/ChangeLog:8
>> +        Provide mock dtmf sender.
> 
> Patch seems fine but I wonder what the end goal is.
> We do not get any additional code coverage/functionality coverage for this patch.
> If the issue is for fuzzers to stop crashing, that seems fine.

AFAIK it is indeed for buzzers to stop crashing. Ryosuke will know more.

>> Source/WebCore/testing/MockLibWebRTCPeerConnection.h:170
>> +class MockDtmfSender : public webrtc::DtmfSenderInterface {
> 
> final.

Is it really needed?

>> Source/WebCore/testing/MockLibWebRTCPeerConnection.h:179
>> +    int inter_tone_gap() const override { return 50; }
> 
> final an probably private as well.

Done.

>> Source/WebCore/testing/MockLibWebRTCPeerConnection.h:209
>> +    rtc::scoped_refptr<webrtc::DtmfSenderInterface> m_dtmfSender;
> 
> mutable to remove the const_cast.

Done.

[youenn fablet]

Comment on attachment 418852 [details]
Patch

Let's go then.
If the issue is fuzzers crashing, I think they should not try to enable mock PC backends.

[EWS]

commit-queue failed to commit attachment 418852 [details] to WebKit repository.

[Rob Buis]

Created attachment 418970 [details]
Patch

[EWS]

Committed r272198: <https://trac.webkit.org/changeset/272198>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418970 [details].