From: Nikita Popov <nikita.ppv@gmail.com>
Date: Thu, 1 Aug 2019 10:55:39 +0000 (+0200)
Subject: Fixed bug #72530
X-Git-Tag: php-7.4.0beta4~20
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=60a7e60b61b8e4a3d455974c83f76a26546ce117;p=php

Fixed bug #72530

For objects with destructors, we will now only call the destructor
in the initial GC run, and remove any nested data. The object is
marked purple so it will be considered a root for the next GC run,
at which point it will be fully destroyed, if possible.

GC counts change on a number of tests, as the objects now get
destroyed later.
---

diff --git a/NEWS b/NEWS
index 1254024d36..aac83845b5 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,7 @@ PHP                                                                        NEWS
     (Nikita)
   . Fixed bug #78406 (Broken file includes with user-defined stream filters).
     (Nikita)
+  . Fixed bug #72530 (Use After Free in GC with Certain Destructors). (Nikita)
 
 - Date:
   . Fixed bug #78383 (Casting a DateTime to array no longer returns its
diff --git a/Zend/tests/bug71818.phpt b/Zend/tests/bug71818.phpt
index e09255ddac..67094c09f7 100644
--- a/Zend/tests/bug71818.phpt
+++ b/Zend/tests/bug71818.phpt
@@ -19,7 +19,7 @@ class MemoryLeak
     private $things = [];
 }
 
-ini_set('memory_limit', '10M');
+ini_set('memory_limit', '20M');
 
 for ($i = 0; $i < 100000; ++$i) {
     $obj = new MemoryLeak();
diff --git a/Zend/tests/bug72530.phpt b/Zend/tests/bug72530.phpt
new file mode 100644
index 0000000000..b70be0b98f
--- /dev/null
+++ b/Zend/tests/bug72530.phpt
@@ -0,0 +1,31 @@
+--TEST--
+Bug #72530: Use After Free in GC with Certain Destructors
+--FILE--
+<?php
+
+class ryat {
+    var $ryat;
+    var $chtg;
+
+    function __destruct() {
+        $this->chtg = $this->ryat;
+        $this->ryat = 1;
+    }
+}
+
+$o = new ryat;
+$o->ryat = $o;
+$x =& $o->chtg;
+
+unset($o);
+gc_collect_cycles();
+var_dump($x);
+
+?>
+--EXPECT--
+object(ryat)#1 (2) {
+  ["ryat"]=>
+  int(1)
+  ["chtg"]=>
+  *RECURSION*
+}
diff --git a/Zend/tests/gc_011.phpt b/Zend/tests/gc_011.phpt
index 9c4cc2cc0e..d11d7b6b46 100644
--- a/Zend/tests/gc_011.phpt
+++ b/Zend/tests/gc_011.phpt
@@ -15,6 +15,7 @@ $a->a = $a;
 var_dump($a);
 unset($a);
 var_dump(gc_collect_cycles());
+var_dump(gc_collect_cycles());
 echo "ok\n"
 ?>
 --EXPECTF--
@@ -23,5 +24,6 @@ object(Foo)#%d (1) {
   *RECURSION*
 }
 __destruct
+int(0)
 int(1)
 ok
diff --git a/Zend/tests/gc_016.phpt b/Zend/tests/gc_016.phpt
index f082d60973..211f03a605 100644
--- a/Zend/tests/gc_016.phpt
+++ b/Zend/tests/gc_016.phpt
@@ -23,6 +23,6 @@ echo "ok\n"
 ?>
 --EXPECT--
 -> int(0)
-int(1)
-int(1)
+int(0)
+int(2)
 ok
diff --git a/Zend/tests/gc_017.phpt b/Zend/tests/gc_017.phpt
index 102c2b6bcb..55f381992e 100644
--- a/Zend/tests/gc_017.phpt
+++ b/Zend/tests/gc_017.phpt
@@ -32,11 +32,13 @@ unset($a);
 unset($b);
 unset($c);
 var_dump(gc_collect_cycles());
+var_dump(gc_collect_cycles());
 echo "ok\n"
 ?>
 --EXPECTF--
 string(1) "%s"
 string(1) "%s"
 string(1) "%s"
-int(4)
+int(0)
+int(1)
 ok
diff --git a/Zend/tests/gc_028.phpt b/Zend/tests/gc_028.phpt
index 8dc70fc397..fb2ea92c91 100644
--- a/Zend/tests/gc_028.phpt
+++ b/Zend/tests/gc_028.phpt
@@ -28,6 +28,8 @@ $bar->foo = $foo;
 unset($foo);
 unset($bar);
 var_dump(gc_collect_cycles());
+var_dump(gc_collect_cycles());
 ?>
 --EXPECT--
-int(2)
+int(0)
+int(1)
diff --git a/Zend/tests/gc_029.phpt b/Zend/tests/gc_029.phpt
index 215d0e0e3b..89c55e5ba7 100644
--- a/Zend/tests/gc_029.phpt
+++ b/Zend/tests/gc_029.phpt
@@ -30,6 +30,8 @@ $bar->foo = $foo;
 unset($foo);
 unset($bar);
 var_dump(gc_collect_cycles());
+var_dump(gc_collect_cycles());
 ?>
---EXPECTREGEX--
-int\([23]\)
+--EXPECT--
+int(0)
+int(1)
diff --git a/Zend/tests/gc_035.phpt b/Zend/tests/gc_035.phpt
index 177c3101f9..187af9108b 100644
--- a/Zend/tests/gc_035.phpt
+++ b/Zend/tests/gc_035.phpt
@@ -22,5 +22,5 @@ var_dump(gc_collect_cycles());
 var_dump(gc_collect_cycles());
 --EXPECT--
 int(0)
-int(2)
 int(0)
+int(2)
diff --git a/Zend/tests/generators/bug76427.phpt b/Zend/tests/generators/bug76427.phpt
index 09ec61a340..53851b0f35 100644
--- a/Zend/tests/generators/bug76427.phpt
+++ b/Zend/tests/generators/bug76427.phpt
@@ -21,4 +21,4 @@ var_dump(gc_collect_cycles());
 
 ?>
 --EXPECT--
-int(4)
+int(2)
diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c
index e1c2295d74..12922a26c8 100644
--- a/Zend/zend_gc.c
+++ b/Zend/zend_gc.c
@@ -141,6 +141,7 @@
 #define GC_ROOT    0x0 /* possible root of circular garbage     */
 #define GC_UNUSED  0x1 /* part of linked list of unused buffers */
 #define GC_GARBAGE 0x2 /* garbage to delete                     */
+#define GC_DTOR_GARBAGE 0x3 /* garbage on which only the dtor should be invoked */
 
 #define GC_GET_PTR(ptr) \
 	((void*)(((uintptr_t)(ptr)) & ~GC_BITS))
@@ -151,9 +152,13 @@
 	((((uintptr_t)(ptr)) & GC_BITS) == GC_UNUSED)
 #define GC_IS_GARBAGE(ptr) \
 	((((uintptr_t)(ptr)) & GC_BITS) == GC_GARBAGE)
+#define GC_IS_DTOR_GARBAGE(ptr) \
+	((((uintptr_t)(ptr)) & GC_BITS) == GC_DTOR_GARBAGE)
 
 #define GC_MAKE_GARBAGE(ptr) \
 	((void*)(((uintptr_t)(ptr)) | GC_GARBAGE))
+#define GC_MAKE_DTOR_GARBAGE(ptr) \
+	((void*)(((uintptr_t)(ptr)) | GC_DTOR_GARBAGE))
 
 /* GC address conversion */
 #define GC_IDX2PTR(idx)      (GC_G(buf) + (idx))
@@ -1328,9 +1333,6 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe
 tail_call:
 	do {
 		if (root) {
-			GC_TRACE_REF(ref, "removing from buffer");
-			gc_remove_from_roots(root);
-			GC_REF_SET_INFO(ref, 0);
 			root = NULL;
 			count++;
 		} else if (GC_REF_ADDRESS(ref) != 0
@@ -1461,67 +1463,79 @@ ZEND_API int zend_gc_collect_cycles(void)
 		end = GC_G(first_unused);
 
 		if (gc_flags & GC_HAS_DESTRUCTORS) {
-			uint32_t *refcounts;
-
 			GC_TRACE("Calling destructors");
 
-			// TODO: may be use emalloc() ???
-			refcounts = pemalloc(sizeof(uint32_t) * end, 1);
-
-			/* Remember reference counters before calling destructors */
+			/* During a destructor call, new externally visible references to nested data may
+			 * be introduced. These references can be introduced in a way that does not
+			 * modify any refcounts, so we have no real way to detect this situation
+			 * short of rerunning full GC tracing. What we do instead is to only run
+			 * destructors at this point, and leave the actual freeing of the objects
+			 * until the next GC run. */
+
+			/* Mark all roots for which a dtor will be invoked as DTOR_GARBAGE. Additionally
+			 * color them purple. This serves a double purpose: First, they should be
+			 * considered new potential roots for the next GC run. Second, it will prevent
+			 * their removal from the root buffer by nested data removal. */
 			idx = GC_FIRST_ROOT;
 			current = GC_IDX2PTR(GC_FIRST_ROOT);
 			while (idx != end) {
 				if (GC_IS_GARBAGE(current->ref)) {
 					p = GC_GET_PTR(current->ref);
-					refcounts[idx] = GC_REFCOUNT(p);
+					if (GC_TYPE(p) == IS_OBJECT && !(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) {
+						zend_object *obj = (zend_object *) p;
+						if (obj->handlers->dtor_obj != zend_objects_destroy_object
+							|| obj->ce->destructor) {
+							current->ref = GC_MAKE_DTOR_GARBAGE(obj);
+							GC_REF_SET_COLOR(obj, GC_PURPLE);
+						} else {
+							GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED);
+						}
+					}
 				}
 				current++;
 				idx++;
 			}
 
-			/* Call destructors
-			 *
-			 * The root buffer might be reallocated during destructors calls,
-			 * make sure to reload pointers as necessary. */
+			/* Remove nested data for objects on which a destructor will be called.
+			 * This will not remove the objects themselves, as they have been colored
+			 * purple. */
 			idx = GC_FIRST_ROOT;
+			current = GC_IDX2PTR(GC_FIRST_ROOT);
 			while (idx != end) {
-				current = GC_IDX2PTR(idx);
-				if (GC_IS_GARBAGE(current->ref)) {
+				if (GC_IS_DTOR_GARBAGE(current->ref)) {
 					p = GC_GET_PTR(current->ref);
-					if (GC_TYPE(p) == IS_OBJECT
-					 && !(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) {
-						zend_object *obj = (zend_object*)p;
-
-						GC_TRACE_REF(obj, "calling destructor");
-						GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED);
-						if (obj->handlers->dtor_obj != zend_objects_destroy_object
-								|| obj->ce->destructor) {
-							GC_ADDREF(obj);
-							obj->handlers->dtor_obj(obj);
-							GC_DELREF(obj);
-						}
-					}
+					count -= gc_remove_nested_data_from_buffer(p, current);
 				}
+				current++;
 				idx++;
 			}
 
-			/* Remove values captured in destructors */
+			/* Actually call destructors.
+			 *
+			 * The root buffer might be reallocated during destructors calls,
+			 * make sure to reload pointers as necessary. */
 			idx = GC_FIRST_ROOT;
-			current = GC_IDX2PTR(GC_FIRST_ROOT);
 			while (idx != end) {
-				if (GC_IS_GARBAGE(current->ref)) {
+				current = GC_IDX2PTR(idx);
+				if (GC_IS_DTOR_GARBAGE(current->ref)) {
 					p = GC_GET_PTR(current->ref);
-					if (GC_REFCOUNT(p) > refcounts[idx]) {
-						count -= gc_remove_nested_data_from_buffer(p, current);
+					/* Mark this is as a normal root for the next GC run,
+					 * it's no longer garbage for this run. */
+					current->ref = p;
+					/* Double check that the destructor hasn't been called yet. It could have
+					 * already been invoked indirectly by some other destructor. */
+					if (!(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) {
+						zend_object *obj = (zend_object*)p;
+						GC_TRACE_REF(obj, "calling destructor");
+						GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED);
+						GC_ADDREF(obj);
+						obj->handlers->dtor_obj(obj);
+						GC_DELREF(obj);
 					}
 				}
-				current++;
 				idx++;
 			}
 
-			pefree(refcounts, 1);
-
 			if (GC_G(gc_protected)) {
 				/* something went wrong */
 				return 0;