Bug 221385 – Nullptr crash in Document::contextDocument() inside FontFaceSet::FontFaceSet

• Component: CSS
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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>

[Ryosuke Niwa]

Created attachment 419240 [details]
Test

[Carlos Garcia Campos]

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 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]

Created attachment 420110 [details]
Patch

[Ryosuke Niwa]

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]

By the way, can we add a test here? I don't think there is any security implication here.

[Carlos Garcia Campos]

(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]

Created attachment 420300 [details]
Patch

[Ryosuke Niwa]

(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 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 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]

Created attachment 420440 [details]
Patch

[Ryosuke Niwa]

Hold on a sec, I'm checking if there is any security implication related to this crash or not.

[Ryosuke Niwa]

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]

Created attachment 421291 [details]
Patch for landing

[EWS]

Committed r273374: <https://commits.webkit.org/r273374>

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