Bug 221385

Summary: Nullptr crash in Document::contextDocument() inside FontFaceSet::FontFaceSet
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, cgarcia, ews-feeder, fred.wang, gpoo, mmaxfield, product-security, rbuis, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test
none
Patch
none
Patch
none
Patch
none
Patch
rniwa: review+
Patch for landing none

Ryosuke Niwa
Reported 2021-02-03 23:16:04 PST
e.g. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00007fff3af505ca WebCore::FontFaceSet::FontFaceSet(WebCore::Document&, WebCore::CSSFontFaceSet&) + 90 1 com.apple.WebCore 0x00007fff3af5027e WebCore::FontFaceSet::create(WebCore::Document&, WebCore::CSSFontFaceSet&) + 190 2 com.apple.WebCore 0x00007fff3af03be5 WebCore::CSSFontSelector::fontFaceSet() + 53 3 com.apple.WebCore 0x00007fff3a21ca1e WebCore::jsDocumentFonts(JSC::JSGlobalObject*, long long, JSC::PropertyName) + 110 4 com.apple.JavaScriptCore 0x00007fff3747d381 JSC::LLInt::performLLIntGetByID(JSC::Instruction const*, JSC::CodeBlock*, JSC::JSGlobalObject*, JSC::JSValue, JSC::Identifier const&, JSC::GetByIdModeMetadata&) + 1681 5 com.apple.JavaScriptCore 0x00007fff36b592bb llint_slow_path_get_by_id + 347 6 com.apple.JavaScriptCore 0x00007fff36d64c3a llint_entry + 36118 7 com.apple.JavaScriptCore 0x00007fff36d5bd2f vmEntryToJavaScript + 216 8 com.apple.JavaScriptCore 0x00007fff373992ea JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*) + 12426 9 com.apple.JavaScriptCore 0x00007fff375edce5 JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 245 10 com.apple.WebCore 0x00007fff3ade1ac4 WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 84 11 com.apple.WebCore 0x00007fff3ade17f2 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) + 178 12 com.apple.WebCore 0x00007fff3ade1b48 WebCore::ScriptController::evaluateIgnoringException(WebCore::ScriptSourceCode const&) + 40 13 com.apple.WebCore 0x00007fff3b0d1060 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&) + 528 14 com.apple.WebCore 0x00007fff39ca3551 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 4369 15 com.apple.WebCore 0x00007fff3b3adcb9 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&) + 425 16 com.apple.WebCore 0x00007fff39c4ac35 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 2597 17 com.apple.WebCore 0x00007fff3b3a4336 WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >&&) + 1494 18 com.apple.WebCore 0x00007fff3b02f534 WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&) + 196 19 com.apple.WebCore 0x00007fff3b509eed WebCore::DocumentLoader::finishedLoading() + 669 20 com.apple.WebCore 0x00007fff3b59fe6f WebCore::CachedResource::checkNotify(WebCore::NetworkLoadMetrics const&) + 95 21 com.apple.WebCore 0x00007fff3b59deed WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&) + 1117 22 com.apple.WebCore 0x00007fff3b56c8dc WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) + 1148 23 com.apple.WebKit 0x00007fff3d279fd5 WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&) + 227 24 com.apple.WebKit 0x00007fff3d3ca2da WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) + 310 25 com.apple.WebKit 0x00007fff3cda1370 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 696 26 com.apple.WebKit 0x00007fff3cda3ddc WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7, void>::call() + 214 27 com.apple.JavaScriptCore 0x00007fff379c6abd WTF::RunLoop::performWork() + 573 <rdar://problem/73661979>
Attachments
Test (297 bytes, text/html)
2021-02-03 23:16 PST, Ryosuke Niwa
no flags
Patch (1.23 KB, patch)
2021-02-11 06:41 PST, Carlos Garcia Campos
no flags
Patch (4.61 KB, patch)
2021-02-12 02:25 PST, Carlos Garcia Campos
no flags
Patch (5.24 KB, patch)
2021-02-15 03:18 PST, Carlos Garcia Campos
no flags
Patch (7.07 KB, patch)
2021-02-16 02:09 PST, Carlos Garcia Campos
rniwa: review+
Patch for landing (7.12 KB, patch)
2021-02-23 01:24 PST, Carlos Garcia Campos
no flags
Ryosuke Niwa
Comment 1 2021-02-03 23:16:23 PST
Carlos Garcia Campos
Comment 2 2021-02-11 06:41:11 PST
Created attachment 419983 [details] Patch This was introduced in r201799. The problem is that we are clearing the CSSFontSelector document when document's last reference is removed but still refeenced by other nodes. Since CSSFontSelector::fontFaceSet() hasn't been called before, it will try to create a FontFaceSet::create() with a nullptr document. This should assert in FontFaceSet::create() for debug builds. So, I think we should not clear the document in that case.
Ryosuke Niwa
Comment 3 2021-02-11 12:32:27 PST
Comment on attachment 419983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419983&action=review > Source/WebCore/dom/Document.cpp:-809 > - m_fontSelector->clearDocument(); I don't think this is quite right. We probably still want to stop font loading timer, etc...
Carlos Garcia Campos
Comment 4 2021-02-12 02:25:30 PST
Ryosuke Niwa
Comment 5 2021-02-12 13:56:37 PST
Comment on attachment 420110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420110&action=review > Source/WebCore/css/CSSFontSelector.cpp:357 > +void CSSFontSelector::clearDocument() > +{ It seems that this function is no longer needed? We can just call detach in ~CSSFontSelector. There is no need to call this function in ~Document since m_document is a WeakPtr.
Ryosuke Niwa
Comment 6 2021-02-12 23:02:08 PST
By the way, can we add a test here? I don't think there is any security implication here.
Carlos Garcia Campos
Comment 7 2021-02-15 02:35:13 PST
(In reply to Ryosuke Niwa from comment #6) > By the way, can we add a test here? I don't think there is any security > implication here. Isn't this a use after free? We end up calling the Document::contextDocument() on an already freed Document.
Carlos Garcia Campos
Comment 8 2021-02-15 03:18:14 PST
Ryosuke Niwa
Comment 9 2021-02-15 14:29:32 PST
(In reply to Carlos Garcia Campos from comment #7) > (In reply to Ryosuke Niwa from comment #6) > > By the way, can we add a test here? I don't think there is any security > > implication here. > > Isn't this a use after free? We end up calling the > Document::contextDocument() on an already freed Document. How could that be? The bug here is that we're clearing m_document in CSSFontSelector too early, and m_document is WeakPtr<Document>.
Ryosuke Niwa
Comment 10 2021-02-15 14:32:36 PST
Comment on attachment 420300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420300&action=review > Source/WebCore/css/CSSFontSelector.cpp:340 > -void CSSFontSelector::clearDocument() > +void CSSFontSelector::detach() We're not really detaching from anything, are we? This is about freeing stuff & stopping timers so that document may be delete later. I'd call something like stopLoadingForDocumentTeardown or something. > Source/WebCore/dom/Document.cpp:-762 > - m_fontSelector->unregisterForInvalidationCallbacks(*this); Why are we removing the call to unregisterForInvalidationCallbacks?
Carlos Garcia Campos
Comment 11 2021-02-16 01:45:02 PST
Comment on attachment 420300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420300&action=review >> Source/WebCore/css/CSSFontSelector.cpp:340 >> +void CSSFontSelector::detach() > > We're not really detaching from anything, are we? > This is about freeing stuff & stopping timers so that document may be delete later. > I'd call something like stopLoadingForDocumentTeardown or something. I used detach for consistency with other similar functions, but I agree we are not detaching from the document because we still need it. I'll find a better name. >> Source/WebCore/dom/Document.cpp:-762 >> - m_fontSelector->unregisterForInvalidationCallbacks(*this); > > Why are we removing the call to unregisterForInvalidationCallbacks? Because clearDocument clears all the clients, so this is always called on an empty m_clients.
Carlos Garcia Campos
Comment 12 2021-02-16 02:09:37 PST
Ryosuke Niwa
Comment 13 2021-02-16 16:19:48 PST
Hold on a sec, I'm checking if there is any security implication related to this crash or not.
Ryosuke Niwa
Comment 14 2021-02-22 18:42:55 PST
Comment on attachment 420440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420440&action=review > Source/WebCore/css/CSSFontSelector.cpp:340 > -void CSSFontSelector::clearDocument() > +void CSSFontSelector::clear() clear is a very generic term. I would have preferred calling it something like stopLoading.
Carlos Garcia Campos
Comment 15 2021-02-23 01:24:51 PST
Created attachment 421291 [details] Patch for landing
EWS
Comment 16 2021-02-24 00:42:23 PST
Committed r273374: <https://commits.webkit.org/r273374> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421291 [details].
Note You need to log in before you can comment on or make changes to this bug.