Fixed bug #72530

nikic · nikic · commit 60a7e60b61b8 · 2019-08-13T14:53:53.000+02:00
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ class MemoryLeak`
`19`	`19`	`private $things = [];`
`20`	`20`	`}`
`21`	`21`
`22`		`-ini_set('memory_limit', '10M');`
	`22`	`+ini_set('memory_limit', '20M');`
`23`	`23`
`24`	`24`	`for ($i = 0; $i < 100000; ++$i) {`
`25`	`25`	`$obj = new MemoryLeak();`
-Original file line number
+Diff line change
 ?>
 --EXPECT--
 -int(4)
 +int(2)