Fix GH-23115: stack overflow in compact() with deep arrays - #23126
Conversation
There was a problem hiding this comment.
We should stop iterating here if php_compact_var() thrown, otherwise we will throw for each element of the loop. The most efficient would be to move the stack limit check just before this loop.
Edit: please test for this (add two elements at each nesting-level in the tested array. Verify that ->getPrevious() is null).
There was a problem hiding this comment.
Done. Both loops now stop when php_compact_var() has thrown: the inner ZEND_HASH_FOREACH_VAL breaks, and the argument loop in PHP_FUNCTION(compact) does RETURN_THROWS().
On the stack limit check, I moved it into the IS_ARRAY branch, just before the recursion, so string elements no longer pay for it. I kept it inside the function rather than doing it once in PHP_FUNCTION(compact), because the recursion itself is what consumes the stack, so the check has to run at each level.
One detail: it sits above the Z_PROTECT_RECURSION_P() block, not immediately before the loop. After the flag is set, an early return would leave the array marked as recursive, and a later compact() on the same array would wrongly report "Recursion detected".
The test now has two elements per nesting level and asserts getPrevious() is null, for one deep argument and for two. I verified it fails without the fix: with only the inner break removed, the sibling throws again and getPrevious() is an Error; with only RETURN_THROWS() removed, the second argument throws again. ext/standard and Zend are at 8730 tests, 0 failures.
There was a problem hiding this comment.
Ok, indeed we still need to check for EG(exception) even after moving the stack overflow check. To avoid the cost of loading EG(exception) after each loop, we should change php_compact_var() to return a zend_result.
There was a problem hiding this comment.
Done. php_compact_var() now returns zend_result, and both call sites branch on the return value instead of reading EG(exception).
One spot needed care: the two warning paths (undefined variable, and an argument that is neither a string nor an array). A user error handler can throw there, and until now the caller's EG(exception) check stopped the iteration. To keep that, those paths return EG(exception) ? FAILURE : SUCCESS. The load stays on the cold warning path and is out of the loop. A plain warning with no handler still returns SUCCESS, so compact(['x', 'nope', 'y']) keeps collecting y.
The rest is straightforward: FAILURE for the stack limit and for "Recursion detected", SUCCESS otherwise.
ext/standard and Zend are green here, same as before the change.
* PHP-8.5: Add a stack limit check in php_compact_var() (#23126)
|
Thank you! |
* PHP-8.4: Add a stack limit check in php_compact_var() (php#23126)
php_compact_var() recurses once per nesting level with no stack check, so passing a deeply nested array to compact() exhausts the native stack and the process dies with a segfault.
This adds the same stack limit check ext/standard already uses in var.c and http.c, so the call throws an Error instead of crashing.
Fixes #23115.