Fix GH-23204: use-after-free when __toString() destroys an array argument

implode() walks the array with ZEND_HASH_FOREACH_VAL while holding no
reference on it. Converting a Stringable element runs user code, and if
that code drops the last remaining reference to the array (`$a = null;`
from __toString()), arData is freed and the next iteration reads freed
memory. strtr() and str_replace() read their array arguments the same
way and crash the same way, so they are fixed here too.

Taking a reference on the table for the duration of the read keeps it
alive and turns an in-place mutation into a separation instead, same as
zend_compare_symbol_tables() does around zend_hash_compare(). In
implode() the reference is released after the pieces have been
concatenated, since the collected zend_strings are still owned by the
array until then.

Close GH-23207

Author: lazerg

ext/standard/string.c

@@ -983,6 +983,9 @@ PHPAPI void php_implode(const zend_string *glue, HashTable *pieces, zval *return
 
 	uint32_t flags = ZSTR_GET_COPYABLE_CONCAT_PROPERTIES(glue);
 
+	/* Converting an element may call __toString(), which can destroy pieces. */
+	GC_TRY_ADDREF(pieces);
+
 	ZEND_HASH_FOREACH_VAL(pieces, tmp) {
 		if (EXPECTED(Z_TYPE_P(tmp) == IS_STRING)) {
 			ptr->str = Z_STR_P(tmp);
@@ -1042,6 +1045,7 @@ PHPAPI void php_implode(const zend_string *glue, HashTable *pieces, zval *return
 	}
 
 	free_alloca(strings, use_heap);
+	GC_TRY_DTOR_NO_REF(pieces);
 	RETURN_NEW_STR(str);
 }
 /* }}} */
@@ -3392,7 +3396,12 @@ static void php_strtr_array(zval *return_value, zend_string *str, HashTable *fro
 {
 	if (zend_hash_num_elements(from_ht) < 1) {
 		RETURN_STR_COPY(str);
-	} else if (zend_hash_num_elements(from_ht) == 1) {
+	}
+
+	/* Converting a replacement may call __toString(), which can destroy from_ht. */
+	GC_TRY_ADDREF(from_ht);
+
+	if (zend_hash_num_elements(from_ht) == 1) {
 		zend_long num_key;
 		zend_string *str_key, *tmp_str, *replace, *tmp_replace;
 		zval *entry;
@@ -3421,11 +3430,13 @@ static void php_strtr_array(zval *return_value, zend_string *str, HashTable *fro
 			}
 			zend_tmp_string_release(tmp_str);
 			zend_tmp_string_release(tmp_replace);
-			return;
+			break;
 		} ZEND_HASH_FOREACH_END();
 	} else {
 		php_strtr_array_ex(return_value, str, from_ht);
 	}
+
+	GC_TRY_DTOR_NO_REF(from_ht);
 }
 
 /* {{{ Translates characters in str using given translation tables */
@@ -4485,6 +4496,17 @@ static void _php_str_replace_common(
 		RETURN_THROWS();
 	}
 
+	/* Converting an element may call __toString(), which can destroy the arrays. */
+	if (search_ht) {
+		GC_TRY_ADDREF(search_ht);
+	}
+	if (replace_ht) {
+		GC_TRY_ADDREF(replace_ht);
+	}
+	if (subject_ht) {
+		GC_TRY_ADDREF(subject_ht);
+	}
+
 	/* if subject is an array */
 	if (subject_ht) {
 		array_init(return_value);
@@ -4511,6 +4533,16 @@ static void _php_str_replace_common(
 	if (zcount) {
 		ZEND_TRY_ASSIGN_REF_LONG(zcount, count);
 	}
+
+	if (search_ht) {
+		GC_TRY_DTOR_NO_REF(search_ht);
+	}
+	if (replace_ht) {
+		GC_TRY_DTOR_NO_REF(replace_ht);
+	}
+	if (subject_ht) {
+		GC_TRY_DTOR_NO_REF(subject_ht);
+	}
 }
 
 /* {{{ php_str_replace_common */

ext/standard/tests/strings/gh23204.phpt

@@ -0,0 +1,109 @@
+--TEST--
+GH-23204 (Use-after-free when __toString() destroys the array being read)
+--CREDITS--
+e1abrador
+--FILE--
+<?php
+class Unset_ implements Stringable {
+    public function __toString(): string {
+        global $a;
+        $a = null;
+        return "X";
+    }
+}
+
+$a = [new Unset_, 2, 3, 4];
+echo "destroyed: ", implode(",", $a), "\n";
+var_dump($a);
+
+class Append implements Stringable {
+    public function __toString(): string {
+        global $b;
+        $b[] = str_repeat("y", 32);
+        return "X";
+    }
+}
+
+$b = [new Append, 2, 3, 4];
+echo "appended: ", implode(",", $b), "\n";
+echo "count: ", count($b), "\n";
+
+class Boom implements Stringable {
+    public function __toString(): string {
+        global $c;
+        $c = null;
+        throw new Exception("boom");
+    }
+}
+
+$c = [new Boom, 2, 3, 4];
+try {
+    implode(",", $c);
+} catch (Exception $e) {
+    echo $e::class, ': ', $e->getMessage(), "\n";
+}
+
+class UnsetPats implements Stringable {
+    public function __toString(): string {
+        global $d;
+        $d = null;
+        return "X";
+    }
+}
+
+$d = ["aa" => new UnsetPats, "bb" => "2", "cc" => "3", "dd" => "4"];
+echo "strtr: ", strtr("aabbccdd", $d), "\n";
+
+$e = ["aa" => new UnsetPats];
+$d = &$e;
+echo "strtr single: ", strtr("aabb", $e), "\n";
+
+class UnsetSearch implements Stringable {
+    public function __toString(): string {
+        global $f;
+        $f = null;
+        return "a";
+    }
+}
+
+$f = [new UnsetSearch, "b", "c", "d"];
+echo "str_replace search: ", str_replace($f, "z", "abcd"), "\n";
+
+class UnsetReplace implements Stringable {
+    public function __toString(): string {
+        global $g;
+        $g = null;
+        return "z";
+    }
+}
+
+$g = [new UnsetReplace, "y", "y", "y"];
+echo "str_replace replace: ", str_replace(["a", "b", "c", "d"], $g, "abcd"), "\n";
+
+class UnsetSubject implements Stringable {
+    public function __toString(): string {
+        global $h;
+        $h = null;
+        return "abcd";
+    }
+}
+
+$h = [new UnsetSubject, "abcd"];
+var_dump(str_replace("a", "z", $h));
+?>
+--EXPECT--
+destroyed: X,2,3,4
+NULL
+appended: X,2,3,4
+count: 5
+Exception: boom
+strtr: X234
+strtr single: Xbb
+str_replace search: zzzz
+str_replace replace: zyyy
+array(2) {
+  [0]=>
+  string(4) "zbcd"
+  [1]=>
+  string(4) "zbcd"
+}