About restricting the recursive implicit calls

12 years ago by Bob Weinand — view source — reply

unread

Hi!

How should I name the ini parameter?
max_magic_calls (magic ?=? everything which is not a direct function call)
max_implicit_calls
max_implicit_function_calls

Or what do you think is the most self documenting name?

Bob

p.s.: There is no reason why not to fix this in this way, I think, as you can test at how may iterations the stack will overflow and set the limit near to this maximum. Which is exactly what we have already today, only without possible crashes.

12 years ago by johannes@schlueters.de — view source — reply

unread

p.s.: There is no reason why not to fix this in this way, I think, as
you can test at how may iterations the stack will overflow and set the
limit near to this maximum. Which is exactly what we have already
today, only without possible crashes.

For one the stack frames aren't always the same size on all nesting
levels, depending on what's exactly happening.

There have neen different discussions about this in the past. One I
could finde first was
Vote for Zend Deep Stack Prevention (ZDSP)
http://news.php.net/php.internals/22011

In general reasons for not doing that were that we'd have a hard time
setting a good default for an counter-based approach, whereas an
approach checking the actual stack size would be too expensive.

I however agree that with recent engine improvements, where infinite
recursion is harder to get one might rethink that. A name using "magic"
sounds inappropriate, though in any case. :-)

johannes

12 years ago by Bob Weinand — view source — reply

unread

Am 25.2.2013 um 17:45 schrieb Johannes Schlüter johannes@schlueters.de:

p.s.: There is no reason why not to fix this in this way, I think, as
you can test at how may iterations the stack will overflow and set the
limit near to this maximum. Which is exactly what we have already
today, only without possible crashes.

For one the stack frames aren't always the same size on all nesting
levels, depending on what's exactly happening.

There have neen different discussions about this in the past. One I
could finde first was
Vote for Zend Deep Stack Prevention (ZDSP)
http://news.php.net/php.internals/22011

In general reasons for not doing that were that we'd have a hard time
setting a good default for an counter-based approach, whereas an
approach checking the actual stack size would be too expensive.

I however agree that with recent engine improvements, where infinite
recursion is harder to get one might rethink that. A name using "magic"
sounds inappropriate, though in any case. :-)

Yes, but you can do an approximation. And in 99.999% of the cases 100
will be enough. I can hardly imagine a case where you need to do over
100 implicit function calls. They should fit in every normal stack size of
servers today.

This "ZDSP" is also checking very often (at every single opcode execution)
while my patch checks only in a more rare - in little scripts nearly never -
called function: a negligible cost.

Right. As far I see the implicit function calls are also the only possibility left
today out to let php crash due to a stack overflow beside pcre-functions...

Then max_implicit_function_calls would be a good name?

Bob

12 years ago by Stas Malyshev — view source — reply

unread

Hi!

Yes, but you can do an approximation. And in 99.999% of the cases 100
will be enough. I can hardly imagine a case where you need to do over
100 implicit function calls. They should fit in every normal stack size of
servers today.

Depth-first search in a modest-size data structure would easily go over

But if you need it, xdebug already has this option.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Bob Weinand — view source — reply

unread

Am 25.2.2013 um 19:10 schrieb Stas Malyshev smalyshev@sugarcrm.com:

Hi!

Yes, but you can do an approximation. And in 99.999% of the cases 100
will be enough. I can hardly imagine a case where you need to do over
100 implicit function calls. They should fit in every normal stack size of
servers today.

Depth-first search in a modest-size data structure would easily go over

But if you need it, xdebug already has this option.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

--

No, no, ...

You may use recursive functions (which are limited by the memory_limit),
but if you use recursive magics, it's a design error. This is not the purpose
of magics (, call_user_func(_array)?) and simply an abuse of the language.

Bob

12 years ago by Stas Malyshev — view source — reply

unread

Hi!

You may use recursive functions (which are limited by the memory_limit),
but if you use recursive magics, it's a design error. This is not the purpose
of magics (, call_user_func(_array)?) and simply an abuse of the language.

I think you are confusing magic functions with internal functions and
functions being called from internal functions. These are different
things. Looking at your patch, you are changing zend_call_function,
which can be called in many situations, every time engine code calls any
function which is not from a userspace context (i.e. PHP code). Limiting
it to any arbitrary number can (and does) break code, that's why it
wasn't done (it's not like it wasn't knows for years) and there are
extensions - like xdebug - which do introduce such limits. In any case,
100 is way, way too low.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Stas Malyshev — view source — reply

unread

Hi!

p.s.: There is no reason why not to fix this in this way, I think,

There actually is. First, any option modifying engine behavior creates a
compatibility problem, since now the code that needs to work with it has
to check and be tested for yet another variable. Second, why does it
concentrate on magics? You can get stack overflow with any functions.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Bob Weinand — view source — reply

unread

Am 25.2.2013 um 19:08 schrieb Stas Malyshev smalyshev@sugarcrm.com:

Hi!

p.s.: There is no reason why not to fix this in this way, I think,

There actually is. First, any option modifying engine behavior creates a
compatibility problem, since now the code that needs to work with it has
to check and be tested for yet another variable. Second, why does it
concentrate on magics? You can get stack overflow with any functions.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

It doesn't break if the limit is not set too low. It is like I now is, only without the
segmentation faults.

No, normal functions don't crash as - not 100% sure if I'm right - the Zend Engine
erealloc's (under control of the MM (and the memory limit)) the previous function
context and then returns; after the execution of the function the stack is restored:
a stack is only emulated and cannot cause crashes (the real c stack doesn't grow).

I concentrate only on implicit function calls as they are the only problem.

Bob

12 years ago by Bob Weinand — view source — reply

unread

Forgotten to cc to internals list:

Am 25.2.2013 um 19:50 schrieb Stas Malyshev smalyshev@sugarcrm.com:

Hi!

You may use recursive functions (which are limited by the memory_limit),
but if you use recursive magics, it's a design error. This is not the purpose
of magics (, call_user_func(_array)?) and simply an abuse of the language.

I think you are confusing magic functions with internal functions and
functions being called from internal functions. These are different
things. Looking at your patch, you are changing zend_call_function,
which can be called in many situations, every time engine code calls any
function which is not from a userspace context (i.e. PHP code). Limiting
it to any arbitrary number can (and does) break code, that's why it
wasn't done (it's not like it wasn't knows for years) and there are
extensions - like xdebug - which do introduce such limits. In any case,
100 is way, way too low.

Hi!

I quickly looked over http://lxr.php.net/search?q=zend_call_function but I didn't
find any case where it may call itself recursively without "wrong" user-input.

I can increase the default limit to 1000, but if it is too high it has exactly no sense.
(Btw. you've seen that the counter is decremented just before returning and don't
assume I'd like to create a global maximum? It's only affecting recursions...)

We don't discuss about xDebug, but about integrating it into the core?

Bob

12 years ago by johannes@schlueters.de — view source — reply

unread

I can increase the default limit to 1000, but if it is too high it has
exactly no sense.

Which is exactly the issue i mentioned about being a "good default"

We don't discuss about xDebug, but about integrating it into the
core?

Well, nobody extracted the relevant pieces for a production environment
from xdebug and put it on pecl, so nobody had a need for this on a
production system ;-)

johannes
(who is +/-0 on this)

12 years ago by Bob Weinand — view source — reply

unread

Am 25.2.2013 um 22:01 schrieb Johannes Schlüter johannes@schlueters.de:

I can increase the default limit to 1000, but if it is too high it has
exactly no sense.

Which is exactly the issue i mentioned about being a "good default"

We don't discuss about xDebug, but about integrating it into the
core?

Well, nobody extracted the relevant pieces for a production environment
from xdebug and put it on pecl, so nobody had a need for this on a
production system ;-)

johannes
(who is +/-0 on this)

I've pushed a new commit to github. Changed the ini setting name to
max_implicit_function_calls (max_implicit_function_call_nesting_level
would be more self documenting but is way too long).

If I knew anything about C programming etc., I'd simply misuse the original
purpose of xDebug and only use it for this functionality if needed.

Bob

p.s.: and as there is a simply inexistent CPU cost: why not?

12 years ago by Bob Weinand — view source — reply

unread

May someone merge this PR (#290) as there are no arguments against it?

Or do I have to wait a little bit? (How long?)

Bob

12 years ago by Stas Malyshev — view source — reply

unread

Hi!

May someone merge this PR (#290) as there are no arguments against it?

I just outlined arguments against it in my last emails. If your email is
not working properly and you miss some emails please check the list
archives.

Or do I have to wait a little bit? (How long?)

In my personal opinion, indefinitely long, since I think it is not a
good idea at all to have it in the core, and the same functionality
already exists in xdebug.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

12 years ago by Ferenc Kovacs — view source — reply

unread

On Wed, Feb 27, 2013 at 9:06 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Hi!

May someone merge this PR (#290) as there are no arguments against it?

I just outlined arguments against it in my last emails. If your email is
not working properly and you miss some emails please check the list
archives.

Or do I have to wait a little bit? (How long?)

In my personal opinion, indefinitely long, since I think it is not a
good idea at all to have it in the core, and the same functionality
already exists in xdebug.

and in suhosin.
I think that it is a pretty handy feature, both from the security
pov(hitting a fatal error is better than php crashing, killing the workers,
resetting random seed, etc.) and can help with debugging the problem(as you
will lose the non flushed output buffer on the crash).
so if we could find an implementation which addresses your issues(no major
perf cost, sane defaults, no edge cases left out, etc.) then I can't see
why we shouldn't add it.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

11 years ago by Ferenc Kovacs — view source — reply

unread

On Wed, Feb 27, 2013 at 9:06 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:

Hi!

May someone merge this PR (#290) as there are no arguments against it?

I just outlined arguments against it in my last emails. If your email is
not working properly and you miss some emails please check the list
archives.

Or do I have to wait a little bit? (How long?)

In my personal opinion, indefinitely long, since I think it is not a
good idea at all to have it in the core, and the same functionality
already exists in xdebug.

and in suhosin.
I think that it is a pretty handy feature, both from the security
pov(hitting a fatal error is better than php crashing, killing the workers,
resetting random seed, etc.) and can help with debugging the problem(as you
will lose the non flushed output buffer on the crash).
so if we could find an implementation which addresses your issues(no major
perf cost, sane defaults, no edge cases left out, etc.) then I can't see
why we shouldn't add it.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

sorry for resurecting the old thread,
if we are concerned about the perf cost of counting and checking the
recursion, and that the default value would break some application, maybe
we could introduce this as an optional feature: the ini setting is 0 by
default, which will disable even the checks for recursion, so people have
to opt-in to use it and it's their burden to configure it properly.
I know that this would still allow distros and hosters to misuse it, and
probably some apps will start checking for this setting to (hopefully only)
warn about the setting being too low, or even picking a different strategy
when the setting is too low or simply increasing the limit at runtime.
maybe we can do something in the next major version which would allow us to
gracefully bail from these kind of errors without imposing arbitrary limits?

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

11 years ago by Dmitry Stogov — view source — reply

unread

The overhead of the patch is negligible, so it's not a big problem to
include it from perf point of view, however:

It won't prevent crash because of C stack overflow, in case the limit is
too big.
Disable check with some default value (0 or -1) would be also good
I'm not sure about naming. It would be better to use something more
clear in INI name and error message,

May be instead of this, we should check C stack pointer in
zend_call_function() and throw exception or error, when the pointer comes
close to the limit?
This check must be compiler and may be platform specific :(

Thanks. Dmitry.

On Wed, Feb 27, 2013 at 9:06 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:

Hi!

May someone merge this PR (#290) as there are no arguments against it?

I just outlined arguments against it in my last emails. If your email is
not working properly and you miss some emails please check the list
archives.

Or do I have to wait a little bit? (How long?)

In my personal opinion, indefinitely long, since I think it is not a
good idea at all to have it in the core, and the same functionality
already exists in xdebug.

and in suhosin.
I think that it is a pretty handy feature, both from the security
pov(hitting a fatal error is better than php crashing, killing the workers,
resetting random seed, etc.) and can help with debugging the problem(as you
will lose the non flushed output buffer on the crash).
so if we could find an implementation which addresses your issues(no
major perf cost, sane defaults, no edge cases left out, etc.) then I can't
see why we shouldn't add it.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

sorry for resurecting the old thread,
if we are concerned about the perf cost of counting and checking the
recursion, and that the default value would break some application, maybe
we could introduce this as an optional feature: the ini setting is 0 by
default, which will disable even the checks for recursion, so people have
to opt-in to use it and it's their burden to configure it properly.
I know that this would still allow distros and hosters to misuse it, and
probably some apps will start checking for this setting to (hopefully only)
warn about the setting being too low, or even picking a different strategy
when the setting is too low or simply increasing the limit at runtime.
maybe we can do something in the next major version which would allow us
to gracefully bail from these kind of errors without imposing arbitrary
limits?

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

11 years ago by Stas Malyshev — view source — reply

unread

Hi!

if we are concerned about the perf cost of counting and checking the
recursion, and that the default value would break some application,
maybe we could introduce this as an optional feature: the ini
setting is 0 by default, which will disable even the checks for
recursion, so people have to opt-in to use it and it's their burden
to configure it properly.

The problem here is not even that we break some app, but that stack
depths and stack requirements are very different in different OSes,
situations, etc. and that means the spaces which may break the app and
the spaces where it still crashes may intersect, so it is not at all
guaranteed that there's a predictable value for which the app always
works and the stack is still safe.

maybe we can do something in the next major version which would
allow us to gracefully bail from these kind of errors without
imposing arbitrary limits?

I don't know a way to check for potential stack depletion that would
work in a portable way. Different calls may consume different amounts of
stack space, so it's kind of hard to predict it in advance. Once it
happened, I don't think you can gracefully bail from it.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

11 years ago by Andrea Faulds — view source — reply

unread

I don't know a way to check for potential stack depletion that would
work in a portable way. Different calls may consume different amounts of
stack space, so it's kind of hard to predict it in advance. Once it
happened, I don't think you can gracefully bail from it.

Is there a way that works for each major platform? We could use stack depletion detection where available, but otherwise use the current behaviour.

Andrea Faulds
http://ajf.me/