Bug 221650 – Nullptr crash in ModifySelectionListLevelCommand::appendSiblingNodeRange

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Created attachment 419813 [details]
Test

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff3b1a22d9 WebCore::ModifySelectionListLevelCommand::appendSiblingNodeRange(WebCore::Node*, WebCore::Node*, WebCore::Element*) + 57
1   com.apple.WebCore             	0x00007fff3b1a24e1 WebCore::IncreaseSelectionListLevelCommand::doApply() + 337
2   com.apple.WebCore             	0x00007fff3b12e10f WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&) + 79
3   com.apple.WebCore             	0x00007fff3b196fbf WebCore::InsertNestedListCommand::doApply() + 1647
4   com.apple.WebCore             	0x00007fff39dcb684 WebCore::CompositeEditCommand::apply() + 500
5   com.apple.WebCore             	0x00007fff3b188462 WebCore::executeInsertNestedOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 98
6   com.apple.WebCore             	0x00007fff39e30f7d WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 77
7   com.apple.WebCore             	0x00007fff3a2324cc WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 428

<rdar://problem/74148852>

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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

[Ryosuke Niwa]

Comment on attachment 420807 [details]
Patch

Ok, seems reasonable.

[EWS]

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

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