Summary: | Nullptr crash in Page::libWebRTCProvider via RTCPeerConnection::generateCertificate | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
Component: | WebRTC | Assignee: | Rob Buis <rbuis> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bfulgham, cgarcia, eric.carlson, ews-feeder, ews-watchlist, fred.wang, glenn, gpoo, hta, jer.noble, philipj, product-security, rbuis, sergio, svillar, tommyw, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2021-06-08 17:11:39 PDT
Reproduced with non-ASAN release build of WebKitTestRunner at r278627. Created attachment 430946 [details]
Patch
Comment on attachment 430946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430946&action=review > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:612 > + promise.reject(InvalidStateError); I would go with something like: auto* page = document.page(); if (!page) { promise.reject(InvalidStateError); return; } LibWebRTCCertificateGenerator::generateCertificate(...); > LayoutTests/ChangeLog:8 > + Add test for this. Comment not really needed > LayoutTests/webrtc/RTCPeerConnection-generateCertificate-crash.html:7 > + await RTCPeerConnection.generateCertificate({ We can probably have a simpler test using an iframe that we detached before calling generateCertificate, something like: promise_test(async (t) => { const iframe = await with_iframe('/'); //requires http. const pc = iframe.contentWindow.RTCPeerConnection; iframe.remove(); return promise_rejects(... pc.generateCertificate...) }); Created attachment 430949 [details]
Patch
Comment on attachment 430946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430946&action=review >> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:612 >> + promise.reject(InvalidStateError); > > I would go with something like: > auto* page = document.page(); > if (!page) { > promise.reject(InvalidStateError); > return; > } > LibWebRTCCertificateGenerator::generateCertificate(...); Sure, done. >> LayoutTests/ChangeLog:8 >> + Add test for this. > > Comment not really needed Removed. >> LayoutTests/webrtc/RTCPeerConnection-generateCertificate-crash.html:7 >> + await RTCPeerConnection.generateCertificate({ > > We can probably have a simpler test using an iframe that we detached before calling generateCertificate, something like: > > promise_test(async (t) => { > const iframe = await with_iframe('/'); //requires http. > const pc = iframe.contentWindow.RTCPeerConnection; > iframe.remove(); > return promise_rejects(... pc.generateCertificate...) > }); Nice! Done. Comment on attachment 430949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430949&action=review > LayoutTests/http/wpt/webrtc/RTCPeerConnection-generateCertificate-crash.html:31 > + return promise_rejects(pc.generateCertificate({ name: 'ECDSA', namedCurve: 'P-256'})); promise_rejects_js is directly available in testharness.js Comment on attachment 430949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430949&action=review >> LayoutTests/http/wpt/webrtc/RTCPeerConnection-generateCertificate-crash.html:31 >> + return promise_rejects(pc.generateCertificate({ name: 'ECDSA', namedCurve: 'P-256'})); > > promise_rejects_js is directly available in testharness.js Sadly that does not work as InvalidStateError is not exposed to JS (like TypeError etc.). ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Created attachment 430954 [details]
Patch
Found 1 new test failure: imported/w3c/web-platform-tests/navigation-timing/nav2_test_attributes_values.html Created attachment 431045 [details]
Patch
Committed r278692 (238666@main): <https://commits.webkit.org/238666@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431045 [details]. |