Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
alexandre-daubois wants to merge 1 commit into php:PHP-8.4
base: PHP-8.4
Choose a base branch
Loading
from alexandre-daubois:mbstring-rec-mb-convert-var

Conversation

@alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Jan 5, 2026
edited
Loading

Fix #20836

arnaud-lb reacted with heart emoji
Copy link
Member

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?

Copy link
Contributor

Agree with @ndossche

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

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?".

Copy link
Member Author

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.

Copy link
Member

@arnaud-lb arnaud-lb left a 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.

@alexandre-daubois alexandre-daubois force-pushed the mbstring-rec-mb-convert-var branch 2 times, most recently from b18ee3b to bbc9c18 Compare January 9, 2026 14:01
Copy link
Contributor

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

Copy link
Member Author

Fixed by decreasing the stack size and increasing the nesting depth so it also crashes on Windows

Copy link
Contributor

@youkidearitai youkidearitai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youkidearitai youkidearitai youkidearitai approved these changes

@arnaud-lb arnaud-lb arnaud-lb approved these changes

@alexdowad alexdowad Awaiting requested review from alexdowad alexdowad is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Stack overflow in mbstring variable conversion with recursive array references

AltStyle によって変換されたページ (->オリジナル) /