From bf70d9ba0d493d054500a1d6033f43948ff03393 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Sun, 6 Oct 2024 19:57:21 +0200
Subject: [PATCH] Fix GH-16261: Reference invariant broken in
 mb_convert_variables()

The behaviour is weird in the sense that the reference must get
unwrapped. What ended up happening is that when destroying the old
reference the sources list was not cleaned properly. We add handling for
that. Normally we would use use ZEND_TRY_ASSIGN_STRINGL but that doesn't
work here as it would keep the reference and change values through
references (see bug #26639).

Closes GH-16272.
---
 NEWS                            |  4 +++
 ext/mbstring/mbstring.c         | 19 ++++++++++++--
 ext/mbstring/tests/gh16261.phpt | 44 +++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 ext/mbstring/tests/gh16261.phpt

diff --git a/NEWS b/NEWS
index 5f4b19f6fa8..5d3c1e46ce3 100644
--- a/NEWS
+++ b/NEWS
@@ -56,6 +56,10 @@ PHP                                                                        NEWS
   . Fix GH-16136 (Memory leak in php_ldap_do_modify() when entry is not a
     proper dictionary). (Girgias)
 
+- MBString:
+  . Fixed bug GH-16261 (Reference invariant broken in mb_convert_variables()).
+    (nielsdos)
+
 - OpenSSL:
   . Fixed stub for openssl_csr_new. (Jakub Zelenka)
 
diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c
index 0b362309ca4..70125dc9097 100644
--- a/ext/mbstring/mbstring.c
+++ b/ext/mbstring/mbstring.c
@@ -3289,7 +3289,7 @@ static int mb_recursive_convert_variable(mbfl_buffer_converter *convd, zval *var
 		if (ret != NULL) {
 			zval_ptr_dtor(orig_var);
 			// TODO: avoid reallocation ???
-			ZVAL_STRINGL(orig_var, (char *)ret->val, ret->len);
+			ZVAL_STRINGL(orig_var, (const char *) ret->val, ret->len);
 			efree(ret->val);
 		}
 	} else if (Z_TYPE_P(var) == IS_ARRAY || Z_TYPE_P(var) == IS_OBJECT) {
@@ -3305,7 +3305,22 @@ static int mb_recursive_convert_variable(mbfl_buffer_converter *convd, zval *var
 
 		ht = HASH_OF(var);
 		if (ht != NULL) {
-			ZEND_HASH_FOREACH_VAL_IND(ht, entry) {
+			ZEND_HASH_FOREACH_VAL(ht, entry) {
+				/* Can be a typed property declaration, in which case we need to remove the reference from the source list.
+				 * Just using ZEND_TRY_ASSIGN_STRINGL is not sufficient because that would not unwrap the reference
+				 * and change values through references (see bug #26639). */
+				if (Z_TYPE_P(entry) == IS_INDIRECT) {
+					ZEND_ASSERT(Z_TYPE_P(var) == IS_OBJECT);
+
+					entry = Z_INDIRECT_P(entry);
+					if (Z_ISREF_P(entry) && Z_TYPE_P(Z_REFVAL_P(entry)) == IS_STRING) {
+						zend_property_info *info = zend_get_typed_property_info_for_slot(Z_OBJ_P(var), entry);
+						if (info) {
+							ZEND_REF_DEL_TYPE_SOURCE(Z_REF_P(entry), info);
+						}
+					}
+				}
+
 				if (mb_recursive_convert_variable(convd, entry)) {
 					if (Z_REFCOUNTED_P(var)) {
 						Z_UNPROTECT_RECURSION_P(var);
diff --git a/ext/mbstring/tests/gh16261.phpt b/ext/mbstring/tests/gh16261.phpt
new file mode 100644
index 00000000000..3573bd191c6
--- /dev/null
+++ b/ext/mbstring/tests/gh16261.phpt
@@ -0,0 +1,44 @@
+--TEST--
+GH-16261 (Reference invariant broken in mb_convert_variables())
+--EXTENSIONS--
+mbstring
+--FILE--
+<?php
+class Test {
+    public string $x;
+    public string $y;
+    public array $z;
+}
+$test = new Test;
+$ref = "hello";
+$ref2 = "world";
+$ref3 = [&$ref2];
+$test->x =& $ref;
+$test->z =& $ref3;
+mb_convert_variables("EUC-JP", "Shift_JIS", $test);
+
+class Test2 {
+    public function __construct(public string $x) {}
+}
+$test2 = new Test2("foo");
+
+mb_convert_variables("EUC-JP", "Shift_JIS", $test->x);
+
+var_dump($test, $test2);
+?>
+--EXPECT--
+object(Test)#1 (2) {
+  ["x"]=>
+  string(5) "hello"
+  ["y"]=>
+  uninitialized(string)
+  ["z"]=>
+  &array(1) {
+    [0]=>
+    string(5) "world"
+  }
+}
+object(Test2)#2 (1) {
+  ["x"]=>
+  string(3) "foo"
+}