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

Author: alexandre-daubois

NEWS

@@ -5,6 +5,8 @@ PHP                                                                        NEWS
 - MbString:
   . Fixed bug GH-20833 (mb_str_pad() divide by zero if padding string is
     invalid in the encoding). (ndossche)
+  . 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

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);
 		}
 	}
 
@@ -3883,7 +3920,9 @@ PHP_FUNCTION(mb_convert_variables)
 				efree(ZEND_VOIDP(elist));
 				efree(ZEND_VOIDP(val_list));
 				efree(len_list);
-				php_error_docref(NULL, E_WARNING, "Cannot handle recursive references");
+				if (!EG(exception)) {
+					php_error_docref(NULL, E_WARNING, "Cannot handle recursive references");
+				}
 				RETURN_FALSE;
 			}
 		}
@@ -3905,7 +3944,9 @@ PHP_FUNCTION(mb_convert_variables)
 		zval *zv = &args[n];
 		ZVAL_DEREF(zv);
 		if (mb_recursive_convert_variable(zv, from_encoding, to_encoding)) {
-			php_error_docref(NULL, E_WARNING, "Cannot handle recursive references");
+			if (!EG(exception)) {
+				php_error_docref(NULL, E_WARNING, "Cannot handle recursive references");
+			}
 			RETURN_FALSE;
 		}
 	}

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

ext/mbstring/tests/gh20836_stack_limit.phpt

@@ -0,0 +1,38 @@
+--TEST--
+GH-20836 (Stack overflow in mb_convert_variables with recursive array references) - Stack limit case
+--EXTENSIONS--
+mbstring
+--SKIPIF--
+<?php
+if (ini_get('zend.max_allowed_stack_size') === false) {
+    die('skip No stack limit support');
+}
+if (getenv('SKIP_ASAN')) {
+    die('skip ASAN needs different stack limit setting due to more stack space usage');
+}
+?>
+--INI--
+zend.max_allowed_stack_size=512K
+--FILE--
+<?php
+
+function createDeepArray($depth) {
+    if ($depth <= 0) {
+        return 'deep value';
+    }
+    return ['nested' => createDeepArray($depth - 1)];
+}
+
+// Create a deeply nested array that will trigger stack limit
+$deepArray = createDeepArray(5000);
+
+mb_convert_variables('utf-8', 'utf-8', $deepArray);
+
+echo "Done\n";
+?>
+--EXPECTF--
+Fatal error: Uncaught Error: Maximum call stack size of %d bytes (zend.max_allowed_stack_size - zend.reserved_stack_size) reached. Infinite recursion? in %s:%d
+Stack trace:
+#0 %s(%d): mb_convert_variables('utf-8', 'utf-8', Array)
+#1 {main}
+  thrown in %s on line %d