Fixed bug #64280

[bwoebi]

This adds a counter to the internal function executor through which also all magic methods have to go. And aborts if there if there are more than max_magic_calls calls to this functions recursively.

Note to the ini entry: You can't set a higher maximum than in the php.ini as it should prevent users to set it too high and let php crash.

[nikic]

Not sure how this relates to magic methods. You patched zend_call_function which is used for quite a lot of things and not just magic. E.g. call_user_func etc will also all go through it.

[bwoebi]

Yes. But all these functions which go through this function may result in a stack overflow.

(I said "through which ALSO all magic functions have to go"...)

[bwoebi]

Example would be:

$a = array("call_user_func_array", &$a);
call_user_func_array("call_user_func_array",&$a);

Yes, is, in theory, senseless, but crashes in practice my apache....

[nikic]

Yeah, sure. What I meant was more that the configuration option should maybe have a different name, so it's clear that it isn't just magic methods :) Though quite honestly I can't think of a good name right now.

[bwoebi]

max_implicit_function_calls would be another option, but if someone understands what's meant with implicit function call...

By the way you can also understand magic call as "indirect call", "implicit call", "the Zend engine does something in background which causes the function to be called".

[cpriest]

Couldn't this more simply be implemented by checking the stack trace level against a max_call_stack_entries ini setting? That is effectively what this is doing, but with a global...

[bwoebi]

When you can explain to me how to get the stack trace level platform independent without executing in each function a macro? Or do you mean the php-stacktrace…? mhm… in which global is it stored?

[cpriest]

I'd need to search the source which I can't do at the moment but it should be in executor_global... EG() macro. You could also look at debug_backtrace() function as it would be accessing the specific variable you're looking for. I'm not sure off the top of my head how it's length would be measured, it could be stored as an int or it could be a sizeof() / sizeof() type check.

[bwoebi]

This is not the subject of this change. We don't have to limit the stacktrace of normal functions, which is already limited by the memory_limit directive, only these functions which increase the c stack (and can lead to a stack overflow).

[smalyshev]

IMO this is not a good idea to have in core. It has high potential of BC breakage for deeply nested code, and there's an extension that already does it for those who need it (xdebug). But in case there's a substantial support for it, it needs RFC and a vote as this is a change of the engine behavior.

[nikic]

Also I think that if we want this, then there should be a bit more testing on how much recursion we can do on common platforms and then create a default based on that. So we actually get a recursion limit that is close to the segfault-limit we get anyways and not just some arbitrarily chosen number.

[bwoebi]

@smalyshev You can set the limit as high as you want per ini directive. It won't break any code.

@nikic Problem is that the stack size is a variable. On some systems it may be 4 MB, on others 8 or 64 MB. The system administrator has to find out himself what's the best.

hmm, really everything needs here a RFC? Good, i'll create one.

[smalyshev]

@bwoebi your claims that "it won't break any code" shows you don't really understand how BC works, which makes me even more concerned. It won't break any code for you maybe, because you know in advance which limits your apps have. However some apps run on servers that are not configured by the apps developers, and for some apps there's almost no way to figure out how deep the code goes until the recursion limit fails, and this failure may in no way be obvious. Adding such limit means that applications would have to check for this limit in order to ensure they can continue working, even though they worked just fine before when the limit did not exist. So you would either have to set the limit unrealistically high (thus eliminating the whole point of it) or arbitrarily low, thus risking breaking code. Until we have some fact-based way to figure out which limit makes sense and is safe, I don't see how this would work - we can not set the default as low as 100, 100 is way less than big apps do, especially if it's a global counter. But we have no idea what it should be, and I'm not a big fan of introducing arbitrary limits we have no base for. If you have any base what the limit should be on common systems - let's get RFC with the numbers and see.

[nikic]

Here is a quick cli call to test it:

 php -r 'function foo() { static $i = 0; echo $i++ . "\n"; call_user_func('foo'); } foo();'

I got 19367 on Windows and 16350 for Ubuntu 32-bit on VirtualBox.

So I think the default limit should be more like 15k and not 1k.

[bwoebi]

@smalyshev I'll try on a few machines I have access to.

@nikic I get 12468 on my OS X (64 bit). With 8 MB stack size (ulimit -Ss).

What we should eventually do is doing the same as a normal fcall: save the actual executor, return and continue with a new executor until the end of the function is reached? This would prevent us imposing arbitrary limits and put everything under the control of the MM.

[bwoebi]

I get 3018 on my server... (git-version). I don't know why so less... (configured with --disable-all, --enable-debug)

Seems as there is no general limit. I am really wondering what is there the problem...

EDIT: I also noticed that the maximum of calls has decreased from every major version to the next...

[nikic]

@bwoebi Might be due to --enable-debug. Less optimization => More stack usage. Though I don't think the size with --enable-debug should bother us. It's not meant for production use after all.

[bwoebi]

Mh, you're right. I get 12472 with PHP 5.4 and 9892 with git

Btw.: It's not so bad on debug servers, yes... But these function calling internal functions may they not create an opcode which calls the function and then ZEND_VM_ENTER()? So that we won't have any recursion?

[hopeseekr]

This is why we don't have nice things in the PHP world. Any non-logging segfault is certainly a bug, and should have been a prioritized fix for years and years. Instead, Zend's modus operandi has been to mark as WONT_FIX the situation for years and years. See https://bugs.php.net/bug.php?id=67265 and the denial of this PR is just the official manifestation of that mindset.

[smalyshev]

@hopeseekr you are more than welcome to propose an obvious solution for this problem that has been overlooked by many people that discussed this problem extensively over the years. After you familiarize yourself with these discussions and it becomes obvious that your solution solves all their concerns and does not introduce additional problems, you absolutely should submit it to the developers list and/or here.

[NewEraCracker]

It's because of little things like this that we need Suhosin extension and 3rd party developers working in security extensions to solve problems in PHP core that some PHP Group developers refuse to fix.

[hopeseekr]

@smalyshev One should not segfault on infinite recursion via an interpreter. Additionally, the engine should always log errors, and it doesn't log this one. Therefore, there are two bugs which should be fixed, and this PR goes toward fixing #1 in at least this one context.

[Tyrael]

@NewEraCracker maybe you missed the fact that we don't have a fix. the proposed limit is good in theory, but if we set the default too low, it could break code which was using deeply nested calls, but not infinite recursion, if you set the default too high, then you risk that the stack is exhausted sooner than the limit is reached, and the stack size will be different across platforms/environments so we can't really have a sane default, so I think if we eventually merge something like this, it should be disabled by default, and it should be the user resposibility to configure it properly based on his/her code and environment.

for the record suhosin.executor.max_depth defaults 750 at the moment, but the before 0.9.37, the default value was 0(disabled).
(and don't get me started how much people bashed suhosin for it's too strict defaults which was breaking code left and right, mostly because hosters/distros were enabling it by default as they thought that they can get extra security without configuring it properly).

[rlerdorf]

@hopeseekr Maybe you can help us then. Please show us an interpreter that doesn't crash on unbounded recursion. I know some interpreters have recursion depth limits, but since it is impossible to know beforehand what a sensible limit is for any particular environment you still get crashes. Try this from your bash prompt, for example:

N=80000; echo '#!/usr/bin/env python' > ouch.py; for i in seq 1 $N; do echo "if None: pass"; done >> ouch.py ; chmod a+rx ouch.py ; ./ouch.py

Adjust N up or down to find the point at which you get a segfault in your environment. And there is no error logged either, since that is another impossible thing to do. Once you get into a segfault situation it is not safe to run any further code, including logging code.

You can find similar segfault-producing examples for Ruby, Perl and pretty much any interpreted language you can think of.

[hopeseekr]

Hi, I am unable to get bash to crash up to N=100000000 with that snippet. I made quick constructor-based infinite recursion scripts in C++, Ruby, Python, Perl, Java, and a non-Zend PHP interpreter.

• C++: Crashed, hard. As expected, it doesn't attempt to hold hands.
• Ruby 1.9.3: Gosh. It just got 311 levels deep and blew up, segfault. I found a couple Ruby bug reports that were marked as WONT_FIX, too.
• Python 3.3.4: After 1000 levels: RuntimeError: maximum recursion depth exceeded [error logged]
• Perl 5.18.2: It lasted longer than this comment. It's still going, at level 10,000,000+ apparently. I think it'll only stop when it's exhausted system memory. [oh it blew up after level 35,000,000 and using 15 GB of RAM. Out of Memory error logged.]
• Java : Deep recursion detected! [error logged]
• PHP 5.5.15 [normal function]: Fatal error: Maximum function nesting level of '100' reached, aborting! in recursion.php on line 3 [error logged]
• PHP 5.5.15 [magic method]: segfault [no error logged] Relevant bug report
• non-Zend PHP interpreter [magic method]: Fatal error: Stack overflow in recursion.php on line 5 [error logged]

[NewEraCracker]

@hopeseekr Are you using Xdebug or any other Zend extension in your PHP 5.5.15 Install? I don't think PHP provides anti-recursion mechanism by design.

[smalyshev]

I do not think another round of "PHP sucks and PHP developers suck, you are doing it all wrong" is going to be helpful for anything. If you've got any real solutions to contribute, go ahead and propose the pull, given you have ensured the concerns that led to the failure of previous attempts are now solved. If not, I personally intend to ignore all further discussion on this topic that does not have any solutions proposed. Repeating "you should fix it", contrary to popular belief, is not actually helping to fix it.

[Cooperb]

"I do not think another round of "PHP sucks and PHP developers suck, you are doing it all wrong" is going to be helpful for anything. If you've got any real solutions to contribute, go ahead and propose the pull, given you have ensured the concerns that led to the failure of previous attempts are now solved. If not, I personally intend to ignore all further discussion on this topic that does not have any solutions proposed. Repeating "you should fix it", contrary to popular belief, is not actually helping to fix it."

I agree but it only works when the PHP dev's don't then mark bugs as worthless to fix or no longer supported or not important/etc/etc/etc (the list goes on and on and on)

[ircmaxell]

This problem is impossible to solve in the general sense. Citing Stas frm 8 years ago:

So far nobody had proposed a solution for endless loop problem that
would satisfy these conditions:

1. No false positives (i.e. good code always works)
2. No slowdown for execution
3. Works with any stack size
   Thus, this problem remains unsloved.

Now, #1 is quite literally impossible to solve due to the halting problem. #2 is trivial if you keep a counter of stack depth (since you're just checking the incremented stack level on stack push).

Finally, #3 Is a much harder problem to solve. Considering that some operating systems will allocate stack space in a non-contiguous manner, it's not going to be possible to implement with 100% accuracy, since it's impossible to portably get the stack size or usage (for a specific platform it may be possible or even easy, but not in general).

So it's not "worthless to fix", it's that there's been no general fix proposed that doesn't have drawbacks for 100% correct code. Keeping a counter at stack push is OK. But where do you set the depth? Either really conservatively (like xdebug), which will break a ton of apps that do deep recursion (I have a number that do that personally) or high enough that you won't prevent segfaults on some systems.

So what's better, a partial solution that sort-of works in some cases? A solution that kills a lot of valid code (that's no where near performance limitations)? Or no "solution", which is exactly what many programming languages do...

[jakoch]

Instead of using a recursion-depth counter (suhosin.executor.max_depth), one could also use a recursion-loop timeout (max_recursion_time). In other words: if a function is self-calling and not returning in X seconds, protect the user by throwing a "recursion timeout" exception.

That's the same kind of safe-guard developers already know from the INI directive "max_execution_time". It's something developers have to learn: that PHP stops after 30s by default and that they have to lift this bound intentionally.

Same rules could apply for the "max_recursion_time". PHP will stop recursive calls after X seconds and developers have to lift this restriction intentionally to work with longer/deeper running recursive calls.
This enforces, that devs have to actually care about the recursive piece of source code.

It's still possible that the stack size limit is hit within the max_recursion_time,
but then it would crash with and without a recursion safe-guard.

[ircmaxell]

@jakoch what would trigger "max_recusion_time"? Recursion is not limited to self-recursion. So defining when recursion starts would be non-trivial. You'd need to scan the entire stack at every function call for the existence of the function call in the stack (or keep a hash-table counter, or something like that).

And what would be the benefit of this expense? As you indicated PHP already limits execution time. If you want to limit recursion's time, simply change the time limit prior to calling the recursive function, and change it back after.

But considering it doesn't prevent crashes (in most forms, since it's trivial to blow the stack in fractions of a second with trivial self recursion), I'm not sure the value add...

[jakoch]

The current state is: crashes are not prevented.

The goal of "max_magic_calls " by bwoebi is to provide a safe-guard mechanism to prevent people from falling into the infinite loop trap. Same goals for "suhosin.executor.max_depth" and "max_recursion_time".

The value added is: that developers are informed by an exception, that "recursion" is happening.
It results in a proper log message, instead of a crash (dump).
An "recursion" exception would be catchable.

The benefit of the expense for recursion detection is convenience.
It's a helper for inexpierenced developers, who often create infinite recursion functions by mistake and wonder why their stuff segfaults. Something like a safety-net for unintentional recursion.

It's still possible to have a "max_execution_time 0s" and "max_recursion_time 5s".

Detecting recursion is non-trivial. But still: we can detect the most trivial forms of recursion by doing some program analysis. You described how this could work: by scaning the call stack. like a garbage collector.

[rlerdorf]

@jakoch note that @ircmaxell was saying that implementing this detection is a non-starter because it would require an entire stack scan on every function call. The performance hit would be astronomical.

[hopeseekr]

The major problem right now with the state of affairs is with large enterprise applications (3 x 10^5 LOC+) that also use a lot of third party libraries and frameworks is when this sort of recursion depth crash happens. The end-user is presented with an amorphous HTTP 500 Error screen if via FPM or the dreaded White Screen of Death if via mod_php. The end-devs and sysadmins have no idea at all where in the code or when the crash is happening. It's a mess.

This happens with any number of segfaults with PHP, and I understand a lot of them, being segfaults, by their very nature are unknown edge cases that just haven't been discovered yet. But recursion-depth crashes are known, so there should be some mechanism to, even if temporarily and with full knowledge of the performance loss, to turn on recursion-depth crashes in the error_log.

That is what I wish to be added.

[rlerdorf]

@hopeseekr but it can be done with zero performance loss simply by enabling core dumps. It is exceedingly obvious from a backtrace when it is a recursion problem. Granted, you need to be somewhat technical to read the backtrace but if you are running massive enterprise apps like that I would hope you have at least one engineer who can read a backtrace around. Alternatively you can install external debugging tools like xdebug and set the recursion limit there. Adding an arbitrary recursion check to mainstream production PHP slowing down all scripts all the time is simply not going to happen. Someone needs to come up with a decent solution with minimal performance impact for this to go anywhere.

[hopeseekr]

I agree with all that. Somewhere in there lies our disconnect ;-) Whenever I encountered issues like this Pull Request address, not even xdebug will break in time, PHP will still segfault, there won't be a backtrace, an error_log entry, nothing, nada, except frazzled customers, QAers, and eventually devs, intermittently.

[rlerdorf]

@hopeseekr you just illustrated why this isn't easily addressed. If you set xdebug's xdebug.max_nesting_level to some relatively low value and it still segfaulted, why do you think doing the same thing in core PHP will fare any better? But I also don't agree with you having nothing to go on. A segfault drops a core file and in that core file is an easily readable backtrace that tells you exactly what is going on.

[jakoch]

Adding an arbitrary recursion check to mainstream production PHP slowing down all scripts all the time is simply not going to happen. Someone needs to come up with a decent solution with minimal performance impact for this to go anywhere.

Doing a stack scan on every function call is too slow.

Using a stack depth / nesting-level / accumulation counter could probably work.

Another solution i could imagine is to introduce a new stack with conditions/limits defined for function behaviour monitoring. The stack has a low level barrier, to limit a certain recursion call chain.
This could be combined with a decorator approach to wrap contributing functions calls.
It adds additional push and pop operations for stack management and memory overhead.

Side-note: there were some mentions of the "halting-problem" or "detection of infinite loops" in this thead. I consider this being not related. Here a condition is introduced to limit the loop.
We stop by criteria: after N calls or after N secs time. The loop is not infinite. It is boxed, working within clear boundaries.

why do you think doing the same thing in core PHP will fare any better?

I consider recursion detection (and protection) a language feature, because the monitoring is really close to the call stack. This shouldn't live in a "security" or "debugger" extension.
Also, when the detection works, possible recursion/loop optimizations techniques (jumps, tail calls) would find a good place in the opcode optimizer.