Bug 182188 – REGRESSION(r225868): Release assert when removing an SVGUseElement from Document::m_svgUseElements

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Said Abou-Hallawa]

The SVGUseElement is already removed from the set Document::m_svgUseElements and we are trying to remove it again. Here is the call stack:

0   WebCore                       	0x000000018e9a8720 WebCore::Document::removeSVGUseElement(WebCore::SVGUseElement&) + 44 (Document.cpp:5287)
1   WebCore                       	0x000000018e9a8710 WebCore::Document::removeSVGUseElement(WebCore::SVGUseElement&) + 28 (Document.cpp:5286)
2   WebCore                       	0x000000018f1936a4 WebCore::SVGUseElement::updateShadowTree() + 124 (SVGUseElement.cpp:241)
3   WebCore                       	0x000000018e99854c WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 192 (Document.cpp:1799)
4   WebCore                       	0x000000018ddd5554 WebCore::Document::updateStyleIfNeeded() + 252 (Document.cpp:1965)
5   WebCore                       	0x000000018ddd10a0 WebCore::Document::finishedParsing() + 260 (Document.cpp:5390)
6   WebCore                       	0x000000018ddcfc40 WebCore::HTMLDocumentParser::prepareToStopParsing() + 172 (HTMLDocumentParser.cpp:406)
7   WebCore                       	0x000000018ddcfb54 WebCore::HTMLDocumentParser::finish() + 224 (HTMLDocumentParser.cpp:427)
8   WebCore                       	0x000000018ddcf370 WebCore::DocumentWriter::end() + 92 (DocumentWriter.cpp:276)
9   WebCore                       	0x000000018ec7aa58 WebCore::DocumentLoader::finishedLoading() + 472 (DocumentLoader.cpp:429)
10  WebCore                       	0x000000018de0111c WebCore::CachedResource::checkNotify() + 324 (CachedResource.cpp:348)
11  WebCore                       	0x000000018ecdd3bc WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 364 (CachedResource.cpp:364)
12  WebCore                       	0x000000018ecb9a40 WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) + 520 (SubresourceLoader.cpp:589)
13  WebKit                        	0x00000001957bb53c WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&) + 220 (WebResourceLoader.cpp:150)
14  WebKit                        	0x00000001957bc2b0 void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)) + 92 (HandleMessage.h:40)
15  WebKit                        	0x00000001955c85dc WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 416 (NetworkProcessConnection.cpp:69)
16  WebKit                        	0x000000019553dc38 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 164 (Connection.cpp:901)
17  WebKit                        	0x0000000195540628 IPC::Connection::dispatchOneMessage() + 232 (Connection.cpp:959)
18  JavaScriptCore                	0x000000018d27290c WTF::RunLoop::performWork() + 348 (Function.h:56)
19  JavaScriptCore                	0x000000018d272b7c WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38)
20  CoreFoundation                	0x00000001853e09f4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24 (CFRunLoop.c:1982)
21  CoreFoundation                	0x00000001853e021c __CFRunLoopDoSources0 + 276 (CFRunLoop.c:2017)
22  CoreFoundation                	0x00000001853ddd8c __CFRunLoopRun + 1204 (CFRunLoop.c:2920)
23  CoreFoundation                	0x00000001852fe498 CFRunLoopRunSpecific + 552 (CFRunLoop.c:3245)
24  Foundation                    	0x0000000185d740e4 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 304 (NSRunLoop.m:367)
25  Foundation                    	0x0000000185dc5d4c -[NSRunLoop(NSRunLoop) run] + 88 (NSRunLoop.m:389)
26  libxpc.dylib                  	0x00000001850aabdc _xpc_objc_main + 516 (main.m:167)
27  libxpc.dylib                  	0x00000001850accb0 xpc_main + 180 (init.c:1476)
28  com.apple.WebKit.WebContent   	0x00000001008db5ac main + 380 (XPCServiceMain.mm:148)
29  libdyld.dylib                 	0x0000000184d9dfc0 start + 4

[Said Abou-Hallawa]

<rdar://problem/36689240>

[Said Abou-Hallawa]

Created attachment 332456 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 332456 [details]
Patch

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

r- because I don't think this approach is desirable.

> Source/WebCore/ChangeLog:30
> +        An assertion will fire if Document::resolveStyle() calls updateShadowTree()
> +        for an SVGUseElement and as a side effect it causes removedFromAncestor()
> +        to be called for an element which is also exist in Document::m_svgUseElements
> +        and has not been processed yet.

Sounds like we should be able to make a test case for this.

> Source/WebCore/ChangeLog:33
> +        The fix is to replace SVGUseElement::m_shadowTreeNeedsUpdate by a function
> +        which checks whether the element is in Document::m_svgUseElements. By doing

This will make the removal of a SVG element a lot more expensive. I don't think we should do that.

> Source/WebCore/ChangeLog:65
> +        (WebCore::SVGUseElement::updateShadowTree): No need to check isConnected().
> +        here. This function is called only from Document::resolveStyle(). Make the
> +        call to removeShadowTreeNeedsUpdate() happens the last thing to ensure
> +        no invalidate-update loops will happen.

What is a no invalidate-update loop?

> Source/WebCore/dom/Document.cpp:1801
> +            if (m_invalidatedSVGUseElements.contains(element.get()))
> +                element->updateShadowTree();

A better fix would be for this code to be checking isConnected() & shadowTreeNeedsUpdate() on each element before calling updateShadowTree().

> Source/WebCore/dom/Document.cpp:5288
> -    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(result.isNewEntry);
> +    auto result = m_invalidatedSVGUseElements.add(&element);
> +    ASSERT_UNUSED(result, result.isNewEntry);

Replacing a release assert by a debug assert would only pamper over a real issue.

> Source/WebCore/dom/Document.cpp:5294
> +    bool didRemove = m_invalidatedSVGUseElements.remove(&element);
> +    ASSERT_UNUSED(didRemove, didRemove);

Ditto.

[Said Abou-Hallawa]

Created attachment 332733 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 332456 [details]
Patch

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

What exactly is not desirable in this patch? Do you consider using HashSet<>::contains() to be a lot more expensive than accessing a data member?

>> Source/WebCore/ChangeLog:30
>> +        and has not been processed yet.
> 
> Sounds like we should be able to make a test case for this.

I could not find a test case for this crash.

>> Source/WebCore/ChangeLog:33
>> +        which checks whether the element is in Document::m_svgUseElements. By doing
> 
> This will make the removal of a SVG element a lot more expensive. I don't think we should do that.

Checking a HashSet should be O(1). and SVGDocumentExtensions has many HashMaps for animations, resource loading and SVGUseElement dependency graph.

>> Source/WebCore/ChangeLog:65
>> +        no invalidate-update loops will happen.
> 
> What is a no invalidate-update loop?

I meant if updating an element causes invalidating itself, this will cause an infinite loop of invalidate and update.

>> Source/WebCore/dom/Document.cpp:1801
>> +                element->updateShadowTree();
> 
> A better fix would be for this code to be checking isConnected() & shadowTreeNeedsUpdate() on each element before calling updateShadowTree().

I added if (!shadowTreeNeedsUpdate()) at the beginning of SVGUseElement::updateShadowTree() to be the opposite of SVGUseElement::invalidateShadowTree() which starts by returning if (!isConnected() || shadowTreeNeedsUpdate()).

>> Source/WebCore/dom/Document.cpp:5288
>> +    ASSERT_UNUSED(result, result.isNewEntry);
> 
> Replacing a release assert by a debug assert would only pamper over a real issue.

I replaced this debug assertions with release assertions as they we were before.

[Ryosuke Niwa]

This is not a security bug.

[Ryosuke Niwa]

(In reply to Said Abou-Hallawa from comment #5)
> Comment on attachment 332456 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332456&action=review
> 
> What exactly is not desirable in this patch? Do you consider using
> HashSet<>::contains() to be a lot more expensive than accessing a data
> member?

Yes. It's considerably more expensive. Also, rewriting the way hash map works completely isn't a way to address this crash we don't even have a test case for.

We really need a more thorough analysis of what might be happening instead of rewriting the entire thing.

[Ryosuke Niwa]

This bug doesn't not address the root cause of the release assertion.

[Ryosuke Niwa]

Reopening to attach new patch.

[Ryosuke Niwa]

Created attachment 339285 [details]
Removes the release assert

[EWS Watchlist]

Attachment 339285 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:23:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 339286 [details]
Fixed typos

[EWS Watchlist]

Attachment 339286 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:22:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Committed r231267: <https://trac.webkit.org/changeset/231267>