Nullptr deref in CompositeEditCommand::isRemovableBlock in DeleteSelectionCommand::removeRedundantBlocks

Ian Gilbert · webkit-commit-queue · commit 298b8d209b9d · 2021-04-17T00:57:21.000Z
https://bugs.webkit.org/show_bug.cgi?id=224518 Patch by Ian Gilbert <iang@apple.com> on 2021-04-16 Reviewed by Ryosuke Niwa. Source/WebCore: Add null check in case node is removed while iterating over tree. Test: editing/execCommand/remove-node-during-command-crash.html * editing/DeleteSelectionCommand.cpp: (WebCore::DeleteSelectionCommand::removeRedundantBlocks): LayoutTests: Add a regression test. * editing/execCommand/remove-node-during-command-crash-expected.txt: Added. * editing/execCommand/remove-node-during-command-crash.html: Added. Canonical link: https://commits.webkit.org/236668@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@276186 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2021-04-16  Ian Gilbert  <iang@apple.com>
+
+        Nullptr deref in CompositeEditCommand::isRemovableBlock in DeleteSelectionCommand::removeRedundantBlocks
+        https://bugs.webkit.org/show_bug.cgi?id=224518
+
+        Reviewed by Ryosuke Niwa.
+
+        Add a regression test.
+
+        * editing/execCommand/remove-node-during-command-crash-expected.txt: Added.
+        * editing/execCommand/remove-node-during-command-crash.html: Added.
+
 2021-04-16  Cameron McCormack  <heycam@apple.com>
 
         Place vertical scrollbars at (inline/block)-end edge in all writing modes.
diff --git a/LayoutTests/editing/execCommand/remove-node-during-command-crash-expected.txt b/LayoutTests/editing/execCommand/remove-node-during-command-crash-expected.txt
@@ -0,0 +1 @@
+Test passes if it does not crash PASS
diff --git a/LayoutTests/editing/execCommand/remove-node-during-command-crash.html b/LayoutTests/editing/execCommand/remove-node-during-command-crash.html
@@ -0,0 +1,27 @@
+<style>
+  br {
+    content: '';
+  }
+</style>
+<script>
+  onload = () => {
+      
+    if (window.testRunner)
+      testRunner.dumpAsText();
+        
+    document.designMode = 'on';
+    document.execCommand('SelectAll');
+    document.execCommand('InsertImage', false, '#');
+    let ifr0 = document.createElement('iframe');
+    document.body.appendChild(ifr0);
+    ifr0.onload = () => {
+      document.execCommand('JustifyRight');
+    };
+    document.execCommand('InsertParagraph');
+    getSelection().extend(document.body);
+    document.execCommand('InsertParagraph');
+
+    document.write('Test passes if it does not crash\n');
+    document.write('PASS');
+  };
+</script>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,17 @@
+2021-04-16  Ian Gilbert  <iang@apple.com>
+
+        Nullptr deref in CompositeEditCommand::isRemovableBlock in DeleteSelectionCommand::removeRedundantBlocks
+        https://bugs.webkit.org/show_bug.cgi?id=224518
+
+        Reviewed by Ryosuke Niwa.
+
+        Add null check in case node is removed while iterating over tree.
+
+        Test: editing/execCommand/remove-node-during-command-crash.html
+
+        * editing/DeleteSelectionCommand.cpp:
+        (WebCore::DeleteSelectionCommand::removeRedundantBlocks):
+
 2021-04-16  Cameron McCormack  <heycam@apple.com>
 
         Place vertical scrollbars at (inline/block)-end edge in all writing modes.
diff --git a/Source/WebCore/editing/DeleteSelectionCommand.cpp b/Source/WebCore/editing/DeleteSelectionCommand.cpp
@@ -865,11 +865,11 @@ String DeleteSelectionCommand::originalStringForAutocorrectionAtBeginningOfSelec
 // This method removes div elements with no attributes that have only one child or no children at all.
 void DeleteSelectionCommand::removeRedundantBlocks()
 {
-    Node* node = m_endingPosition.containerNode();
-    Node* rootNode = node->rootEditableElement();
+    auto node = makeRefPtr(m_endingPosition.containerNode());
+    auto rootNode = makeRefPtr(node->rootEditableElement());
    
-    while (node != rootNode) {
-        if (isRemovableBlock(node)) {
+    while (node && node != rootNode) {
+        if (isRemovableBlock(node.get())) {
             if (node == m_endingPosition.anchorNode())
                 updatePositionForNodeRemovalPreservingChildren(m_endingPosition, *node);
             
