Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » PCRE JIT stack size limit »

PCRE JIT stack size limit

php.internals

Christoph Becker

11 years ago
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] <https://bugs.php.net/bug.php?id=70110>
-- Christoph M. Becker

Pierre Joye

11 years ago
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 On Jul 23, 2015 2:07 PM, "Christoph Becker" <cmbecker69@gmx.de> wrote:

Christoph Becker

11 years ago
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] <http://www.pcre.org/original/doc/html/pcrejit.html#SEC8>
-- Christoph M. Becker

Pierre Joye

11 years ago
On Jul 23, 2015 9:47 PM, "Christoph Becker" <cmbecker69@gmx.de> wrote:
> > 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] <http://www.pcre.org/original/doc/html/pcrejit.html#SEC8>
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 Becker

11 years ago
Pierre Joye wrote:
> On Jul 23, 2015 9:47 PM, "Christoph Becker" <cmbecker69@gmx.de> 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] <http://www.pcre.org/original/doc/html/pcrejit.html#SEC8> > > 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

Adam Harvey

11 years ago
On 23 July 2015 at 11:47, Christoph Becker <cmbecker69@gmx.de> wrote: <snip great explanation, thanks>
> 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

Pierre Joye

11 years ago
On Jul 24, 2015 10:34 AM, "Adam Harvey" <aharvey@php.net> wrote:
> > On 23 July 2015 at 11:47, Christoph Becker <cmbecker69@gmx.de> wrote: > <snip great explanation, thanks> > > 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?
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.

Christoph Becker

11 years ago
Adam Harvey wrote:
> On 23 July 2015 at 11:47, Christoph Becker <cmbecker69@gmx.de> wrote: > <snip great explanation, thanks> >> 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.
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

Christoph Becker

11 years ago
Christoph Becker wrote:
> Adam Harvey wrote: > >> On 23 July 2015 at 11:47, Christoph Becker <cmbecker69@gmx.de> wrote: >> <snip great explanation, thanks> >>> 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. > > 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]
>> 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.
[1] <https://github.com/php/php-src/blob/php-7.0.0beta2/ext/pcre/php_pcre.c#L436-L440>
-- Christoph M. Becker

David Zuelke

11 years ago
On 24.07.2015, at 09:33, Adam Harvey <aharvey@php.net> wrote:
> > On 23 July 2015 at 11:47, Christoph Becker <cmbecker69@gmx.de> wrote: > <snip great explanation, thanks> >> 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.
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.

Pierre Joye

11 years ago
On Jul 24, 2015 3:32 PM, "David Zuelke" <dz@heroku.com> wrote:
> > On 24.07.2015, at 09:33, Adam Harvey <aharvey@php.net> wrote: > > > > On 23 July 2015 at 11:47, Christoph Becker <cmbecker69@gmx.de> wrote: > > <snip great explanation, thanks> > >> 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. > > 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.

Christoph Becker

11 years ago
On 24.07.2015 at 14:32, David Zuelke wrote:
> On 24.07.2015, at 09:33, Adam Harvey <aharvey@php.net> wrote: >> >> On 23 July 2015 at 11:47, Christoph Becker <cmbecker69@gmx.de> wrote: >> <snip great explanation, thanks> >>> 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. > > 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

Anatol Belski

11 years ago
Hi Christoph,
> -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Thursday, July 23, 2015 8:47 PM > To: Pierre Joye <pierre.php@gmail.com> > Cc: PHP internals <internals@lists.php.net> > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > 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] <http://www.pcre.org/original/doc/html/pcrejit.html#SEC8> >
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

Christoph Becker

11 years ago
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] <http://www.pcre.org/original/doc/html/pcrestack.html> [2] <https://github.com/cmb69/php-src/commit/1546e3025403bb7ebed233c71fbc299fd584c9e3>
-- Christoph M. Becker

Anatol Belski

11 years ago
Hi Christoph,
> -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Saturday, July 25, 2015 12:09 AM > To: Anatol Belski <anatol.php@belski.net>; 'Pierre Joye' > <pierre.php@gmail.com> > Cc: 'PHP internals' <internals@lists.php.net> > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > 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. >
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

Christoph Becker

11 years ago
Hi Anatol, Anatol Belski wrote:
> Hi Christoph, > >> -----Original Message----- >> From: Christoph Becker [mailto:cmbecker69@gmx.de] >> Sent: Saturday, July 25, 2015 12:09 AM >> To: Anatol Belski <anatol.php@belski.net>; 'Pierre Joye' >> <pierre.php@gmail.com> >> Cc: 'PHP internals' <internals@lists.php.net> >> Subject: Re: [PHP-DEV] PCRE JIT stack size limit >> >> 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.
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).
>> 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.
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.
>> 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.
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] <http://www.pcre.org/original/doc/html/pcrejit.html#SEC3> [2] <http://www.pcre.org/original/doc/html/pcrejit.html#SEC8> [3] <https://github.com/cmb69/php-src/commit/78ad15d589c25b5d10aa68f5b419333f4040c16a>
-- Christoph M. Becker

Anatol Belski

11 years ago
Hi Christoph,
> -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Saturday, July 25, 2015 2:25 AM > To: Anatol Belski <anatol.php@belski.net>; 'Pierre Joye' > <pierre.php@gmail.com> > Cc: 'PHP internals' <internals@lists.php.net> > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > Hi Anatol, > > Anatol Belski wrote: > > > Hi Christoph, > > > >> -----Original Message----- > >> From: Christoph Becker [mailto:cmbecker69@gmx.de] > >> Sent: Saturday, July 25, 2015 12:09 AM > >> To: Anatol Belski <anatol.php@belski.net>; 'Pierre Joye' > >> <pierre.php@gmail.com> > >> Cc: 'PHP internals' <internals@lists.php.net> > >> Subject: Re: [PHP-DEV] PCRE JIT stack size limit > >> > >> 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. > > 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.
> > >> 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. > > 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.
> > > 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]. >
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.
> >> 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. > > 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

Christoph Becker

11 years ago
Hi Anatol, Anatol Belski wrote:
>> -----Original Message----- >> From: Christoph Becker [mailto:cmbecker69@gmx.de] >> Sent: Saturday, July 25, 2015 2:25 AM >> To: Anatol Belski <anatol.php@belski.net>; 'Pierre Joye' >> <pierre.php@gmail.com> >> Cc: 'PHP internals' <internals@lists.php.net> >> Subject: Re: [PHP-DEV] PCRE JIT stack size limit >> >> 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.
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] <https://github.com/php/php-src/blob/php-7.0.0beta2/ext/pcre/php_pcre.c#L257>
-- Christoph M. Becker

Anatol Belski

11 years ago
Hi Christoph,
> -----Original Message----- > From: Christoph Becker [mailto:cmbecker69@gmx.de] > Sent: Saturday, July 25, 2015 6:02 PM > To: Anatol Belski <anatol.php@belski.net>; 'Pierre Joye' > <pierre.php@gmail.com> > Cc: 'PHP internals' <internals@lists.php.net> > Subject: Re: [PHP-DEV] PCRE JIT stack size limit > > Hi Anatol, > > Anatol Belski wrote: > > >> -----Original Message----- > >> From: Christoph Becker [mailto:cmbecker69@gmx.de] > >> Sent: Saturday, July 25, 2015 2:25 AM > >> To: Anatol Belski <anatol.php@belski.net>; 'Pierre Joye' > >> <pierre.php@gmail.com> > >> Cc: 'PHP internals' <internals@lists.php.net> > >> Subject: Re: [PHP-DEV] PCRE JIT stack size limit > >> > >> 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. > > That would be nice. I'll make a respective PR.
Thanks.
> > > 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. >
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).
> > 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. :) >
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

Christoph Becker

11 years ago
On 23.07.2015 at 13:07, Christoph Becker wrote:
> 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] <https://bugs.php.net/bug.php?id=70110>
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