| Summary: | Nullptr crash in LibWebRTCDTMFSenderBackend constructor via RTCRtpSender::dtmf | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
| Component: | WebRTC | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, product-security, rbuis, svillar, webkit-bug-importer, youennf | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ryosuke Niwa
2021-01-06 13:49:50 PST
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? (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! (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. Created attachment 418851 [details]
Patch
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. Created attachment 418852 [details]
Patch
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. 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.
commit-queue failed to commit attachment 418852 [details] to WebKit repository.
Created attachment 418970 [details]
Patch
Committed r272198: <https://trac.webkit.org/changeset/272198> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418970 [details]. |