Summary: | REGRESSION(r225868): Release assert when removing an SVGUseElement from Document::m_svgUseElements | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||
Component: | SVG | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, dbates, ddkilzer, esprehn+autocc, ews-watchlist, jonlee, kangil.han, koivisto, rniwa, simon.fraser, thorton, webkit-bug-importer, zimmermann | ||||||||||
Priority: | P2 | Keywords: | InRadar, Regression | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2018-01-26 14:48:41 PST
Created attachment 332456 [details]
Patch
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. Created attachment 332733 [details]
Patch
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. This is not a security bug. (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. This bug doesn't not address the root cause of the release assertion. Reopening to attach new patch. Created attachment 339285 [details]
Removes the release assert
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.
Created attachment 339286 [details]
Fixed typos
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.
Committed r231267: <https://trac.webkit.org/changeset/231267> |