Bug 209641

Summary:

Nullptr crash in CompositeEditCommand::moveParagraphs when inserting OL into uneditable parent.

Product:

WebKit

Reporter:

Jack <shihchieh_lee>

Component:

HTML Editing

Assignee:

Jack <shihchieh_lee>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, ews-feeder, ews-watchlist, mifenton, product-security, rniwa, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none

Jack

Reported 2020-03-26 21:10:11 PDT

<rdar://60915598> 0 com.apple.WebCore 0x0000000113bfdf6d WTF::Optional<WebCore::BoundaryPoint>::operator*() && + 45 1 com.apple.WebCore 0x00000001162fb268 WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 4616 2 com.apple.WebCore 0x00000001163a7fc4 WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*) + 1860 3 com.apple.WebCore 0x00000001163a73e1 WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::Range*) + 2977 4 com.apple.WebCore 0x00000001163a6510 WebCore::InsertListCommand::doApply() + 2624 5 com.apple.WebCore 0x00000001162d7957 WebCore::CompositeEditCommand::apply() + 439 6 com.apple.WebCore 0x000000011638dd19 WebCore::executeInsertOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 201 7 com.apple.WebCore 0x0000000116007e88 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 248

Attachments
Patch (4.51 KB, patch) 2020-03-26 21:44 PDT, Jack	no flags	Details Formatted Diff Diff
Patch (4.80 KB, patch) 2020-03-26 23:09 PDT, Jack	no flags	Details Formatted Diff Diff
Patch for landing (4.77 KB, patch) 2020-03-27 10:36 PDT, Jack	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Jack

Comment 1 2020-03-26 21:23:08 PDT

In this case we try to insert ol at the position of li. Because li is originally in un-ordered list, we call unlistifyParagraph to move li into a BR element. Since BR needs to be inserted in body, but body is not editable, the insertion fails and leave BR parentless. The next call in moveParagraphs deref the parent of BR and crashed. </style> <script> window.onload = () => { document.getSelection().setPosition(LI); document.execCommand("insertOrderedList", false); } </script> <body><ul><li id=LI contenteditable="true">

Jack

Comment 2 2020-03-26 21:44:45 PDT

Created attachment 394702 [details] Patch

Jack

Comment 3 2020-03-26 21:56:11 PDT

This is a conservative patch. We are checking parentless node after insertion. In debug build, a lot of assertions for editable content trigger before and during insertion. I was wondering if checking parent of container node of the visible position for edibility is okay? Is it too aggressive and may find false positive? I roughly checked all the insertion calls and each of them eventually checks parent or ancestor for editability. Something like this: if (!start.deepEquivalent().containerNode()->hasEditableStyle() || !start.deepEquivalent().containerNode()->parentNode()->hasEditableStyle()) return;

Ryosuke Niwa

Comment 4 2020-03-26 22:06:28 PDT

(In reply to Jack from comment #3) > This is a conservative patch. We are checking parentless node after > insertion. In debug build, a lot of assertions for editable content trigger > before and during insertion. Hm... we probably need to fix that because we can't land a test which hits assertions in debug builds. > I was wondering if checking parent of container node of the visible position > for edibility is okay? Is it too aggressive and may find false positive? I > roughly checked all the insertion calls and each of them eventually checks > parent or ancestor for editability. > > Something like this: > > if (!start.deepEquivalent().containerNode()->hasEditableStyle() || > !start.deepEquivalent().containerNode()->parentNode()->hasEditableStyle()) > return; At where? That doesn't seem right if container node itself is editable. Why does the parent of the container node needs to be also editable?

Jack

Comment 5 2020-03-26 22:12:54 PDT

(In reply to Ryosuke Niwa from comment #4) > (In reply to Jack from comment #3) > > This is a conservative patch. We are checking parentless node after > > insertion. In debug build, a lot of assertions for editable content trigger > > before and during insertion. > > Hm... we probably need to fix that because we can't land a test which hits > assertions in debug builds. I see. > > if (!start.deepEquivalent().containerNode()->hasEditableStyle() || > > !start.deepEquivalent().containerNode()->parentNode()->hasEditableStyle()) > > return; > > At where? That doesn't seem right if container node itself is editable. Why > does the parent of the container node needs to be also editable? In the beginning of the function. It's because the function insertNodeAfter or insertNodeBefore eventually append or insert to the parent, for example: void CompositeEditCommand::insertNodeAfter(Ref<Node>&& insertChild, Node& refChild) { ContainerNode* parent = refChild.parentNode(); ... if (parent->lastChild() == &refChild) appendNode(WTFMove(insertChild), *parent); }

Jack

Comment 6 2020-03-26 22:17:21 PDT

And InsertNodeBefore also checks parent for editability. void InsertNodeBeforeCommand::doApply() { ContainerNode* parent = m_refChild->parentNode(); if (!parent || (m_shouldAssumeContentIsAlwaysEditable == DoNotAssumeContentIsAlwaysEditable && !isEditableNode(*parent))) return; ASSERT(isEditableNode(*parent)); parent->insertBefore(m_insertChild, m_refChild.ptr()); }

Ryosuke Niwa

Comment 7 2020-03-26 22:20:59 PDT

(In reply to Jack from comment #5) > (In reply to Ryosuke Niwa from comment #4) > > (In reply to Jack from comment #3) > > > This is a conservative patch. We are checking parentless node after > > > insertion. In debug build, a lot of assertions for editable content trigger > > > before and during insertion. > > > > Hm... we probably need to fix that because we can't land a test which hits > > assertions in debug builds. > I see. > > > > if (!start.deepEquivalent().containerNode()->hasEditableStyle() || > > > !start.deepEquivalent().containerNode()->parentNode()->hasEditableStyle()) > > > return; > > > > At where? That doesn't seem right if container node itself is editable. Why > > does the parent of the container node needs to be also editable? > In the beginning of the function. The beginning of which function? InsertListCommand::unlistifyParagraph? > It's because the function insertNodeAfter or insertNodeBefore eventually > append or insert to the parent, for example: In which function? It's unclear what code of which function you're talking about.

Jack

Comment 8 2020-03-26 22:24:31 PDT

> In which function? It's unclear what code of which function you're talking > about. Oh sorry, the changes are in unlistifyParagraph and listifyParagraph.

Jack

Comment 9 2020-03-26 23:09:28 PDT

Created attachment 394706 [details] Patch

Jack

Comment 10 2020-03-26 23:13:32 PDT

Thanks to Ryosuke for discussion. We use more aggressive editability check in unlistifyParagraph because the insertion position is always the listNode. Try this patch on EWS tests. (In reply to Jack from comment #9) > Created attachment 394706 [details] > Patch

Ryosuke Niwa

Comment 11 2020-03-26 23:58:51 PDT

Comment on attachment 394706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394706&action=review > Source/WebCore/editing/InsertListCommand.cpp:277 > + Nit: whitespace. > Source/WebCore/editing/InsertListCommand.cpp:280 > + Nit: whitespace. > Source/WebCore/editing/InsertListCommand.cpp:398 > + Nit: whitespace. > LayoutTests/editing/inserting/insert-ol-uneditable-parent.html:1 > +</style> Do we really need this?

Ryosuke Niwa

Comment 12 2020-03-26 23:59:59 PDT

There is no security implication here.

Jack

Comment 13 2020-03-27 10:36:16 PDT

Created attachment 394728 [details] Patch for landing

Jack

Comment 14 2020-03-27 10:38:12 PDT

Thanks for the good catches! It's in the original test case, somehow my eyes automatically skip that... :-) (In reply to Ryosuke Niwa from comment #11) > Comment on attachment 394706 [details]

EWS

Comment 15 2020-03-27 21:17:05 PDT

Committed r259153: <https://trac.webkit.org/changeset/259153> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394728 [details].

Note You need to log in before you can comment on or make changes to this bug.