Changeset 242964 in webkit

Timestamp:

Mar 14, 2019, 2:09:46 PM (7 years ago)

Author:

rniwa@webkit.org

Message:

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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (1 diff)
• dom/Document.h (modified) (1 diff)
• dom/Node.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -721 +721 @@
         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;
     }

trunk/Source/WebCore/dom/Document.h

@@ -377 +377 @@
             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;
         }

trunk/Source/WebCore/dom/Node.cpp

@@ -333 +333 @@
 {
     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);
@@ -2533 +2535 @@
     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;
 }