# About restricting the recursive implicit calls - Thread: https://externals.io/message/66194 - Original archive: https://news-web.php.net/php.internals/66194 - Started by Bob Weinand on 2013-02-25T15:36:39+00:00 - Messages: 18 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #66194 — Bob Weinand — 2013-02-25T15:36:39+00:00 URL: https://externals.io/message/66194#66194 Hi! https://github.com/php/php-src/pull/290 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. --- ## #66200 — johannes@schlueters.de — 2013-02-25T16:45:38+00:00 URL: https://externals.io/message/66194#66200 · In reply to: #66194 > 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 --- ## #66202 — Bob Weinand — 2013-02-25T16:56:34+00:00 URL: https://externals.io/message/66194#66202 · In reply to: #66200 Am 25.2.2013 um 17:45 schrieb Johannes Schlüter : > > > […] > > 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 --- ## #66206 — Stas Malyshev — 2013-02-25T18:08:12+00:00 URL: https://externals.io/message/66194#66206 · In reply to: #66194 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 --- ## #66207 — Stas Malyshev — 2013-02-25T18:10:57+00:00 URL: https://externals.io/message/66194#66207 · In reply to: #66202 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 100. But if you need it, xdebug already has this option. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #66209 — Bob Weinand — 2013-02-25T18:27:34+00:00 URL: https://externals.io/message/66194#66209 · In reply to: #66206 Am 25.2.2013 um 19:08 schrieb Stas Malyshev : > Hi! > > > […] > > 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 --- ## #66210 — Bob Weinand — 2013-02-25T18:33:14+00:00 URL: https://externals.io/message/66194#66210 · In reply to: #66207 Am 25.2.2013 um 19:10 schrieb Stas Malyshev : > Hi! > > > […] > > Depth-first search in a modest-size data structure would easily go over > 100. 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 --- ## #66215 — Stas Malyshev — 2013-02-25T18:50:44+00:00 URL: https://externals.io/message/66194#66215 · In reply to: #66210 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 --- ## #66216 — Bob Weinand — 2013-02-25T19:36:07+00:00 URL: https://externals.io/message/66194#66216 Forgotten to cc to internals list: Am 25.2.2013 um 19:50 schrieb Stas Malyshev : > Hi! > > > […] > > 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 --- ## #66217 — johannes@schlueters.de — 2013-02-25T21:01:46+00:00 URL: https://externals.io/message/66194#66217 · In reply to: #66216 > > 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) --- ## #66218 — Bob Weinand — 2013-02-25T21:29:05+00:00 URL: https://externals.io/message/66194#66218 · In reply to: #66217 Am 25.2.2013 um 22:01 schrieb Johannes Schlüter : > > > […] > > Which is exactly the issue i mentioned about being a "good default" > > > […] > > 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? --- ## #66285 — Bob Weinand — 2013-02-27T18:16:26+00:00 URL: https://externals.io/message/66194#66285 · In reply to: #66218 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 --- ## #66298 — Stas Malyshev — 2013-02-27T20:06:10+00:00 URL: https://externals.io/message/66194#66298 · In reply to: #66285 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 --- ## #66300 — Ferenc Kovacs — 2013-02-27T21:15:27+00:00 URL: https://externals.io/message/66194#66300 · In reply to: #66298 On Wed, Feb 27, 2013 at 9:06 PM, Stas Malyshev wrote: > Hi! > > > […] > > 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. > > > […] > > 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 --- ## #74770 — Ferenc Kovacs — 2014-06-06T06:25:00+00:00 URL: https://externals.io/message/66194#74770 · In reply to: #66300 > > > > On Wed, Feb 27, 2013 at 9:06 PM, Stas Malyshev > wrote: > > > […] > 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 --- ## #74771 — Dmitry Stogov — 2014-06-06T06:55:59+00:00 URL: https://externals.io/message/66194#74771 · In reply to: #74770 The overhead of the patch is negligible, so it's not a big problem to include it from perf point of view, however: 1) It won't prevent crash because of C stack overflow, in case the limit is too big. 2) Disable check with some default value (0 or -1) would be also good 3) 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. > > > > > > > […] > > 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 --- ## #74772 — Stas Malyshev — 2014-06-06T07:07:00+00:00 URL: https://externals.io/message/66194#74772 · In reply to: #74771 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 --- ## #74776 — Andrea Faulds — 2014-06-06T12:55:58+00:00 URL: https://externals.io/message/66194#74776 · In reply to: #74772 > 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/