Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87302
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
To: "'Christoph Becker'" <cmbecker69@gmx.de>,
	"'Pierre Joye'" <pierre.php@gmail.com>
Cc: "'PHP internals'" <internals@lists.php.net>
References: <55B0CADD.8000807@gmx.de> <CAEZPtU7aPzPQ=DL5fn9fOEuU_hySm5WXUkbjFw+Sq0T5xNfb3A@mail.gmail.com> <55B136BE.8010006@gmx.de> <046401d0c644$9cf76880$d6e63980$@belski.net> <55B2B78A.4080202@gmx.de> <048001d0c665$ed19ec40$c74dc4c0$@belski.net> <55B2D779.9090409@gmx.de> <04d301d0c6e5$6b28f0c0$417ad240$@belski.net> <55B3B2E6.7080905@gmx.de>
In-Reply-To: <55B3B2E6.7080905@gmx.de>
Date: Sun, 26 Jul 2015 21:31:36 +0200
Message-ID: <056e01d0c7d9$b1edd990$15c98cb0$@belski.net>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQHD9RJvenw/Ss5kSkuHSfZFOrs2uQBWRtPIAqC4oYQC1yvg/gL/4swBAjr9PxcCDsvXWAGHs1w9AZmBwtqdhfuvIA==
Content-Language: en-us
Subject: RE: [PHP-DEV] PCRE JIT stack size limit
From: anatol.php@belski.net ("Anatol Belski")

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
>=20
> Hi Anatol,
>=20
> Anatol Belski wrote:
>=20
> >> -----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.
>=20
> That would be nice.  I'll make a respective PR.
Thanks.

>=20
> > Btw I've just noticed that the snippet I've posted earlier won't =
currently work.
> >
> > If (false =3D=3D=3D preg_match(",evil pattern,", ...)) {
> >                If (OUT_OF_JIT_STACK =3D=3D 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 (
>=20
> 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.
>=20
Good catch. Maybe another option could be implementing the PHP pattern =
above
In C. Say

- check what pcre_exec delivered
- if it=E2=80=99s 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=20
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).=20

> > 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.
>=20
> 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 =E2=80=93 too late for 7.0 =
anyway. :)
>=20
Yep, the whole needs a good consideration about how to do it best =
without a big=20
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