Fix GH-20836: Stack overflow in mb_convert_variables with recursive array references

Author: alexandre-daubois

NEWS

@@ -2,6 +2,10 @@ PHP                                                                        NEWS
 |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 ?? ??? ????, PHP 8.4.18
 
+- Mbstring:
+  . Fixed bug GH-20836 (Stack overflow in mb_convert_variables with
+    recursive array references). (alexandre-daubois)
+
 - Readline:
   . Fixed bug GH-18139 (Memory leak when overriding some settings
     via readline_info()). (ndossche)

ext/mbstring/mbstring.c

@@ -3691,11 +3691,26 @@ PHP_FUNCTION(mb_convert_kana)
 	RETVAL_STR(jp_kana_convert(str, enc, opt));
 }
 
+static zend_always_inline bool mb_check_stack_limit(void)
+{
+#ifdef ZEND_CHECK_STACK_LIMIT
+	if (UNEXPECTED(zend_call_stack_overflowed(EG(stack_limit)))) {
+		zend_call_stack_size_error();
+		return true;
+	}
+#endif
+	return false;
+}
+
 static unsigned int mb_recursive_count_strings(zval *var)
 {
 	unsigned int count = 0;
 	ZVAL_DEREF(var);
 
+	if (mb_check_stack_limit()) {
+		return 0;
+	}
+
 	if (Z_TYPE_P(var) == IS_STRING) {
 		count++;
 	} else if (Z_TYPE_P(var) == IS_ARRAY || Z_TYPE_P(var) == IS_OBJECT) {
@@ -3726,6 +3741,10 @@ static bool mb_recursive_find_strings(zval *var, const unsigned char **val_list,
 {
 	ZVAL_DEREF(var);
 
+	if (mb_check_stack_limit()) {
+		return true;
+	}
+
 	if (Z_TYPE_P(var) == IS_STRING) {
 		val_list[*count] = (const unsigned char*)Z_STRVAL_P(var);
 		len_list[*count] = Z_STRLEN_P(var);
@@ -3763,6 +3782,10 @@ static bool mb_recursive_convert_variable(zval *var, const mbfl_encoding* from_e
 {
 	zval *entry, *orig_var;
 
+	if (mb_check_stack_limit()) {
+		return true;
+	}
+
 	orig_var = var;
 	ZVAL_DEREF(var);
 
@@ -3771,17 +3794,25 @@ static bool mb_recursive_convert_variable(zval *var, const mbfl_encoding* from_e
 		zval_ptr_dtor(orig_var);
 		ZVAL_STR(orig_var, ret);
 	} else if (Z_TYPE_P(var) == IS_ARRAY || Z_TYPE_P(var) == IS_OBJECT) {
-		if (Z_TYPE_P(var) == IS_ARRAY) {
-			SEPARATE_ARRAY(var);
-		}
-		if (Z_REFCOUNTED_P(var)) {
-			if (Z_IS_RECURSIVE_P(var)) {
+		HashTable *ht = HASH_OF(var);
+		HashTable *orig_ht = ht;
+
+		if (ht) {
+			if (GC_IS_RECURSIVE(ht)) {
 				return true;
 			}
-			Z_PROTECT_RECURSION_P(var);
+
+			GC_TRY_PROTECT_RECURSION(ht);
 		}
 
-		HashTable *ht = HASH_OF(var);
+		if (Z_TYPE_P(var) == IS_ARRAY) {
+			SEPARATE_ARRAY(var);
+			ht = Z_ARRVAL_P(var);
+
+			if (ht && ht != orig_ht && !GC_IS_RECURSIVE(ht)) {
+				GC_TRY_PROTECT_RECURSION(ht);
+			}
+		}
 		if (ht != NULL) {
 			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.
@@ -3800,16 +3831,22 @@ static bool mb_recursive_convert_variable(zval *var, const mbfl_encoding* from_e
 				}
 
 				if (mb_recursive_convert_variable(entry, from_encoding, to_encoding)) {
-					if (Z_REFCOUNTED_P(var)) {
-						Z_UNPROTECT_RECURSION_P(var);
+					if (ht && ht != orig_ht) {
+						GC_TRY_UNPROTECT_RECURSION(ht);
+					}
+					if (orig_ht) {
+						GC_TRY_UNPROTECT_RECURSION(orig_ht);
 					}
 					return true;
 				}
 			} ZEND_HASH_FOREACH_END();
 		}
 
-		if (Z_REFCOUNTED_P(var)) {
-			Z_UNPROTECT_RECURSION_P(var);
+		if (ht && ht != orig_ht) {
+			GC_TRY_UNPROTECT_RECURSION(ht);
+		}
+		if (orig_ht) {
+			GC_TRY_UNPROTECT_RECURSION(orig_ht);
 		}
 	}
 

ext/mbstring/tests/gh20836.phpt

@@ -0,0 +1,33 @@
+--TEST--
+GH-20836 (Stack overflow in mb_convert_variables with recursive array references)
+--EXTENSIONS--
+mbstring
+--FILE--
+<?php
+
+$a = [];
+$b = [];
+$a[] = $b[] = &$a;
+var_dump(mb_convert_variables('utf-8', 'utf-8', $a));
+
+$c = [];
+$c[] = &$c;
+var_dump(mb_convert_variables('utf-8', 'utf-8', $c));
+
+$normal = ['test', 'array'];
+var_dump(mb_convert_variables('utf-8', 'utf-8', $normal));
+
+$d = ['level1' => ['level2' => ['level3' => 'data']]];
+var_dump(mb_convert_variables('utf-8', 'utf-8', $d));
+
+echo "Done\n";
+?>
+--EXPECTF--
+Warning: mb_convert_variables(): Cannot handle recursive references in %s on line %d
+bool(false)
+
+Warning: mb_convert_variables(): Cannot handle recursive references in %s on line %d
+bool(false)
+string(5) "UTF-8"
+string(5) "UTF-8"
+Done