Fix stack overflow in count() with COUNT_RECURSIVE and deep arrays - #23197
Conversation
| cnt += php_count_recursive(Z_ARRVAL_P(element)); | ||
| if (UNEXPECTED(EG(exception))) { |
There was a problem hiding this comment.
Similarly to compact(), we can avoid loading EG(exception) after each element by signaling errors via the return value of php_count_recursive() (we should return -1 after zend_call_stack_size_error())
There was a problem hiding this comment.
Done. php_count_recursive() returns -1 after zend_call_stack_size_error(), the loop branches on the returned value, and PHP_FUNCTION(count) checks cnt < 0 instead of EG(exception).
One spot needed the same treatment as in compact(): the "Recursion detected" warning. A user error handler can throw there, and until now the caller's EG(exception) check is what stopped the iteration. That path returns EG(exception) ? -1 : 0, so the load stays on the cold warning path and out of the loop. A plain warning with no handler still returns 0 and counting continues, unchanged.
I also updated the other caller, SplObjectStorage::count(COUNT_RECURSIVE), so it throws instead of returning -1, and documented the sentinel in php_array.h since the function is exported.
ext/standard, ext/spl and Zend are green here, same as before. I re-checked that the test still fails without the new early exit: with the sub_cnt < 0 break removed, getPrevious() is an Error again.
| @@ -624,6 +631,9 @@ PHPAPI zend_long php_count_recursive(HashTable *ht) /* {{{ */ | |||
| ZVAL_DEREF(element); | |||
| if (Z_TYPE_P(element) == IS_ARRAY) { | |||
| cnt += php_count_recursive(Z_ARRVAL_P(element)); | |||
There was a problem hiding this comment.
I recently improved performance in php userland (PHPStan) by removing recursion and use iteration instead.
would it be an option to turn this code into iterative logic - which I guess likely means no stack overflow checks would be necessary?
(assuming iteration is faster than recursion in C land)
There was a problem hiding this comment.
It is possible, but it is a much bigger change than this fix needs. Each nesting level also sets a GC recursion flag that has to be cleared on the way out, so an iterative version needs its own explicit stack of hash positions and flagged tables, in a function that every count($a, COUNT_RECURSIVE) call goes through.
This PR is one of a series of stack limit fixes (#23090, #23111, #23113, #23115, #23125, #23126). They all use the same guard, so I would rather keep this one consistent with them and small enough to review. The check itself is one comparison against EG(stack_limit), so performance is not really the motivation here.
Worth looking at as a separate PR though, if a maintainer is interested.
There was a problem hiding this comment.
Stack limit checks have the best value/effort ratio, so we should prioritize this over eliminating recursion (at least when hitting the limit is very unlikely). Eliminating recursion is ideal of course, but it may not necessarily be faster.
fd87339 to
a11b59f
Compare
* PHP-8.5: Add a stack limit check in php_count_recursive() (#23197)
|
Thank you! |
* PHP-8.4: Add a stack limit check in php_count_recursive() (php#23197)
php_count_recursive()recurses once per nesting level with no stack check, socount($array, COUNT_RECURSIVE)on a deeply nested array exhausts the native stack and the process dies with a segfault.This is the separate PR offered in #23125, applying the same stack limit check the neighbouring functions in ext/standard use, so the call throws an Error instead of crashing. There is no GitHub issue for it.
The loop over the elements also breaks once the error is thrown, otherwise every remaining sibling recurses again and chains another Error onto the first one, and
count()returns throughRETURN_THROWS()in that case.