Title: [244006] releases/WebKitGTK/webkit-2.24/Source/WebCore
Revision
244006
Author
carlo...@webkit.org
Date
2019-04-08 05:39:00 -0700 (Mon, 08 Apr 2019)

Log Message

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

Modified Paths

Diff

Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog (244005 => 244006)


--- releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog	2019-04-08 12:38:56 UTC (rev 244005)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog	2019-04-08 12:39:00 UTC (rev 244006)
@@ -1,3 +1,27 @@
+2019-03-14  Ryosuke Niwa  <rn...@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  <za...@apple.com>
 
         Cleanup inline boxes when list marker gets blockified

Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Document.cpp (244005 => 244006)


--- releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Document.cpp	2019-04-08 12:38:56 UTC (rev 244005)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Document.cpp	2019-04-08 12:39:00 UTC (rev 244006)
@@ -720,6 +720,7 @@
         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;
     }
 }

Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Document.h (244005 => 244006)


--- releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Document.h	2019-04-08 12:38:56 UTC (rev 244005)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Document.h	2019-04-08 12:39:00 UTC (rev 244006)
@@ -370,6 +370,7 @@
 #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;
         }
     }

Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Node.cpp (244005 => 244006)


--- releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Node.cpp	2019-04-08 12:38:56 UTC (rev 244005)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/dom/Node.cpp	2019-04-08 12:39:00 UTC (rev 244006)
@@ -332,7 +332,9 @@
 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 @@
 #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;
 }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to