Merge r242964 - Storing a Node in Ref/RefPtr inside its destructor re…

…sults in double delete https://bugs.webkit.org/show_bug.cgi?id=195661 Reviewed by Brent Fulgham. Set Node::m_refCount to 1 before calling its virtual destructor. This is a security mitigation to prevent any code which ends up storing the node to Ref / RefPtr inside the destructor, which is a programming error caught by debug assertions, from triggering a double-delete on the same Node. Such a code would hit the debug assertions in Node::deref() because m_inRemovedLastRefFunction had been set to true by then. * dom/Document.cpp: (WebCore::Document::removedLastRef): * dom/Document.h: (WebCore::Document::decrementReferencingNodeCount): * dom/Node.cpp: (WebCore::Node::~Node): (WebCore::Node::removedLastRef):
WebKit · Apr 8, 2019 · 7f1db1b · 7f1db1b
1 parent 0089d8a
commit 7f1db1b
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 1 deletion.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,27 @@
+2019-03-14  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Storing a Node in Ref/RefPtr inside its destructor results in double delete
+        https://bugs.webkit.org/show_bug.cgi?id=195661
+
+        Reviewed by Brent Fulgham.
+
+        Set Node::m_refCount to 1 before calling its virtual destructor.
+
+        This is a security mitigation to prevent any code which ends up storing the node to Ref / RefPtr
+        inside the destructor, which is a programming error caught by debug assertions, from triggering
+        a double-delete on the same Node.
+
+        Such a code would hit the debug assertions in Node::deref() because m_inRemovedLastRefFunction
+        had been set to true by then.
+
+        * dom/Document.cpp:
+        (WebCore::Document::removedLastRef):
+        * dom/Document.h:
+        (WebCore::Document::decrementReferencingNodeCount):
+        * dom/Node.cpp:
+        (WebCore::Node::~Node):
+        (WebCore::Node::removedLastRef):
+
 2019-03-14  Zalan Bujtas  <zalan@apple.com>
 
         Cleanup inline boxes when list marker gets blockified

diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
@@ -720,6 +720,7 @@ void Document::removedLastRef()
         m_inRemovedLastRefFunction = false;
         m_deletionHasBegun = true;
 #endif
+        m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
         delete this;
     }
 }

diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
@@ -370,6 +370,7 @@ class Document
 #if !ASSERT_DISABLED
             m_deletionHasBegun = true;
 #endif
+            m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
             delete this;
         }
     }

diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
@@ -332,7 +332,9 @@ Node::Node(Document& document, ConstructionType type)
 Node::~Node()
 {
     ASSERT(isMainThread());
-    ASSERT(!m_refCount);
+    // We set m_refCount to 1 before calling delete to avoid double destruction through use of Ref<T>/RefPtr<T>.
+    // This is a security mitigation in case of programmer errorm (caught by a debug assertion).
+    ASSERT(m_refCount == 1);
     ASSERT(m_deletionHasBegun);
     ASSERT(!m_adoptionIsRequired);
 
@@ -2523,6 +2525,7 @@ void Node::removedLastRef()
 #ifndef NDEBUG
     m_deletionHasBegun = true;
 #endif
+    m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
     delete this;
 }