Bug 218494 – Release assertion failure in Optional<WebCore::SimpleRange>::operator* via CompositeEditCommand::moveParagraphs

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ian Gilbert]

Crash found by fuzzing. Reproduces on WebKit revision 265375.

Stack Trace
===========

0   com.apple.WebCore             	0x00000004de1ab37d WTF::Optional<WebCore::SimpleRange>::operator*() && + 45
1   com.apple.WebCore             	0x00000004e0c6d4c3 WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 2035
2   com.apple.WebCore             	0x00000004e0d1c837 WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*) + 1927
3   com.apple.WebCore             	0x00000004e0d1bc61 WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::SimpleRange&) + 2961
4   com.apple.WebCore             	0x00000004e0d1aa86 WebCore::InsertListCommand::doApply() + 1750
5   com.apple.WebCore             	0x00000004e0c4a6e7 WebCore::CompositeEditCommand::apply() + 535
6   com.apple.WebCore             	0x00000004e0d06949 WebCore::executeInsertOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 201
7   com.apple.WebCore             	0x00000004e0958a84 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 244
8   com.apple.WebCore             	0x00000004ddfe71ac WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 1164
9   com.apple.WebCore             	0x00000004ddf18a5c long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 252

[Radar WebKit Bug Importer]

<rdar://problem/70987390>

[Ian Gilbert]

Created attachment 413028 [details]
Crashing Input

[Carlos Garcia Campos]

Created attachment 413298 [details]
Reduced test case

[Carlos Garcia Campos]

The crash happens in CompositeEditCommand::moveParagraphs() because it's receiving a null endOfParagraphToMove. That causes that *makeSimpleRange(start, end) ends up trying to get a value that is actually nullopt. The problem comes from InsertListCommand::unlistifyParagraph(), that tries to get the start and end points of the list item children.

start = firstPositionInNode(listChildNode);
end = lastPositionInNode(listChildNode);

end is null here. There's no </li> so all nodes after the <li> are considered children of the list item:

						LI	0x7f2102132380 (renderer 0x7f2100000240)  CLASS=class1 STYLE=border-image: url(#htmlvar00007) 47% 1 -1 round; -webkit-ltr-ordering: visual; filter: brightness(1) brightness(0.361593078481); background-origin: content-box, padding-box; mso-width-alt: 1
							#text	0x7f2102138b58 "\n"
							VIDEO	0x7f2102132400 (renderer 0x7f2100000360) 
								#text	0x7f2102138bb0 "\n"
								BASE	0x7f2102132af0 (renderer (nil))  CLASS=class6
								#text	0x7f2102138c08 "\n"
								SOURCE	0x7f2102132b70 (renderer (nil))  STYLE=stroke-linecap: round; scroll-snap-destination: 44% 1px; box-flex-group: 0; -webkit-ruby-position: after; max-width: none
								#text	0x7f2102138c60 "EFgrVTq+XimoYYd+9$\n"
*							#text	0x7f2102138cb8 "\n"
							MARQUEE	0x7f2102132c50 (renderer 0x7f2102128f00) 
								#text	0x7f2102138d10 "\n"
								TIME	0x7f2102132ce0 (renderer 0x7f2100008f18) 
									#text	0x7f2102138d68 "\n"
									BODY	0x7f2100000650 (renderer 0x7f2102129000) 

The video one is the reason null is returned, but I don't know why yet. If I remove the video element then it doesn't crash, but the end point is still wrong, I assume it should be the text child with the offset = len.

[Carlos Garcia Campos]

Created attachment 413925 [details]
Patch

In the end I decided to stop the insertion when end position is null to match what firefox and chrome do.

[Carlos Garcia Campos]

It seems the test failure can just be rebaselined, for some reason there's one empty line less in the outpout now:

https://ews-build.s3-us-west-2.amazonaws.com/macOS-Mojave-Release-WK2-Tests-EWS/r413925-22814/editing/inserting/insert-list-in-iframe-in-list-pretty-diff.html

[Ryosuke Niwa]

Can we attach the reduced test case since there doesn't appear to be a security bug here?

[Ryosuke Niwa]

Comment on attachment 413925 [details]
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:301
> +    if (end.isNull())
> +        return false;

Why is end null here?

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #7)
> Can we attach the reduced test case since there doesn't appear to be a
> security bug here?

I already attached the reduced test case, I'll clean it up a bit and include it as part of the patch then.

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 413925 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413925&action=review
> 
> > Source/WebCore/editing/InsertListCommand.cpp:301
> > +    if (end.isNull())
> > +        return false;
> 
> Why is end null here?

See comment #4. There's an orphan <li> without the </li> causing the elements after that one to be considered children. The next element is a video one, that causes lastPositionInNode(listChildNode) to return null.

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #10)
> (In reply to Ryosuke Niwa from comment #8)
> > Comment on attachment 413925 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=413925&action=review
> > 
> > > Source/WebCore/editing/InsertListCommand.cpp:301
> > > +    if (end.isNull())
> > > +        return false;
> > 
> > Why is end null here?
> 
> See comment #4. There's an orphan <li> without the </li> causing the
> elements after that one to be considered children. The next element is a
> video one, that causes lastPositionInNode(listChildNode) to return null.

But why? We should be able to deal with the fact video is the last child of li. There is nothing wrong with that.

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #11)
> (In reply to Carlos Garcia Campos from comment #10)
> > (In reply to Ryosuke Niwa from comment #8)
> > > Comment on attachment 413925 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=413925&action=review
> > > 
> > > > Source/WebCore/editing/InsertListCommand.cpp:301
> > > > +    if (end.isNull())
> > > > +        return false;
> > > 
> > > Why is end null here?
> > 
> > See comment #4. There's an orphan <li> without the </li> causing the
> > elements after that one to be considered children. The next element is a
> > video one, that causes lastPositionInNode(listChildNode) to return null.
> 
> But why? We should be able to deal with the fact video is the last child of
> li. There is nothing wrong with that.

It's not the last child, there's also a marquee, leaving the video but removing the marquee doesn't crash either. Let me investigate more.

[Carlos Garcia Campos]

Created attachment 414011 [details]
Reduced test case

I've reduced more the test case and cleaned it up

[Ryosuke Niwa]

Maybe VisiblePosition is choking up? It would be good to get to the bottom of why it's getting null. That kind of bug can lead to crashes elsewhere in the editing code. We don't want to be keep chasing new crash elsewhere.

[Carlos Garcia Campos]

This is the new tree with the reduced test case:

BODY	0x7f3f118f52d0 (renderer 0x7f3f118f5460) 
	#text	0x7f3f118f6430 "\n"
	FORM	0x7f3f118f6490 (renderer 0x7f3f118f7070)  STYLE=-webkit-user-modify: read-write;
		#text	0x7f3f118f6590 "\n"
		DIV	0x7f3f118f65f0 (renderer 0x7f3f118f7180) 
		#text	0x7f3f118f6670 "\n"
		UL	0x7f3f1192c200 (renderer 0x7f3f11934200) 
			LI	0x7f3f118f66d0 (renderer 0x7f3f118f7290) 
				#text	0x7f3f118f6750 "\n"
				VIDEO	0x7f3f118f67b0 (renderer 0x7f3f118f73b0) 
*				#text	0x7f3f118f6ea0 "\n"
				MARQUEE	0x7f3f118f6f00 (renderer 0x7f3f11934300) 
					#text	0x7f3f118f6f90 "\n"
					TIME	0x7f3f118f6ff0 (renderer 0x7f3f118f7670) 
						#text	0x7f3f1193c058 "\n"
						BODY	0x7f3f118f78b0 (renderer 0x7f3f11934400) 

The last BODY, which is what the script adds, is what confuses everything. The tree is iterated looking for the end position and the last BODY returns true for isCandidate() but null is returned because !nextIsInSameEditableElement && !prevIsInSameEditableElement (the editing root for BODY is the body itself). And that seems to be the actual bug, because that body is not editable. Node::rootEditableElement() breaks ig the node is an HTMLBodyElment, but it shoudl actually check it's the document body element.

[Carlos Garcia Campos]

Created attachment 414037 [details]
Patch

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #15)
>
> The last BODY, which is what the script adds, is what confuses everything.
> The tree is iterated looking for the end position and the last BODY returns
> true for isCandidate() but null is returned because
> !nextIsInSameEditableElement && !prevIsInSameEditableElement (the editing
> root for BODY is the body itself). And that seems to be the actual bug,
> because that body is not editable. Node::rootEditableElement() breaks ig the
> node is an HTMLBodyElment, but it shoudl actually check it's the document
> body element.

Ah, I see. That makes sense. Thanks for the good analysis.

[Ryosuke Niwa]

Comment on attachment 414037 [details]
Patch

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

> LayoutTests/ChangeLog:7
> +

Please say that we're adding a regression test.

[Ryosuke Niwa]

Looks like there is some assertion failure on Mac WK1:

CRASHING TEST: editing/inserting/insert-list-in-iframe-in-list.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010d643af0 WTFCrash + 16 (Assertions.cpp:295)
1   com.apple.WebCore             	0x0000000126d4115b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x0000000129f06f75 WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 149
3   com.apple.WebCore             	0x0000000129f8ea56 WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*) + 2022
4   com.apple.WebCore             	0x0000000129f8e0ae WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::SimpleRange&) + 2014
5   com.apple.WebCore             	0x0000000129f8d881 WebCore::InsertListCommand::doApply() + 2385
6   com.apple.WebCore             	0x0000000129eef665 WebCore::CompositeEditCommand::apply() + 421
7   com.apple.WebCore             	0x0000000129f7a4ad WebCore::executeInsertOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 157
8   com.apple.WebCore             	0x0000000129f515b7 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 231
9   com.apple.WebCore             	0x0000000129c0fc46 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 86
10  com.apple.WebCore             	0x0000000127772e1e WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 990
11  com.apple.WebCore             	0x00000001277729f9 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 793
12  com.apple.WebCore             	0x00000001276d47c4 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 36
13  ???                           	0x000029dbf7401178 0 + 46024722747768

[Ryosuke Niwa]

Comment on attachment 414037 [details]
Patch

r- because of the aforementioned assertion failure. Please fix that.

[Carlos Garcia Campos]

Created attachment 414211 [details]
Patch

I've fixed the assert, end can be null, but only when start is null too.

[Ryosuke Niwa]

Comment on attachment 414211 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1403
> +    ASSERT(startOfParagraphToMove == endOfParagraphToMove || !endOfParagraphToMove.isNull());

This is harder to parse than (startOfParagraphToMove.isNull() && endOfParagraphToMove.isNull()) || endOfParagraphToMove.isNull()

[Carlos Garcia Campos]

Created attachment 414336 [details]
Patch for landing

[Ryosuke Niwa]

Are we waiting for something to land patch this patch??

[EWS]

Committed r269946: <https://trac.webkit.org/changeset/269946>

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