Fix GH-20836: Stack overflow in mb_convert_variables with recursive array references

[alexandre-daubois]

Fix #20836

[ndossche]

This looks weird that you need to utilise both recursion protection and stack protection. Are you sure that you need both?

[alexdowad]

Agree with @ndossche

[alexandre-daubois]

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]

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]

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

[alexandre-daubois]

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.

[arnaud-lb]

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.