# PCRE JIT stack size limit - Thread: https://externals.io/message/87245 - Original archive: https://news-web.php.net/php.internals/87245 - Started by Christoph Becker on 2015-07-23T11:07:09+00:00 - Messages: 40 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/87245.md?page=2 - 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 --- ## #87245 — Christoph Becker — 2015-07-23T11:07:09+00:00 URL: https://externals.io/message/87245#87245 Hi! PHP7 supports PCRE's JIT compilation of patterns by default, which mostly works fine. However, there are issues when the matching exceeds the JIT stack limit, see bug #70110[1]. I'm not sure how to solve this best. Basically, I see two possible solutions: either we fall back to non JIT matching, if pcre_exec() fails with PCRE_ERROR_JIT_STACKLIMIT, or we use a custom JIT stack and make its size a configurable ini setting (similar to pcre.backtrack_limit), and raise `E_WARNING` if the matching fails due to limited stack size. Thoughts? [1] -- Christoph M. Becker --- ## #87251 — Pierre Joye — 2015-07-23T17:36:50+00:00 URL: https://externals.io/message/87245#87251 · In reply to: #87245 Hi Christoph, There are ways to dymacally increase the stack. Apache's modphp can use the apache config. Fpm, fcgi or cli can change it on windows (afair it is in the doc). It is possible too on Linux with ulimit. An alternative (not a big fan) would be to use setrlimit with an ini setting. Cheers, Pierre > Hi! > > PHP7 supports PCRE's JIT compilation of patterns by default, which > mostly works fine. However, there are issues when the matching exceeds > the JIT stack limit, see bug #70110[1]. > > I'm not sure how to solve this best. Basically, I see two possible > solutions: either we fall back to non JIT matching, if pcre_exec() fails > with PCRE_ERROR_JIT_STACKLIMIT, or we use a custom JIT stack and make > its size a configurable ini setting (similar to pcre.backtrack_limit), > and raise `E_WARNING` if the matching fails due to limited stack size. > > Thoughts? > > [1] > > -- > Christoph M. Becker --- ## #87254 — Christoph Becker — 2015-07-23T18:47:26+00:00 URL: https://externals.io/message/87245#87254 · In reply to: #87251 Hi Pierre, Pierre Joye wrote: > Hi Christoph, > > There are ways to dymacally increase the stack. Apache's modphp can use the > apache config. Fpm, fcgi or cli can change it on windows (afair it is in > the doc). It is possible too on Linux with ulimit. > > An alternative (not a big fan) would be to use setrlimit with an ini > setting. Ah, I should have explained better that libpcre's JIT support is implemented as own virtual machine, and libpcre has functions to use a custom stack which is allocated on the heap. If these functions are not used (as it's now in ext/pcre) a fixed 32K on the machine stack are used.[1] Anyhow, it is not possible to change the stack (size) during a call to pcre_exec() (unless, maybe, libpcre would be modified), and it is not possible (to my knowledge) to calculate the required stack size in advance. The required stack size depends on pattern and subject. So basically, we have to call pcre_exec(), and if it fails due to limited stack size, we either fail as well, or we try again. In the latter case we could either use a bigger stack or do without JIT. In the former case we should at least give users a setting to choose the desired stack size, which is quite comparable to pcre.backtrack_limit and pcre.recursion_limit. Therefore I tend to prefer a new ini setting (say, pcre.jitstack_limit). That would mean, however, to add yet another ini setting, of which there are already so many. [1] -- Christoph M. Becker --- ## #87260 — Pierre Joye — 2015-07-24T07:17:28+00:00 URL: https://externals.io/message/87245#87260 · In reply to: #87254 > > Hi Pierre, > > Pierre Joye wrote: > > > […] the > > […] > > Ah, I should have explained better that libpcre's JIT support is > implemented as own virtual machine, and libpcre has functions to use a > custom stack which is allocated on the heap. If these functions are not > used (as it's now in ext/pcre) a fixed 32K on the machine stack are used.[1] > > Anyhow, it is not possible to change the stack (size) during a call to > pcre_exec() (unless, maybe, libpcre would be modified), and it is not > possible (to my knowledge) to calculate the required stack size in > advance. The required stack size depends on pattern and subject. > > So basically, we have to call pcre_exec(), and if it fails due to > limited stack size, we either fail as well, or we try again. In the > latter case we could either use a bigger stack or do without JIT. In > the former case we should at least give users a setting to choose the > desired stack size, which is quite comparable to pcre.backtrack_limit > and pcre.recursion_limit. > > Therefore I tend to prefer a new ini setting (say, pcre.jitstack_limit). > That would mean, however, to add yet another ini setting, of which > there are already so many. > > [1] Only to be sure, ou mean it is a different issue than the common one (dozen of bugs for pcre about it)? Solved by increasing the stack size of the process or binary? Also an ini setting may not be portable. That's something to double check :) Cheers, Pierre > -- > Christoph M. Becker --- ## #87261 — Adam Harvey — 2015-07-24T07:33:54+00:00 URL: https://externals.io/message/87245#87261 · In reply to: #87254 > Therefore I tend to prefer a new ini setting (say, pcre.jitstack_limit). > That would mean, however, to add yet another ini setting, of which > there are already so many. I'm not a big fan of that, although it's at least in the spirit of what configuration settings are meant to be used for. What if we added the PCRE_ERROR_JIT_STACKLIMIT error constant to those exposed to userland so that it's more easily noticed via `preg_last_error()`, and adding a modifier that can be used to disable the JIT on a per-pattern basis (by setting PCRE_NO_START_OPTIMIZE, which admittedly disables other stuff too, but at least the regex will run)? At least then users could check the error when the regex fails and re-run the regex without the JIT if they chose to. How likely is the average user to hit this, do you think? Adam --- ## #87263 — Pierre Joye — 2015-07-24T08:14:05+00:00 URL: https://externals.io/message/87245#87263 · In reply to: #87261 > > > > > […] > > I'm not a big fan of that, although it's at least in the spirit of > what configuration settings are meant to be used for. > > What if we added the PCRE_ERROR_JIT_STACKLIMIT error constant to those > exposed to userland so that it's more easily noticed via > `preg_last_error()`, and adding a modifier that can be used to disable > the JIT on a per-pattern basis (by setting PCRE_NO_START_OPTIMIZE, > which admittedly disables other stuff too, but at least the regex will > run)? At least then users could check the error when the regex fails > and re-run the regex without the JIT if they chose to. > > How likely is the average user to hit this, do you think? I am not sure we have accurate numbers or can have. It really depends on the expressions. A simple one can exhaust the stack as much as a apparently more complex expressions. I have to say that the default stack we use now on windows has reduced the amount of bug reports. I do not think we can eliminate it and increase it too much may bring some bad side effects as well. --- ## #87266 — Christoph Becker — 2015-07-24T11:18:16+00:00 URL: https://externals.io/message/87245#87266 · In reply to: #87260 Pierre Joye wrote: > > > […] > used.[1] > > […] > > Only to be sure, ou mean it is a different issue than the common one (dozen > of bugs for pcre about it)? Solved by increasing the stack size of the > process or binary? I'm pretty sure that is another issue. The JIT functionality of libpcre is available only for PHP 7, and the issue reported in #70110 only happens when pcre.jit is enabled. And to my knowledge #70110 is the first reported bug that's caused by the JIT. > Also an ini setting may not be portable. That's something to double check :) ACK. However, AFAICT pcre_jit_stack_alloc() and pcre_assign_jit_stack() are supposed to be portable for systems where JIT support is available. -- Christoph M. Becker --- ## #87267 — Christoph Becker — 2015-07-24T11:28:52+00:00 URL: https://externals.io/message/87245#87267 · In reply to: #87261 Adam Harvey wrote: > > > > […] > > I'm not a big fan of that, although it's at least in the spirit of > what configuration settings are meant to be used for. > > What if we added the PCRE_ERROR_JIT_STACKLIMIT error constant to those > exposed to userland so that it's more easily noticed via > `preg_last_error()`, and adding a modifier that can be used to disable > the JIT on a per-pattern basis (by setting PCRE_NO_START_OPTIMIZE, > which admittedly disables other stuff too, but at least the regex will > run)? At least then users could check the error when the regex fails > and re-run the regex without the JIT if they chose to. Yes, that would be an option. I'm not sure if we need to touch PCRE_NO_START_OPTIMIZE (or if it would even help); simply not calling pcre_study() respectively not passing the studied extra data (second argument) to pcre_exec() should be sufficient. > How likely is the average user to hit this, do you think? I don't know. The libpcre man pages say: | JIT uses far less memory for recursion than the interpretive code, | and a maximum stack size of 512K to 1M should be more than enough for | any pattern. Of course, a hard-coded 512K or even more would be rather much. The default stack size (32K) for the regex in the test script of #70110 would be sufficient, if the subject is repeated only about 2000 times, but that doesn't say much (the `preg_match()` is rather contrived). Anyway, that shows that it's not only the pattern that's relevant for the needed JIT stack space, but also the subject string. -- Christoph M. Becker --- ## #87268 — Christoph Becker — 2015-07-24T11:45:05+00:00 URL: https://externals.io/message/87245#87268 · In reply to: #87267 Christoph Becker wrote: > Adam Harvey wrote: > > > […] > > Yes, that would be an option. I'm not sure if we need to touch > PCRE_NO_START_OPTIMIZE (or if it would even help); simply not calling > pcre_study() respectively not passing the studied extra data (second > argument) to pcre_exec() should be sufficient. Correction: we simply should not pass PCRE_STUDY_JIT_COMPILE to pcre_study() if we want to disable JIT support for individual regexps.[1] > > […] > > I don't know. The libpcre man pages say: > > | JIT uses far less memory for recursion than the interpretive code, > | and a maximum stack size of 512K to 1M should be more than enough for > | any pattern. > > Of course, a hard-coded 512K or even more would be rather much. The > default stack size (32K) for the regex in the test script of #70110 > would be sufficient, if the subject is repeated only about 2000 times, > but that doesn't say much (the `preg_match()` is rather contrived). > > Anyway, that shows that it's not only the pattern that's relevant for > the needed JIT stack space, but also the subject string. [1] -- Christoph M. Becker --- ## #87270 — David Zuelke — 2015-07-24T12:32:21+00:00 URL: https://externals.io/message/87245#87270 · In reply to: #87261 > > > > > […] > > I'm not a big fan of that, although it's at least in the spirit of > what configuration settings are meant to be used for. > > What if we added the PCRE_ERROR_JIT_STACKLIMIT error constant to those > exposed to userland so that it's more easily noticed via > `preg_last_error()`, and adding a modifier that can be used to disable > the JIT on a per-pattern basis (by setting PCRE_NO_START_OPTIMIZE, > which admittedly disables other stuff too, but at least the regex will > run)? At least then users could check the error when the regex fails > and re-run the regex without the JIT if they chose to. But this might mean that patterns which previously worked, because no JIT was used, suddenly fail in existing code with a new error constant. Which I guess is a BC break. --- ## #87271 — Pierre Joye — 2015-07-24T12:58:05+00:00 URL: https://externals.io/message/87245#87271 · In reply to: #87270 > > > > […] pcre.jitstack_limit). > > […] > > But this might mean that patterns which previously worked, because no JIT was used, suddenly fail in existing code with a new error constant. Which I guess is a BC break. Such things happened in the past with normal updates, both ways. Simple code changes, fixes etc may have led to changes in the stack usage. I can remember issues with some specific updates. --- ## #87272 — Christoph Becker — 2015-07-24T12:58:28+00:00 URL: https://externals.io/message/87245#87272 · In reply to: #87270 > > > […] > > But this might mean that patterns which previously worked, because no JIT was used, suddenly fail in existing code with a new error constant. Which I guess is a BC break. Maybe it's best to change the default of pcre.jit to "0"? -- Christoph M. Becker --- ## #87275 — Christoph Becker — 2015-07-24T18:14:40+00:00 URL: https://externals.io/message/87245#87275 · In reply to: #87245 > PHP7 supports PCRE's JIT compilation of patterns by default, which > mostly works fine. However, there are issues when the matching exceeds > the JIT stack limit, see bug #70110[1]. > > I'm not sure how to solve this best. Basically, I see two possible > solutions: either we fall back to non JIT matching, if pcre_exec() fails > with PCRE_ERROR_JIT_STACKLIMIT, or we use a custom JIT stack and make > its size a configurable ini setting (similar to pcre.backtrack_limit), > and raise `E_WARNING` if the matching fails due to limited stack size. > > Thoughts? > > [1] Zoltán Herczeg just told me that the size of the JIT stack doesn't have to be fixed, but rather allows to dynamically grow up to a maximum given size (preallocated by mmap/VirtualAlloc). I'll do some testing and report back later. -- Christoph M. Becker --- ## #87276 — Anatol Belski — 2015-07-24T19:11:55+00:00 URL: https://externals.io/message/87245#87276 · In reply to: #87254 Hi Christoph, > -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Thursday, July 23, 2015 8:47 PM > To: Pierre Joye > Cc: PHP internals > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > Hi Pierre, > > Pierre Joye wrote: > > > […] > > Ah, I should have explained better that libpcre's JIT support is implemented as > own virtual machine, and libpcre has functions to use a custom stack which is > allocated on the heap. If these functions are not used (as it's now in ext/pcre) a > fixed 32K on the machine stack are used.[1] > > Anyhow, it is not possible to change the stack (size) during a call to > pcre_exec() (unless, maybe, libpcre would be modified), and it is not possible (to > my knowledge) to calculate the required stack size in advance. The required > stack size depends on pattern and subject. > > So basically, we have to call pcre_exec(), and if it fails due to limited stack size, > we either fail as well, or we try again. In the latter case we could either use a > bigger stack or do without JIT. In the former case we should at least give users a > setting to choose the desired stack size, which is quite comparable to > pcre.backtrack_limit and pcre.recursion_limit. > > Therefore I tend to prefer a new ini setting (say, pcre.jitstack_limit). > That would mean, however, to add yet another ini setting, of which there are > already so many. > > [1] > This looks like an extremely fragile topic because it depends on how much stack is available to an executable. A custom JIT stack can behave more stable but cannot be resized. And the main issue is that the JIT stack size, machine stack size and ext/pcre cache size are completely unrelated terms. For example, a binary can have not enough stack, but the custom JIT stack using mmap/VirtualAlloc could even succeed, but then pcre_exec will be executed and overflow the machine stack. We can never know which one is exhausted first - the one for the JIT compilation or the other one for the execution, or vice versa. Generally, moving the JIT compilation away from the machine stack and increasing the PCRE cache size should be more stable against this . However it's an edge case. IMHO we should not do it just to fix some crazy usage. Users who need it might just turn off JIT. Normal usage seems not to be affected, say loading some sane functional script, which FE is done by any benchmark with WP, Symfony, etc. But moving JIT compilation away from the machine stack wil lpossibly affect it. Regards Anatol --- ## #87277 — Christoph Becker — 2015-07-24T22:09:14+00:00 URL: https://externals.io/message/87245#87277 · In reply to: #87276 Hi Anatol, Anatol Belski wrote: > This looks like an extremely fragile topic because it depends on how > much stack is available to an executable. A custom JIT stack can > behave more stable but cannot be resized. And the main issue is that > the JIT stack size, machine stack size and ext/pcre cache size are > completely unrelated terms. For example, a binary can have not enough > stack, but the custom JIT stack using mmap/VirtualAlloc could even > succeed, but then pcre_exec will be executed and overflow the machine > stack. We can never know which one is exhausted first - the one for > the JIT compilation or the other one for the execution, or vice > versa. > > Generally, moving the JIT compilation away from the machine stack and > increasing the PCRE cache size should be more stable against this . > However it's an edge case. IMHO we should not do it just to fix some > crazy usage. Users who need it might just turn off JIT. Normal usage > seems not to be affected, say loading some sane functional script, > which FE is done by any benchmark with WP, Symfony, etc. But moving > JIT compilation away from the machine stack wil lpossibly affect it. Beforehand, I'm not suggesting to change anything regarding our PCRE cache (PCRE_G(pcre_cache)); this seems to be fine as it is, and is indeed not related to this topic. Now please consider the following simple expression: preg_match('/^(foo)+$/', str_repeat('foo', $n)) This will fail (i.e. yield FALSE) independently of pcre.jit for large enough $n. However, a user can change pcre.recursion_limit what will affect the $n limit (the expression will fail for smaller or larger $n), if pcre.jit=0. If pcre.jit=1 the user can't influence this boundary in any way, currently. And maybe even worse, with pcre.jit=0 the boundary is 50,000, but with pcre.jit=1 it is only 1,366. Of course, one can argue that this is a contrived example, and that such usage is crazy, but why do we have a default pcre.recursion_limit of 100,000 then? A recursion_limit of 2,734 would be sufficient to have a boundary of $n == 1,366. All in all, as this example already suggests, classic execution of matching is done by recursive calls (using normal stack frames), while JIT execution of matching is iterative, using a special JIT stack.[1] I don't think it is justified to give users a setting to adjust for the former, but not for the latter (except to disable JIT, albeit JIT might bring quite some boost especially for such cases). As we're pretty late in the game for PHP 7.0, it might be best to postpone a new ini setting or other changes to PHP 7.1, but at the very least I would introduce a new error constant, say PHP_PCRE_JIT_STACKLIMIT_ERROR[2], so users get a more meaningful result when calling `preg_last_error()` than PHP_PCRE_INTERNAL_ERROR. And it seems to be appropriate to add a note to UPGRADING that pcre.jit=1 may cause some preg_*() to fail which would work with pcre.jit=0. [1] [2] -- Christoph M. Becker --- ## #87278 — Anatol Belski — 2015-07-24T23:10:22+00:00 URL: https://externals.io/message/87245#87278 · In reply to: #87277 Hi Christoph, > -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Saturday, July 25, 2015 12:09 AM > To: Anatol Belski ; 'Pierre Joye' > > Cc: 'PHP internals' > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > Hi Anatol, > > Anatol Belski wrote: > > > […] > > Beforehand, I'm not suggesting to change anything regarding our PCRE cache > (PCRE_G(pcre_cache)); this seems to be fine as it is, and is indeed not related to > this topic. > > Now please consider the following simple expression: > > preg_match('/^(foo)+$/', str_repeat('foo', $n)) > > This will fail (i.e. yield FALSE) independently of pcre.jit for large enough $n. > However, a user can change pcre.recursion_limit what will affect the $n limit > (the expression will fail for smaller or larger $n), if pcre.jit=0. If pcre.jit=1 the > user can't influence this boundary in any way, currently. > > And maybe even worse, with pcre.jit=0 the boundary is 50,000, but with > pcre.jit=1 it is only 1,366. Of course, one can argue that this is a contrived > example, and that such usage is crazy, but why do we have a default > pcre.recursion_limit of 100,000 then? A recursion_limit of > 2,734 would be sufficient to have a boundary of $n == 1,366. > The 100000 is an empirical value by my guess. It regards to the default stack sizes on different platforms and to an average pattern. There is no prediction that PCRE will not exhaust the stack available to the binary. > All in all, as this example already suggests, classic execution of matching is done > by recursive calls (using normal stack frames), while JIT execution of matching is > iterative, using a special JIT stack.[1] I don't think it is justified to give users a > setting to adjust for the former, but not for the latter (except to disable JIT, > albeit JIT might bring quite some boost especially for such cases). > JIT without custom stack will use "32k of machine stack", by the doc. We currently don't really give users a choice to choose between iterative and recursive PCRE execution, it's a compile time decision. And this is again because an iterative execution will be safer, but will affect an average case with unnecessary thrift. In this JIT case, one could give this choice, you're right, but we should evaluate it carefully. Fe what happens to the PHP pattern cache if JIT stack is exhausted? What I was more precisely talking about is like If (false === preg_match(",evil pattern,", ...)) { Ini_set("pcre.jit", 0); // retry } So received error - no JIT. And no additional logic/overhead in ext/pcre. Maybe custom JIT were eligible in this case, but according to the PCRE doc it can easily bring issues as the global JIT memory can't be resized/migrated just by one's finger click. Say one would be forced to either use the custom JIT stack or default JIT stack from the start on. This can end up with the over complication using a custom JIT stack for a particular pattern. > As we're pretty late in the game for PHP 7.0, it might be best to postpone a new > ini setting or other changes to PHP 7.1, but at the very least I would introduce a > new error constant, say PHP_PCRE_JIT_STACKLIMIT_ERROR[2], so users get a > more meaningful result when calling `preg_last_error()` than > PHP_PCRE_INTERNAL_ERROR. And it seems to be appropriate to add a note to > UPGRADING that pcre.jit=1 may cause some preg_*() to fail which would work > with pcre.jit=0. > Yes, instead of returning false one could return an explicit error, or indicate in any other ways. Thanks for bringing it up and maybe we'll have more info after your further investigation. But documentation is appropriate in any cases. Regards Anatol --- ## #87282 — Christoph Becker — 2015-07-25T00:25:29+00:00 URL: https://externals.io/message/87245#87282 · In reply to: #87278 Hi Anatol, Anatol Belski wrote: > Hi Christoph, > > > […] > > The 100000 is an empirical value by my guess. It regards to the default stack sizes on different platforms and to an average pattern. There is no prediction that PCRE will not exhaust the stack available to the binary. ACK. However, the user is able to tune this value according to the environment. And yes, I'm aware that pcre.recursion_limit may be necessary to prevent a stack overflow, what can't happen with JIT compilation (if the JIT stack is exhausted, pcre_exec() returns gracefully), but nonetheless giving users some control over the size of the JIT stack seems to be appropriate (at least in the long run, aka. PHP 7.1). > > […] > > JIT without custom stack will use "32k of machine stack", by the doc. We currently don't really give users a choice to choose between iterative and recursive PCRE execution, it's a compile time decision. Well, one can enforce non JIT execution by setting pcre.jit=0. pcre.jit=1 indeed may not enforce JIT execution, depending on the platform[1] and the libpcre version. > And this is again because an iterative execution will be safer, but will affect an average case with unnecessary thrift. In this JIT case, one could give this choice, you're right, but we should evaluate it carefully. Fe what happens to the PHP pattern cache if JIT stack is exhausted? That is unrelated. It is possible to use a single custom PCRE JIT stack for all patterns (actually, pcre_exec() calls). Only if multi-threading comes into play, each thread should have its own stack (that's not absolutely necessary, but it simplifies things a lot).[2] Therefore having a single PCRE_G(jit_stack) would be sufficient. I've committed a naive draft to my php-src fork[3]. > What I was more precisely talking about is like > > If (false === preg_match(",evil pattern,", ...)) { > Ini_set("pcre.jit", 0); > // retry > } > > So received error - no JIT. And no additional logic/overhead in ext/pcre. Yes, that is a viable way for userland (even if I would suggest to test against pcre_last_error(); `preg_match()` may return `FALSE` for several error conditions, such as PHP_PCRE_BAD_UTF8_ERROR, and then retrying with pcre.jit=0 wouldn't help). We could as well put the same logic into ext/pcre (but I do not suggest to do so, even though I mentioned this as option in the OP). > Maybe custom JIT were eligible in this case, but according to the PCRE doc it can easily bring issues as the global JIT memory can't be resized/migrated just by one's finger click. Say one would be forced to either use the custom JIT stack or default JIT stack from the start on. This can end up with the over complication using a custom JIT stack for a particular pattern. I would use the same JIT stack for all patterns (whether that is the default JIT stack, or a custom one). The libpcre manual states[2]: | You may safely use the same JIT stack for more than one pattern | (either by assigning directly or by callback), as long as the | patterns are all matched sequentially in the same thread. To my knowledge in ext/pcre all patterns are matched sequentially, but I'll double-check, and I'll inquire on the libpcre mailing list. > > […] > > Yes, instead of returning false one could return an explicit error, or indicate in any other ways. I would not suggest to change the return value of `preg_match()` and friends. `FALSE` seems to be appropriate here. Instead I suggest to add a new error constant which would be returned from `preg_last_error()`. > Thanks for bringing it up and maybe we'll have more info after your further investigation. But documentation is appropriate in any cases. I'll make a respective PR after having checked back with the libpcre devs. [1] [2] [3] -- Christoph M. Becker --- ## #87290 — Anatol Belski — 2015-07-25T14:23:00+00:00 URL: https://externals.io/message/87245#87290 · In reply to: #87282 Hi Christoph, > -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Saturday, July 25, 2015 2:25 AM > To: Anatol Belski ; 'Pierre Joye' > > Cc: 'PHP internals' > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > Hi Anatol, > > Anatol Belski wrote: > > > […] > currently. > > […] > sizes on different platforms and to an average pattern. There is no prediction > that PCRE will not exhaust the stack available to the binary. > > ACK. However, the user is able to tune this value according to the environment. > And yes, I'm aware that pcre.recursion_limit may be necessary to prevent a > stack overflow, what can't happen with JIT compilation (if the JIT stack is > exhausted, pcre_exec() returns gracefully), but nonetheless giving users some > control over the size of the JIT stack seems to be appropriate (at least in the > long run, aka. > PHP 7.1). Yep, I see your point about the config option. That would only work at MINIT stage. Though we also don't give many other options to the user land, like the pattern cache size. Which is IMHO fine because users would normally not need such nternal detail. > > > […] > might bring quite some boost especially for such cases). > > […] > currently don't really give users a choice to choose between iterative and > recursive PCRE execution, it's a compile time decision. > > Well, one can enforce non JIT execution by setting pcre.jit=0. > pcre.jit=1 indeed may not enforce JIT execution, depending on the platform[1] > and the libpcre version. But we're discussing only the cases where JIT is available, that's what it is about. No need to care about it otherwise. > > > […] > average case with unnecessary thrift. In this JIT case, one could give this choice, > you're right, but we should evaluate it carefully. Fe what happens to the PHP > pattern cache if JIT stack is exhausted? > > That is unrelated. It is possible to use a single custom PCRE JIT stack for all > patterns (actually, pcre_exec() calls). Only if multi-threading comes into play, > each thread should have its own stack (that's not absolutely necessary, but it > simplifies things a lot).[2] Therefore having a single PCRE_G(jit_stack) would be > sufficient. I've committed a naive draft to my php-src fork[3]. > If it's going to happen, of course it should be a single JIT stack for all patterns per thread. PCRE_G is required, otherwise some locking has to be implemented manually. But what I merely wanted to illustrate is exactly what you say - there are unrelated things which can go wrong but possibly affect/cause each other. > I would use the same JIT stack for all patterns (whether that is the default JIT > stack, or a custom one). The libpcre manual states[2]: > > | You may safely use the same JIT stack for more than one pattern > | (either by assigning directly or by callback), as long as the patterns > | are all matched sequentially in the same thread. > > To my knowledge in ext/pcre all patterns are matched sequentially, but I'll > double-check, and I'll inquire on the libpcre mailing list. > Sure, just after MINIT it cannot be reassigned/resized/changed. Thus, given something like pcre.jit_stack_size INI were implemented, if it's requested under 32kb - nothing should be done, machine stack should be used. If it were over it - then custom JIT stack based on mmap/VirtualAlloc. But frankly I don't see an essential difference to the handling in the user space. Yes, it might be an advantage to still use JIT, but it moves away from the machine stack. And IMHO as it's an edge case, fixing it were fine but should not be done on the cost of much over complication and affecting normal usage. From what is to see right now - sane usage is not affected while improved by JIT. If you're in the mood to add a note to upgrading, it'll be probably just file for now - disabling JIT retains the full PHP5 compatible behavior. > > […] > any other ways. > > I would not suggest to change the return value of `preg_match()` and friends. > `FALSE` seems to be appropriate here. Instead I suggest to add a new error > constant which would be returned from `preg_last_error()`. > Yep, sounds feasible. At least users can be informed about this kind of error, seems a constant is feasible for 7.0. Btw I've just noticed that the snippet I've posted earlier won't currently work. If (false === preg_match(",evil pattern,", ...)) { If (OUT_OF_JIT_STACK == pcre_last_error()) { ini_set("pcre.jit", 0); // retry } } The retry would fail even when JIT is disabled on demand. This is actually rather unexpected, maybe something in PCRE JIT state cannot be recovered once JIT compilation has failed. But actually, even without running into debugging, it is also a bad sign telling that the behavior can get complicated ( Running for the INI option and some custom JIT stack functionality needs more brainstorming and test. Please ping when you have an initial implementation, I'd be glad to help testing, etc. anyway. Regards Anatol --- ## #87291 — Christoph Becker — 2015-07-25T16:01:42+00:00 URL: https://externals.io/message/87245#87291 · In reply to: #87290 Hi Anatol, Anatol Belski wrote: > > […] > Yep, sounds feasible. At least users can be informed about this kind of error, seems a constant is feasible for 7.0. That would be nice. I'll make a respective PR. > Btw I've just noticed that the snippet I've posted earlier won't currently work. > > If (false === preg_match(",evil pattern,", ...)) { > If (OUT_OF_JIT_STACK == pcre_last_error()) { > ini_set("pcre.jit", 0); > // retry > } > } > > The retry would fail even when JIT is disabled on demand. This is actually rather unexpected, maybe something in PCRE JIT state cannot be recovered once JIT compilation has failed. But actually, even without running into debugging, it is also a bad sign telling that the behavior can get complicated ( Indeed! That's caused by our PCRE cache. The cached regex is alread studied with JIT info, as all that happens in pcre_get_compiled-regex_cache[1]. So switching pcre.jit at runtime may not have the desired effect. I'm not sure if that has to be regarded as bug, and whether we should do something about it. One solution might be to clear the regex cache whenever the ini setting changes. I'll have a closer look. > Running for the INI option and some custom JIT stack functionality needs more brainstorming and test. Please ping when you have an initial implementation, I'd be glad to help testing, etc. anyway. All in all it seems that we probably should have an RFC for that. I'll consider to write one, but there's no need to hurry – too late for 7.0 anyway. :) [1] -- Christoph M. Becker --- ## #87302 — Anatol Belski — 2015-07-26T19:31:36+00:00 URL: https://externals.io/message/87245#87302 · In reply to: #87291 Hi Christoph, > -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Saturday, July 25, 2015 6:02 PM > To: Anatol Belski ; 'Pierre Joye' > > Cc: 'PHP internals' > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > Hi Anatol, > > Anatol Belski wrote: > > > […] > seems a constant is feasible for 7.0. > > That would be nice. I'll make a respective PR. Thanks. > > > […] > > Indeed! That's caused by our PCRE cache. The cached regex is alread studied > with JIT info, as all that happens in pcre_get_compiled-regex_cache[1]. So > switching pcre.jit at runtime may not have the desired effect. I'm not sure if that > has to be regarded as bug, and whether we should do something about it. One > solution might be to clear the regex cache whenever the ini setting changes. I'll > have a closer look. > Good catch. Maybe another option could be implementing the PHP pattern above In C. Say - check what pcre_exec delivered - if it’s the JIT stack error - look for the pattern in the cache - if it's found - remove it from the cache - recompile and cache - exec a non JIT pattern version However this would silently ignore JIT, not transparent for user. Maybe another option could be introducing a user space function to clear a particular pattern from the cache. When expunging the full cache, it'll for sure have a negative performance effect when the cache isn't empty. But it's actually logic as one can base it on the idea - either JIT is enabled for everything, or it is not (for everything). > > […] > more brainstorming and test. Please ping when you have an initial > implementation, I'd be glad to help testing, etc. anyway. > > All in all it seems that we probably should have an RFC for that. I'll consider to > write one, but there's no need to hurry – too late for 7.0 anyway. :) > Yep, the whole needs a good consideration about how to do it best without a big impact on the user side. Also probably it could be worth it to check how PCRE2 behaves in this cases, maybe it were worthwhile to upgrade to PCRE2 for 7.1. Regards Anatol --- ## #87307 — Christoph Becker — 2015-07-26T22:05:59+00:00 URL: https://externals.io/message/87245#87307 · In reply to: #87302 Hi Anatol, Anatol Belski wrote: > Hi Christoph, > > > […] > Thanks. > > > […] > Good catch. Maybe another option could be implementing the PHP pattern above > In C. Say > > - check what pcre_exec delivered > - if it’s the JIT stack error - look for the pattern in the cache > - if it's found - remove it from the cache > - recompile and cache > - exec a non JIT pattern version > > However this would silently ignore JIT, not transparent for user. Maybe another > option could be introducing a user space function to clear a particular pattern from > the cache. When expunging the full cache, it'll for sure have a negative performance > effect when the cache isn't empty. But it's actually logic as one can base it on the idea - > either JIT is enabled for everything, or it is not (for everything). Definitely interesting ideas. However, currently I'm leaning towards a full-fledged solution in PHP 7.1 (whatever that may be), and to only add the error constant for PHP 7.0. Users can set pcre.jit=0 in php.ini and not change the setting during runtime, if necessary. FWIW, some hours ago I've implemented the clean whole cache solution: . > > […] > Yep, the whole needs a good consideration about how to do it best without a big > impact on the user side. Also probably it could be worth it to check how PCRE2 > behaves in this cases, maybe it were worthwhile to upgrade to PCRE2 for 7.1. Indeed, offering support for PCRE2 should be considered for 7.1. And also support of the JIT fast path API: . Regards -- Christoph M. Becker --- ## #87317 — Anatol Belski — 2015-07-27T11:28:09+00:00 URL: https://externals.io/message/87245#87317 · In reply to: #87307 Hi Christoph, > -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Monday, July 27, 2015 12:06 AM > To: Anatol Belski ; 'Pierre Joye' > > Cc: 'PHP internals' > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > Definitely interesting ideas. However, currently I'm leaning towards a full- > fledged solution in PHP 7.1 (whatever that may be), and to only add the error > constant for PHP 7.0. Users can set pcre.jit=0 in php.ini and not change the > setting during runtime, if necessary. > > FWIW, some hours ago I've implemented the clean whole cache solution: > src/commit/c41d78046da48db2fa8f5bd82fd6ac58099288f8>. > I wrote this script to test it https://gist.github.com/weltling/cd7f8127378989cca87c , but as expected - cache reset make a significant impact when the cache is full. Though, maybe as it's an exceptional situation, it were the simplest solution. Because it does not affect the normal usage, maybe it's ok for 7.0. Maybe we should give this flexibility, still in doubt. But the constant IMHO should be done anyway. > > […] > upgrade to PCRE2 for 7.1. > > Indeed, offering support for PCRE2 should be considered for 7.1. And also > support of the JIT fast path API: > . > Yes, maybe we'll have to support both for a while because PCRE2 is quite new so it'll need quite some time distros catch up. Thanks Anatol --- ## #87590 — Christoph Becker — 2015-08-04T00:34:57+00:00 URL: https://externals.io/message/87245#87590 · In reply to: #87317 > > […] > > I wrote this script to test it https://gist.github.com/weltling/cd7f8127378989cca87c , but as expected - cache reset make a significant impact when the cache is full. Though, maybe as it's an exceptional situation, it were the simplest solution. Because it does not affect the normal usage, maybe it's ok for 7.0. Maybe we should give this flexibility, still in doubt. But the constant IMHO should be done anyway. IMHO we shouldn't introduce the cache clearing. As your test script shows, the performance might suffer considerably when switching pcre.jit in a running script. Furthermore cached regexps currently in use (e.g. via preg_replace_callback) can't be cleared, what might lead to confusion (in very rare cases, though). As it is now users can set pcre.jit=0 and so will retain full compatibility with PHP 5. If they're running with pcre.jit=1, they're able to get a meaningful error (PREG_JIT_STACKLIMIT_ERROR), if the default stack is not large enough (what indeed might rarely be the case). If we leave it as this, I suggest to make pcre.jit PHP_INI_PERDIR instead of PHP_INI_ALL for PHP 7.0. That still gives users the option to disable it (ideally even on shared hosting), but avoids issues when dynamically switching the setting with regard to our regexp caching. If we find a better solution for PHP 7.1, we still can extend it to PHP_INI_ALL without a BC break. For PHP 7.1 it might be worthwhile to consider to store jitted resp. non-jitted regexps in the cache, and to fetch what fits to the current setting of pcre.jit (and otherwise to compile again, adding the new entry to the cache, or maybe replacing it). -- Christoph M. Becker --- ## #87596 — Anatol Belski — 2015-08-04T09:24:00+00:00 URL: https://externals.io/message/87245#87596 · In reply to: #87590 Hi Christoph, > -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Tuesday, August 4, 2015 2:35 AM > To: Anatol Belski ; 'Christoph Becker' > ; 'Pierre Joye' > Cc: 'PHP internals' > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > > > > […] > https://gist.github.com/weltling/cd7f8127378989cca87c , but as expected - > cache reset make a significant impact when the cache is full. Though, maybe as > it's an exceptional situation, it were the simplest solution. Because it does not > affect the normal usage, maybe it's ok for 7.0. Maybe we should give this > flexibility, still in doubt. But the constant IMHO should be done anyway. > > IMHO we shouldn't introduce the cache clearing. As your test script shows, the > performance might suffer considerably when switching pcre.jit in a running > script. Furthermore cached regexps currently in use (e.g. > via preg_replace_callback) can't be cleared, what might lead to confusion (in > very rare cases, though). > > As it is now users can set pcre.jit=0 and so will retain full compatibility with PHP > 5. If they're running with pcre.jit=1, they're able to get a meaningful error > (PREG_JIT_STACKLIMIT_ERROR), if the default stack is not large enough (what > indeed might rarely be the case). > Same opinion here. > If we leave it as this, I suggest to make pcre.jit PHP_INI_PERDIR instead of > PHP_INI_ALL for PHP 7.0. That still gives users the option to disable it (ideally > even on shared hosting), but avoids issues when dynamically switching the > setting with regard to our regexp caching. If we find a better solution for PHP > 7.1, we still can extend it to PHP_INI_ALL without a BC break. > That would disable possibility to change it in the registry which is PHP_INI_USER. That's something about shared hosting, too. > For PHP 7.1 it might be worthwhile to consider to store jitted resp. > non-jitted regexps in the cache, and to fetch what fits to the current setting of > pcre.jit (and otherwise to compile again, adding the new entry to the cache, or > maybe replacing it). > Not sure to understand what you mean. Storing every pattern in both variants studied with and without JIT? Regards Anatol --- ## #87597 — Christoph Becker — 2015-08-04T11:15:46+00:00 URL: https://externals.io/message/87245#87597 · In reply to: #87596 Hi Anatol, > Hi Christoph, > > > […] > That would disable possibility to change it in the registry which is PHP_INI_USER. That's something about shared hosting, too. Ah, I see. Then we ought to prominently document that chaning the setting during script execution might not have the desired effect due to internal caching. > > […] > Not sure to understand what you mean. Storing every pattern in both variants studied with and without JIT? I didn't mean to store all patterns in both variants, but rather as requested. Say, the PHP script starts with pcre.jit=0, so all patterns will be stored studied without JIT. Then the script changes to pcre.jit=1. Following cache lookups check whether an already cached pattern was studied with JIT. If not, a new cache entry is made with the pattern studied with JIT (that new entry may replace the old one or not; not sure what's best). Changing back to pcre.jit=0 will again keep the old patterns, but ensures that only those which have been studied without JIT are retrieved from the cache. -- Christoph M. Becker --- Next page: https://externals.io/message/87245.md?page=2