-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix GH-20836: Stack overflow in mb_convert_variables with recursive array references #20839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix GH-20836: Stack overflow in mb_convert_variables with recursive array references #20839
Conversation
ndossche
commented
Jan 5, 2026
This looks weird that you need to utilise both recursion protection and stack protection. Are you sure that you need both?
alexdowad
commented
Jan 6, 2026
Agree with @ndossche
alexandre-daubois
commented
Jan 6, 2026
I added stack protection in order to have some fast path at the beginning of functions to immediately return if we can detect an error, instead of going through the whole code waiting for a recursion error to be raised
ndossche
commented
Jan 6, 2026
I added stack protection in order to have some fast path at the beginning of functions to immediately return if we can detect an error, instead of going through the whole code waiting for a recursion error to be raised
You're optimizing the sad flow here instead of the happy flow. We generally don't do that. While here it doesn't really degrade the performance of the happy flow noticeably, it's still adding complexity. I'd prefer to stick with the recursion protection alone.
arnaud-lb
commented
Jan 8, 2026
both recursion protection and stack protection
It can be useful to differentiate between the two cases, as one of them is not supported by mb_convert_variables(), while the other is supported as long as the stack is large enough. So a "Cannot handle recursive references" tells you that you can not use mb_convert_variables() like that, while "Maximum call stack size reached" tells you that you may be able to fix the problem by increasing the stack size.
But changing the stack limit message emitted by mb_convert_variables() so that it mentions potential recursive references would work too, I think: "Maximum call stack size reached. Deeply nested structure or recursive references?".
38e60e4 to
6c7a109
Compare
alexandre-daubois
commented
Jan 9, 2026
So I pushed a new version adding a check before emitting the warning. This way, the error is displayed if the stack is overflowed and the warning is kept in case of deep/infinite recursion. Because of how the code is articulated, that's what I could achieve.
6c7a109 to
ca26e6d
Compare
@arnaud-lb
arnaud-lb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right to me!
Please add tests for the stack limit case (see the SKIPIF and INI sections in https://github.com/devnexen/php-src/blob/dcdcad3ab1f83563dc2775282d73e3bc8afde828/ext/standard/tests/general_functions/gh20840.phpt).
Also maybe wait for other reviews.
b18ee3b to
bbc9c18
Compare
youkidearitai
commented
Jan 10, 2026
Windows test is failed. Diff is below:
001- 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
002- Stack trace:
003- #0 %s(%d): mb_convert_variables('utf-8', 'utf-8', Array)
004- #1 {main}
005- thrown in %s on line %d
001+ Done
...e array references
bbc9c18 to
36777da
Compare
alexandre-daubois
commented
Jan 12, 2026
Fixed by decreasing the stack size and increasing the nesting depth so it also crashes on Windows
@youkidearitai
youkidearitai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Uh oh!
There was an error while loading. Please reload this page.
Fix #20836