Bug 210016

Summary: Nullptr crash in WebCore::lastPositionInNode when indenting text node that has user-select:all parent.
Product: WebKit Reporter: Jack <shihchieh_lee>
Component: HTML EditingAssignee: Jack <shihchieh_lee>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, ews-feeder, ews-watchlist, mifenton, product-security, rniwa, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Jack
Reported 2020-04-04 21:52:27 PDT
<rdar://61014577> #0 0x3b81dea51 in WebCore::Node::getFlag(WebCore::Node::NodeFlags) const (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x1dca51) #1 0x3ba9fa7f4 in WebCore::lastPositionInNode(WebCore::Node*) (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x29f87f4) #2 0x3bb4f1edb in WebCore::ApplyBlockElementCommand::rangeForParagraphSplittingTextNodesIfNeeded(WebCore::VisiblePosition const&, WebCore::Position&, WebCore::Position&) (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34efedb) #3 0x3bb4f0a3e in WebCore::ApplyBlockElementCommand::formatSelection(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&) (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34eea3e) #4 0x3bb4efe83 in WebCore::ApplyBlockElementCommand::doApply() (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34ede83) #5 0x3bb4eeba6 in WebCore::CompositeEditCommand::apply() (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x34ecba6) #6 0x3bb5a4278 in WebCore::executeIndent(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) (Safari_ASAN_259163_6d5fb2d39898a6352d12792f8e6477a2373020ec.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x35a2278) #7 0x3bb21f127 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
Attachments
Patch (6.27 KB, patch)
2020-04-04 22:06 PDT, Jack 		no flags
DetailsFormatted DiffDiff
Patch (6.26 KB, patch)
2020-04-04 22:08 PDT, Jack 		no flags
DetailsFormatted DiffDiff
Patch (5.19 KB, patch)
2020-04-06 13:03 PDT, Jack 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Jack
Comment 1 2020-04-04 21:52:55 PDT
Root cause: In function rangeForParagraphSplittingTextNodesIfNeeded, the attemt to split text node fails, so there is no sibling node being created. However, the code assumes sibling exists and deref it in lastPositionInNode(). 1. In this test, formatSelection is called to indent all the elements and nodes in <body>. 2. To determine which paragraph to indent, fuction RangeForParagraphSplittingTextNodesIfNeeded is called. 3. For text node in pre element, because there is newline in it, fucntion splitTextNode is called to create a new text node for “\n”. 4. However, since <details> and <pre> has use-select set to all, which is considered uneditable, splitTextNode returns without creating a sibling text node. 5. Next we call functions firstPositionInOrBeforeNode and lastPositionInNode with the null sibling node and crashes. <style> #DETAILS { -webkit-user-select: all; } </style> <script> window.onload = () => { document.execCommand("selectAll", false); document.execCommand("indent", false); } </script> <body contentEditable="true"><br></br><details id=DETAILS open="true"><pre>a </pre></details><span>a</span> Node tree before indentation: BODY 0x60c0000814c0 (renderer 0x612000061cc0) BR 0x60c000081580 (renderer 0x6110000c2f00) BR 0x60c000081640 (renderer 0x6110000c3040) DETAILS 0x60e00005ce00 (renderer 0x612000061e40) #document-fragment 0x61200005a7c0 (renderer 0x0) (needs style recalc) (child needs style recalc) SLOT 0x60d000063e70 (renderer 0x0) SUMMARY 0x60c0000817c0 (renderer 0x612000062140) #document-fragment 0x61200005a940 (renderer 0x0) (needs style recalc) (child needs style recalc) DIV 0x60c000081880 (renderer 0x6120000622c0) SLOT 0x60d000063f40 (renderer 0x0) #text 0x60800004d220 "Details" SLOT 0x60d000064280 (renderer 0x0) PRE 0x60c000081b80 (renderer 0x612000062440) #text 0x60800004d2a0 "a\n" SPAN 0x60c000081c40 (renderer 0x6110000c3180) * #text 0x60800004d320 "a" #text 0x60800004d3a0 "\n" legacy, offset, offset:1 
Jack
Comment 2 2020-04-04 22:06:21 PDT
Created attachment 395480 [details] Patch
Jack
Comment 3 2020-04-04 22:08:04 PDT
Created attachment 395481 [details] Patch
Ryosuke Niwa
Comment 4 2020-04-06 12:31:17 PDT
Comment on attachment 395481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395481&action=review > Source/WebCore/editing/ApplyBlockElementCommand.cpp:247 > + if (endContainer->previousSibling()) { We prefer early exit over nested if's so this should check for endContainer not having the previous sibling. Also, we should probably check that the previous sibling is a text node. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:258 > + start = end = { }; Nit: Each statement should have its own line so should be: start = { }; end = { };
Ryosuke Niwa
Comment 5 2020-04-06 12:31:33 PDT
There is no security implication here.
Jack
Comment 6 2020-04-06 13:03:21 PDT
Created attachment 395604 [details] Patch
EWS
Comment 7 2020-04-06 18:46:01 PDT
Committed r259619: <https://trac.webkit.org/changeset/259619> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395604 [details].
Note You need to log in before you can comment on or make changes to this bug.