Changeset 274862 in webkit

Timestamp:

Mar 23, 2021, 6:36:58 AM (5 years ago)

Author:

commit-queue@webkit.org

Message:

Nullptr crash in HTMLConverter::convert
​https://bugs.webkit.org/show_bug.cgi?id=221719

Patch by Frédéric Wang <​fwang@igalia.com> on 2021-03-23
Reviewed by Ryosuke Niwa.

When the "Undo" command is called after DOM changes, one of the selection's position anchors
may have been moved to a new document. In that case, just clear the selection. Also add
asserts to ensure the selection is in good state after unapply and reapply commands.

• editing/CompositeEditCommand.cpp:

(WebCore::EditCommandComposition::unapply): Add security assert to ensure selection is in
good state.
(WebCore::EditCommandComposition::reapply): Ditto.

• editing/FrameSelection.cpp:

(WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance): If the selection's
position anchors have been moved to a new document then just clear the selection.
(WebCore::FrameSelection::isConnectedToDocument const): New method to verify that all the
positions of the visible selection are in m_document.

• editing/FrameSelection.h: Declare new method.
• editing/VisibleSelection.cpp:

(WebCore::VisibleSelection::document const): New method that returns a common document for
all positions or nullptr otherwise.

• editing/VisibleSelection.h: Declare new method.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• editing/CompositeEditCommand.cpp (modified) (2 diffs)
• editing/FrameSelection.cpp (modified) (2 diffs)
• editing/FrameSelection.h (modified) (1 diff)
• editing/VisibleSelection.cpp (modified) (1 diff)
• editing/VisibleSelection.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-03-23  Frédéric Wang  <fwang@igalia.com>
+
+        Nullptr crash in HTMLConverter::convert
+        https://bugs.webkit.org/show_bug.cgi?id=221719
+
+        Reviewed by Ryosuke Niwa.
+
+        When the "Undo" command is called after DOM changes, one of the selection's position anchors
+        may have been moved to a new document. In that case, just clear the selection. Also add
+        asserts to ensure the selection is in good state after unapply and reapply commands.
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::EditCommandComposition::unapply): Add security assert to ensure selection is in
+        good state.
+        (WebCore::EditCommandComposition::reapply): Ditto.
+        * editing/FrameSelection.cpp:
+        (WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance): If the selection's
+        position anchors have been moved to a new document then just clear the selection.
+        (WebCore::FrameSelection::isConnectedToDocument const): New method to verify that all the
+        positions of the visible selection are in m_document.
+        * editing/FrameSelection.h: Declare new method.
+        * editing/VisibleSelection.cpp:
+        (WebCore::VisibleSelection::document const): New method that returns a common document for
+        all positions or nullptr otherwise.
+        * editing/VisibleSelection.h: Declare new method.
+
 2021-03-23  Kimmo Kinnunen  <kkinnunen@apple.com>
 

trunk/Source/WebCore/editing/CompositeEditCommand.cpp

@@ -244 +244 @@
     if (AXObjectCache::accessibilityEnabled())
         m_replacedText.postTextStateChangeNotificationForUnapply(m_document->existingAXObjectCache());
+
+    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(m_document->selection().isNone() || m_document->selection().isConnectedToDocument());
 }
 
@@ -271 +273 @@
     if (AXObjectCache::accessibilityEnabled())
         m_replacedText.postTextStateChangeNotificationForReapply(m_document->existingAXObjectCache());
+
+    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(m_document->selection().isNone() || m_document->selection().isConnectedToDocument());
 }
 

trunk/Source/WebCore/editing/FrameSelection.cpp

@@ -370 +370 @@
         }
 
+        bool selectionEndpointsBelongToMultipleDocuments = newSelection.base().document() && !newSelection.document();
+        bool selectionIsInAnotherDocument = newSelection.document() && newSelection.document() != m_document.get();
+        if (selectionEndpointsBelongToMultipleDocuments || selectionIsInAnotherDocument) {
+            clear();
+            return false;
+        }
+
         if (closeTyping)
             TypingCommand::closeTyping(*m_document);
@@ -2795 +2802 @@
 }
 
+bool FrameSelection::isConnectedToDocument() const
+{
+    return selection().document() == m_document.get();
+}
+
 RefPtr<Range> FrameSelection::associatedLiveRange()
 {

trunk/Source/WebCore/editing/FrameSelection.h

@@ -253 +253 @@
 
     bool isInDocumentTree() const;
+    bool isConnectedToDocument() const;
+
     RefPtr<Range> associatedLiveRange();
     void associateLiveRange(Range&);

trunk/Source/WebCore/editing/VisibleSelection.cpp

@@ -143 +143 @@
         return true;
     return false;
+}
+
+RefPtr<Document> VisibleSelection::document() const
+{
+    auto baseDocument = makeRefPtr(m_base.document());
+    if (!baseDocument)
+        return nullptr;
+
+    if (m_extent.document() != baseDocument.get() || m_start.document() != baseDocument.get() || m_end.document() != baseDocument.get())
+        return nullptr;
+
+    if (baseDocument->settings().liveRangeSelectionEnabled() && (m_anchor.document() != baseDocument.get() || m_focus.document() != baseDocument.get()))
+        return nullptr;
+
+    return baseDocument;
 }
 

trunk/Source/WebCore/editing/VisibleSelection.h

@@ -88 +88 @@
     bool isNoneOrOrphaned() const { return isNone() || start().isOrphan() || end().isOrphan(); }
     bool isOrphan() const;
+    RefPtr<Document> document() const;
 
     bool isBaseFirst() const { return m_anchorIsFirst; }