diff --git a/NEWS b/NEWS index fc4f5d3d2978c..5c1ba17925f70 100644 --- a/NEWS +++ b/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 diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c index 1491e1728cd58..b320a6a5f0e47 100644 --- a/ext/mbstring/mbstring.c +++ b/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; } } diff --git a/ext/mbstring/tests/gh20836.phpt b/ext/mbstring/tests/gh20836.phpt new file mode 100644 index 0000000000000..60b6a4ce8d4f0 --- /dev/null +++ b/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-- + ['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 diff --git a/ext/mbstring/tests/gh20836_stack_limit.phpt b/ext/mbstring/tests/gh20836_stack_limit.phpt new file mode 100644 index 0000000000000..38c2de53a6123 --- /dev/null +++ b/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-- + +--INI-- +zend.max_allowed_stack_size=512K +--FILE-- + 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