Changeset 243592 in webkit

Timestamp:

Mar 27, 2019, 4:52:44 PM (7 years ago)

Author:

Alan Coon

Message:

Cherry-pick r242964. rdar://problem/49359851

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

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

Location:

branches/safari-607-branch/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)

branches/safari-607-branch/Source/WebCore/ChangeLog

@@ +1 @@
+2019-03-27  Alan Coon  <alancoon@apple.com>
+
+        Cherry-pick r242964. rdar://problem/49359851
+
+    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):
+    
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242964 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    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-27  Alan Coon  <alancoon@apple.com>
 

branches/safari-607-branch/Source/WebCore/dom/Document.cpp

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

branches/safari-607-branch/Source/WebCore/dom/Document.h

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

branches/safari-607-branch/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);
@@ -2538 +2540 @@
     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;
 }