Fix handling of destructors during GC

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();

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*
+}

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

Zend/tests/gc_016.phpt

@@ -23,6 +23,6 @@ echo "ok\n"
 ?>
 --EXPECT--
 -> int(0)
-int(1)
-int(1)
+int(0)
+int(2)
 ok

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

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)

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)

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)

Zend/tests/generators/bug76427.phpt

@@ -21,4 +21,4 @@ var_dump(gc_collect_cycles());
 
 ?>
 --EXPECT--
-int(4)
+int(2)

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 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 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;