Summary: | Nullptr crash in WebCore::canHaveChildrenForEditing via CompositeEditCommand::insertNode | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jack <shihchieh_lee> | ||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ajuma, bfulgham, commit-queue, dougk, ews-feeder, ews-watchlist, mifenton, product-security, rniwa, webkit-bug-importer, wenson_hsieh | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Local Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Jack
2020-02-23 15:02:10 PST
Root cause: In this case, hr is uneditable, and the script tries to insert an ol after hr, so the code is creating a BR element and inserting it after HR. However, since hr is uneditable, the insertion fails, making the BR element parentless. and that results in an null insertionPos, and it is later referenced in canHaveChildrenForEditing. <style> body { -webkit-user-modify: read-write; background-image: url(); } </style> <script> onload = function fun() { document.getSelection().setPosition(HR); HR.appendChild(document.createElement("option")); document.execCommand("insertOrderedList", false); } </script> <body><hr id=HR contenteditable="false"></hr> In debug build, it would trigger two assertions: 1. CompositeEditCommand::insertNodeAt ASSERT(isEditablePosition(editingPosition)); 2. InsertNodeBeforeCommand::InsertNodeBeforeCommand ASSERT(m_refChild->parentNode()->hasEditableStyle() || !m_refChild->parentNode()->renderer()); Created attachment 391591 [details]
Patch
Comment on attachment 391591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391591&action=review > Source/WebCore/editing/InsertListCommand.cpp:341 > - if (start.isNull() || end.isNull()) > + if (start.isNull() || end.isNull() || !start.deepEquivalent().deprecatedNode()->hasEditableStyle()) Please check the eatability of start.deepEquivalent().containerNode() instead. This is not a security bug. Thanks! Yes, in this case the anchorType is PositionIsOffsetInAnchor, so container node is also m_anchorNode. (lldb) p anchorType() (WebCore::Position::AnchorType) $3 = PositionIsOffsetInAnchor (In reply to Ryosuke Niwa from comment #4) > Comment on attachment 391591 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391591&action=review > > > Source/WebCore/editing/InsertListCommand.cpp:341 > > - if (start.isNull() || end.isNull()) > > + if (start.isNull() || end.isNull() || !start.deepEquivalent().deprecatedNode()->hasEditableStyle()) > > Please check the eatability of start.deepEquivalent().containerNode() > instead. Created attachment 391660 [details]
Patch
Hey Jack, I just happened to notice unlistify can crash in much the same way -- does it make sense to apply the check at the level of doApply() instead? doApply() already has an early-abort for if content is not editable: if (endingSelection().isNoneOrOrphaned() || !endingSelection().isContentRichlyEditable()) return; Comment on attachment 391660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391660&action=review > Source/WebCore/editing/InsertListCommand.cpp:341 > - if (start.isNull() || end.isNull()) > + if (start.isNull() || end.isNull() || !start.deepEquivalent().containerNode()->hasEditableStyle()) Sorry, I forgot to point out earlier but we should probably check that the end is also editable. Thanks, Doug. It is possible. However, the actual inserting position is determined later in the call stack (depending on the tag and its children and some other properties) and can change in other cases, so it can be false positives if the code returns too early. In this test case, at doApply() the endingSelection has anchor type = PositionIsBeforeAnchor, so the editability is checked on the parent of anchor node, which is <body> hence it does not return here. When it enters listifyParagraph and after startOfParagraph is called, the anchor type becomes PositionIsOffsetInAnchor so the editability is checked on the anchor node, <hr>, which is uneditable. One experiment I did was removing the child (option) of the uneditable element (hr). Even though the anchor node is the uneditable element, the code does continue and inserts ol in front of hr. It sort of makes sense since if there is nothing to be "listified" inside <hr>, it is not considered "editing" <hr>? (In reply to Doug Kelly from comment #8) > Hey Jack, > > I just happened to notice unlistify can crash in much the same way -- does > it make sense to apply the check at the level of doApply() instead? > > doApply() already has an early-abort for if content is not editable: > > if (endingSelection().isNoneOrOrphaned() || > !endingSelection().isContentRichlyEditable()) > return; Thanks! I thought about it but wondered if it is okay to listify contents if only the end is uneditable. Definitely it is more important to prevent crash! (In reply to Ryosuke Niwa from comment #9) > Comment on attachment 391660 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391660&action=review > > > Source/WebCore/editing/InsertListCommand.cpp:341 > > - if (start.isNull() || end.isNull()) > > + if (start.isNull() || end.isNull() || !start.deepEquivalent().containerNode()->hasEditableStyle()) > > Sorry, I forgot to point out earlier but we should probably check that the > end is also editable. Created attachment 391723 [details]
Patch
Comment on attachment 391723 [details] Patch Clearing flags on attachment: 391723 Committed r257536: <https://trac.webkit.org/changeset/257536> All reviewed patches have been landed. Closing bug. *** Bug 208515 has been marked as a duplicate of this bug. *** |