Storing a Node in Ref/RefPtr inside its destructor results in double delete
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Mar 2019 21:09:46 +0000 (21:09 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Mar 2019 21:09:46 +0000 (21:09 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242964 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/dom/Node.cpp

index 72af018..5219ef7 100644 (file)
@@ -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  Brent Fulgham  <bfulgham@apple.com>
 
         Move CommonCrypto SPI declarations to an appropriate PAL/spi header
index 5f2909a..a0c2d51 100644 (file)
@@ -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;
     }
 }
index 6583446..f1dbd48 100644 (file)
@@ -376,6 +376,7 @@ public:
 #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;
         }
     }
index 8312d9a..7729179 100644 (file)
@@ -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);
 
@@ -2532,6 +2534,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;
 }