PCRE JIT stack size limit View as Markdown

Christoph Becker wrote:

Adam Harvey 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

<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.

<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

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