Summary: | Nullptr crash in ModifySelectionListLevelCommand::appendSiblingNodeRange | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | HTML Editing | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, product-security, rbuis, svillar, webkit-bug-importer, wenson_hsieh | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2021-02-09 21:24:08 PST
Created attachment 420807 [details] Patch So the issue here is that ModifySelectionListLevelCommand::appendSiblingNodeRange browses the list of children from the startNode to the endNode, assuming they are siblings in that order. But will do a null pointer dereference if that's assumption is false: https://webkit-search.igalia.com/webkit/rev/96373eab5c9a5d968cdfac1e0c0069773d717c23/Source/WebCore/editing/ModifySelectionListLevel.cpp#123 Debugging backwards (see output below), the mistake is happening in getStartEndListChildren where the end node is moved to the next sibling in the render tree, which does not correspond to a sibling in the DOM tree (here, because the SLOT element does not generate any renderer). It seems the crash in ModifySelectionListLevelCommand::appendSiblingNodeRange is not a security bug (just a nullptr dereference) so I'm adding the tests to the patch for now and can change if you disagree. I haven't checked whether the mistake in getStartEndListChildren can cause similar issues, but that function is used for both list level increment and decrement. (rr) p showTree(startListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) * LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $1 = void (rr) p showTree(endListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) * LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $2 = void (rr) p showRenderTree(r) (B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout B---YGLS- -- RenderView at (0,0) size 800x600 renderer->(0x7f76f41f5240) B-----LS- -- HTML RenderBlock at (0,0) size 800x600 renderer->(0x7f76f41f56a0) node->(0x7f76f41f7a60) B-------- -- BODY RenderBody at (8,8) size 784x576 renderer->(0x7f76f41f57c0) node->(0x7f76f41f7d30) (layout overflow -1,0 785x576) (visual overflow -1,0 785x576) B-------- -- LI RenderListItem at (0,0) size 784x18 renderer->(0x7f76f41e8190) node->(0x7f76f41f7ec0) (layout overflow -1,0 785x18) (visual overflow -1,0 785x18) -------- -- Line: (top: 0 bottom: 17) with leading (top: 0 bottom: 18) -------- -- RootInlineBox at (0,0) size 14x17 (0x7f76f41e8560) renderer->(0x7f76f41e8190) -------- -- InlineBox at (-1,0) size 7x17 (0x7f76f41e8510) renderer->(0x7f76f41e82d0) I---YG--- -- RenderListMarker at (-1,0) size 7x17 renderer->(0x7f76f41e82d0) B-------- -- LI RenderListItem at (0,18) size 784x18 renderer->(0x7f76f41e8890) node->(0x7f76f41e8800) (layout overflow -1,0 785x18) (visual overflow -1,0 785x18) -------- -- Line: (top: 0 bottom: 17) with leading (top: 0 bottom: 18) -------- -- RootInlineBox at (0,0) size 14x17 (0x7f76f41e8b40) renderer->(0x7f76f41e8890) -------- -- InlineBox at (-1,0) size 7x17 (0x7f76f41e8af0) renderer->(0x7f76f41e89d0) I---YG--- -- RenderListMarker at (-1,0) size 7x17 renderer->(0x7f76f41e89d0) B-------- --* OL RenderBlock at (0,52) size 784x0 renderer->(0x7f76f41e83f0) node->(0x7f76f41e8080) $3 = void (rr) n 84 endListChild = r->node(); (rr) n 87 start = startListChild; (rr) p showTree(endListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" * OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $4 = void (In reply to Frédéric Wang (:fredw) from comment #1) > Debugging backwards (see output below), the mistake is happening in > getStartEndListChildren where the end node is moved to the next sibling in > the render tree, which does not correspond to a sibling in the DOM tree > (here, because the SLOT element does not generate any renderer). Additional remark here. The slot element has "display: contents" https://webkit-search.igalia.com/webkit/rev/6c8fdfee755dcaf35bd338726b21a7527d3917de/Source/WebCore/editing/markup.cpp#575 so this is really another issue with editing & display: contents. Comment on attachment 420807 [details]
Patch
Ok, seems reasonable.
Committed r273302: <https://commits.webkit.org/r273302> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420807 [details]. |