Merge branch 'PHP-7.4'

nikic · nikic · commit 8c927442b45d · 2019-08-13T14:54:57.000+02:00
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))
@@ -1320,9 +1325,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
@@ -1451,67 +1453,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;
diff --git a/ext/pcre/php_pcre.c b/ext/pcre/php_pcre.c
@@ -753,6 +753,7 @@ PHPAPI pcre_cache_entry* pcre_get_compiled_regex_cache(zend_string *regex)
 				return NULL;
 			}
 			_k = zend_string_init(ZSTR_VAL(BG(locale_string)), ZSTR_LEN(BG(locale_string)), 1);
+			GC_MAKE_PERSISTENT_LOCAL(_k);
 			zend_hash_add_ptr(&char_tables, _k, (void *)tables);
 			zend_string_release(_k);
 		}
diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c
@@ -4549,10 +4549,19 @@ static void add_config_entries(HashTable *hash, zval *return_value);
 static void add_config_entry(zend_ulong h, zend_string *key, zval *entry, zval *retval)
 {
 	if (Z_TYPE_P(entry) == IS_STRING) {
+		zend_string *str = Z_STR_P(entry);
+		if (!ZSTR_IS_INTERNED(str)) {
+			if (!(GC_FLAGS(str) & GC_PERSISTENT)) {
+				zend_string_addref(str);
+			} else {
+				str = zend_string_init(ZSTR_VAL(str), ZSTR_LEN(str), 0);
+			}
+		}
+
 		if (key) {
-			add_assoc_str_ex(retval, ZSTR_VAL(key), ZSTR_LEN(key), zend_string_copy(Z_STR_P(entry)));
+			add_assoc_str_ex(retval, ZSTR_VAL(key), ZSTR_LEN(key), str);
 		} else {
-			add_index_str(retval, h, zend_string_copy(Z_STR_P(entry)));
+			add_index_str(retval, h, str);
 		}
 	} else if (Z_TYPE_P(entry) == IS_ARRAY) {
 		zval tmp;

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)
Original file line number	Diff line number	Diff line change
`@@ -753,6 +753,7 @@ PHPAPI pcre_cache_entry* pcre_get_compiled_regex_cache(zend_string *regex)`
`753`	`753`	`return NULL;`
`754`	`754`	`}`
`755`	`755`	`_k = zend_string_init(ZSTR_VAL(BG(locale_string)), ZSTR_LEN(BG(locale_string)), 1);`
	`756`	`+ GC_MAKE_PERSISTENT_LOCAL(_k);`
`756`	`757`	`zend_hash_add_ptr(&char_tables, _k, (void *)tables);`
`757`	`758`	`zend_string_release(_k);`
`758`	`759`	`}`