[RFC,v2] grep: allow for run time disabling of JIT in PCRE
diff mbox series

Message ID 20190729105955.44390-1-carenas@gmail.com
State New
Headers show
Series
  • [RFC,v2] grep: allow for run time disabling of JIT in PCRE
Related show

Commit Message

Carlo Marcelo Arenas Belón July 29, 2019, 10:59 a.m. UTC
PCRE1 allowed for a compile time flag to disable JIT, but PCRE2 never
had one, forcing the use of JIT if -P was requested.

After ed0479ce3d (Merge branch 'ab/no-kwset' into next, 2019-07-15)
the PCRE2 engine will be used more broadly and therefore adding this
knob will allow users a escape from situations where JIT might be
problematic.

JIT will be used by default but it can be disabled with the --no-pcre-jit
option in `git grep` or by setting 0/false into the pcre.jit config.

If a value of -1 is used instead then the following error is prevented by
using the interpreter when a JIT failure consistent with known security
restrictions is found at regex compilation time.

  $ git grep 'foo bar'
  fatal: Couldn't JIT the PCRE2 pattern 'foo bar', got '-48'

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
V2: add command line to grep as suggested by Junio

 Documentation/git-grep.txt | 11 +++++++++++
 builtin/grep.c             |  4 ++++
 grep.c                     | 30 ++++++++++++++++++++++++++----
 grep.h                     |  1 +
 4 files changed, 42 insertions(+), 4 deletions(-)


base-commit: 870eea81669bfff4333b37b11fedd870cd05fd90

Comments

Carlo Marcelo Arenas Belón July 29, 2019, 11:33 a.m. UTC | #1
Known Issues:
* PCRE1 is broken, but fixing it would make more sense on top of the
topic[1] (not in pu)
* it depends on the current ab/pcre-jit-fixes that is missing 1
critical commit in pu
* no tests yet; would need to extend it on top of the debug from Beat
and test-tool changes from Ævar, neither of which are final
* need to build on top of pu and will need further changes to be ready
for next/master/maint

The code has been tested in OpenBSD with PCRE2 (latest from svn, but
any version should work if they are JIT enabled), it is
expected to also work in NetBSD (even with PAX enabled kernels) and
macOS 10.13.6 but haven't yet tested them.  HardenedBSD
will likely segfault unless pcre.jit=0 as described in the original report[2]

Testing with SElinux and PAX enabled for Linux encouraged

Carlo

[1] https://public-inbox.org/git/20190726202642.7986-1-carenas@gmail.com/
[2] https://public-inbox.org/git/20181209230024.43444-1-carenas@gmail.com/
René Scharfe July 29, 2019, 3:11 p.m. UTC | #2
Am 29.07.19 um 12:59 schrieb Carlo Marcelo Arenas Belón:
> PCRE1 allowed for a compile time flag to disable JIT, but PCRE2 never
> had one, forcing the use of JIT if -P was requested.
>
> After ed0479ce3d (Merge branch 'ab/no-kwset' into next, 2019-07-15)
> the PCRE2 engine will be used more broadly and therefore adding this
> knob will allow users a escape from situations where JIT might be
> problematic.
>
> JIT will be used by default but it can be disabled with the --no-pcre-jit
> option in `git grep` or by setting 0/false into the pcre.jit config.
>
> If a value of -1 is used instead then the following error is prevented by
> using the interpreter when a JIT failure consistent with known security
> restrictions is found at regex compilation time.
>
>   $ git grep 'foo bar'
>   fatal: Couldn't JIT the PCRE2 pattern 'foo bar', got '-48'
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> V2: add command line to grep as suggested by Junio
>
>  Documentation/git-grep.txt | 11 +++++++++++
>  builtin/grep.c             |  4 ++++
>  grep.c                     | 30 ++++++++++++++++++++++++++----
>  grep.h                     |  1 +
>  4 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index c89fb569e3..895c6b34ec 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -13,6 +13,7 @@ SYNOPSIS
>  	   [-v | --invert-match] [-h|-H] [--full-name]
>  	   [-E | --extended-regexp] [-G | --basic-regexp]
>  	   [-P | --perl-regexp]
> +	   [-j | --[no]-pcre-jit]

Do users care?  Enough so to add a short option for this?

>  	   [-F | --fixed-strings] [-n | --line-number] [--column]
>  	   [-l | --files-with-matches] [-L | --files-without-match]
>  	   [(-O | --open-files-in-pager) [<pager>]]
> @@ -69,6 +70,12 @@ grep.fallbackToNoIndex::
>  	If set to true, fall back to git grep --no-index if git grep
>  	is executed outside of a git repository.  Defaults to false.
>
> +pcre.jit::
> +	If set to false, disable JIT when using PCRE.  Defaults to
> +	true.
> +	if set to -1 will try first to use JIT and fallback to the
> +	interpreter instead of returning an error.

Why not implement only -1, without adding this config setting?

> +
>
>  OPTIONS
>  -------
> @@ -175,6 +182,10 @@ providing this option will cause it to die.
>  	Use fixed strings for patterns (don't interpret pattern
>  	as a regex).
>
> +-j::
> +--[no-]pcre-jit::
> +	Diable JIT in PCRE with --no-pcre-jit.

"Disable".

René
Junio C Hamano July 29, 2019, 5:47 p.m. UTC | #3
René Scharfe <l.s.r@web.de> writes:

>> +pcre.jit::
>> +	If set to false, disable JIT when using PCRE.  Defaults to
>> +	true.
>> +	if set to -1 will try first to use JIT and fallback to the
>> +	interpreter instead of returning an error.
>
> Why not implement only -1, without adding this config setting?

... nor command line option.  If we have an auto-fallback, I would
think that makes the most sense.  IIRC the first iteration with only
the configuration was really about working around the (non-working)
pcre-jit---if we can self-detect and skip a non-working case, that
would allow us to drop end-user facing knobs, which is ideal.

Thanks for a doze of sanity.
Carlo Marcelo Arenas Belón July 30, 2019, 12:49 a.m. UTC | #4
On Mon, Jul 29, 2019 at 10:47 AM Junio C Hamano <gitster@pobox.com> wrote:
> René Scharfe <l.s.r@web.de> writes:
> >> +pcre.jit::
> >> +    If set to false, disable JIT when using PCRE.  Defaults to
> >> +    true.
> >> +    if set to -1 will try first to use JIT and fallback to the
> >> +    interpreter instead of returning an error.
> >
> > Why not implement only -1, without adding this config setting?
>
> ... nor command line option.  If we have an auto-fallback, I would
> think that makes the most sense.  IIRC the first iteration with only
> the configuration was really about working around the (non-working)
> pcre-jit---if we can self-detect and skip a non-working case, that
> would allow us to drop end-user facing knobs, which is ideal.

because that was proposed earlier[1] and wasn't accepted ;)

the main pushback though I got was that doing the fallback would degrade
performance and so it was suggested[2] that keeping the error should be
possible somehow (with the implication it will add yet another macro)

since living without grep -P was a reasonable tradeoff at that time got
punted, but the need to find a solution for this become more urgent once
it was announced[3] PCRE2 would be used also used outside -P

> Thanks for a doze of sanity.

Obviously I am biased, but I kind of like the knob as it allows the user
more flexibility to tweak the internals of grep and because we had
made those internals already visible (ex: not handling any library
errors ourselves and just aborting with a pcre error), but without any
flexibility to fix those problems themselves (unless they open the code
and rebuild, in most cases)

the comment from the user that reported[4] a regression with GNU grep
because of JIT stack size and that I quote below is representative of how
that layering violation affect users, and while git users are more likely
than grep users to do the code tweaking needed, they could use some
help.

"As using the JIT can not be turned off at runtime, nor can the stacksize
be controlled without patching + recompiling, this breaks previously
working expressions for me, so I consider this a new regression,
introduced with b06f7a29a58bbdd5866afc1e92dba3fdc9e2ed59 .

I tested that increasing the stack-size to 1 M fixes the problem for me.
A better fix could maybe consist of a better error message, allowing
stack-size control at runtime and / or making JIT optional at runtime."

making JIT optional at runtime is therefore the title of this patch and as
I mentioned in some other thread it might be even useful to us for our
own tests.

Carlo

[1] https://public-inbox.org/git/20181209230024.43444-3-carenas@gmail.com/
[2] https://public-inbox.org/git/87zhtbn5xb.fsf@evledraar.gmail.com/
[3] https://public-inbox.org/git/CAPUEspjKxQFiRgmfb2SuR_xpVu4=MN66kGEeBK1pHdBgXQbv7Q@mail.gmail.com/
[4] https://www.mail-archive.com/bug-grep@gnu.org/msg05762.html
René Scharfe July 30, 2019, 5:55 p.m. UTC | #5
Am 30.07.19 um 02:49 schrieb Carlo Arenas:
> On Mon, Jul 29, 2019 at 10:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>>> +pcre.jit::
>>>> +    If set to false, disable JIT when using PCRE.  Defaults to
>>>> +    true.
>>>> +    if set to -1 will try first to use JIT and fallback to the
>>>> +    interpreter instead of returning an error.
>>>
>>> Why not implement only -1, without adding this config setting?
>>
>> ... nor command line option.  If we have an auto-fallback, I would
>> think that makes the most sense.  IIRC the first iteration with only
>> the configuration was really about working around the (non-working)
>> pcre-jit---if we can self-detect and skip a non-working case, that
>> would allow us to drop end-user facing knobs, which is ideal.
>
> because that was proposed earlier[1] and wasn't accepted ;)

So you add all those knobs for Ævar?

Users on OpenBSD would get an error whenever they used -P?  And they
are expected to discover an option for turning off said error?  That
could be handled automatically?

This sounds like bullying to me.  I simply wouldn't use -P anymore if
that happened to me.  If that error was returned without -P, I'd
contemplate switching to the Silver Searcher or a similar tool.

> the main pushback though I got was that doing the fallback would degrade
> performance and so it was suggested[2] that keeping the error should be
> possible somehow (with the implication it will add yet another macro)

The slowdown by the fallback should be minimal -- just the extra
wasted time to compile the pattern to machine code.  Compilation time is
probably (hopefully?) dwarfed by search time.

IIUC, Ævar's concern was more about being able to discover when JIT is
not available for some reason.  Printing a warning with --debug or
--verbose could help with that.

Personally I don't care about JIT at all and wouldn't want to see such a
warning and certainly prefer not to get any error message about it,
even more so since there is nothing I can do about it.  (Disabling
security features to get faster search sounds sounds like a no-go.)

> since living without grep -P was a reasonable tradeoff at that time got
> punted, but the need to find a solution for this become more urgent once
> it was announced[3] PCRE2 would be used also used outside -P

Right.

> Obviously I am biased, but I kind of like the knob as it allows the user
> more flexibility to tweak the internals of grep and because we had
> made those internals already visible (ex: not handling any library
> errors ourselves and just aborting with a pcre error), but without any
> flexibility to fix those problems themselves (unless they open the code
> and rebuild, in most cases)
>
> the comment from the user that reported[4] a regression with GNU grep
> because of JIT stack size and that I quote below is representative of how
> that layering violation affect users, and while git users are more likely
> than grep users to do the code tweaking needed, they could use some
> help.

So JIT can cause other problems, and some of them we don't (or can't)
handle in our code?  Turning on an unstable feature without a way to
disable it sounds like a bad idea indeed.  Is it really that bad?  Can
we grow the stack on demand, for example, as GNU grep does now in
response to the bug you mentioned (http://bugs.gnu.org/19833)?  Are
there more such examples?

> making JIT optional at runtime is therefore the title of this patch and as
> I mentioned in some other thread it might be even useful to us for our
> own tests.

Testability is a valid concern, especially if the JIT code is
considered, well, unfinished.

René
Johannes Schindelin July 31, 2019, 12:32 p.m. UTC | #6
Hi,

On Mon, 29 Jul 2019, Carlo Marcelo Arenas Belón wrote:

>   $ git grep 'foo bar'
>   fatal: Couldn't JIT the PCRE2 pattern 'foo bar', got '-48'

My immediate reaction to this error message was: That's not helpful.
What is `-48` supposed to mean? Why do we even think it sensible to
throw such an error message at the end user? Can't we do a much better
job translating that into something that makes actual sense without
knowing implementation details?

But then, I realized that -48 must be a well-known constant in PCRE2,
and my reaction transformed into something much more hopeful: why don't
we detect the situation where the JIT'ed code was not actually
executable [*1*], and fall back to the non-JIT'ed code path ourselves,
without troubling the end user (maybe warning, but maybe better not lest
we annoy the user with something pointless)?

Even after finding out that -48 disappointingly means
PCRE2_ERROR_NOMEMORY (as opposed to something like
PCRE2_ERROR_CANNOT_EXECUTE_JIT_CODE), I like the idea of not bothering
end users and doing the sensible fallback under the hood.

Ciao,
Dscho

Footnote *1*: Why anybody would think it sensible to build a PCRE2 with
JIT on an OS that does not allow executing code that was written by the
same process is beyond me. Or is there a mode in OpenBSD that *does*
allow JIT'ed code to be executed?
Johannes Schindelin July 31, 2019, 12:36 p.m. UTC | #7
Hi Carlo,

On Mon, 29 Jul 2019, Carlo Arenas wrote:

> On Mon, Jul 29, 2019 at 10:47 AM Junio C Hamano <gitster@pobox.com> wrote:
> > René Scharfe <l.s.r@web.de> writes:
> > >> +pcre.jit::
> > >> +    If set to false, disable JIT when using PCRE.  Defaults to
> > >> +    true.
> > >> +    if set to -1 will try first to use JIT and fallback to the
> > >> +    interpreter instead of returning an error.
> > >
> > > Why not implement only -1, without adding this config setting?
> >
> > ... nor command line option.  If we have an auto-fallback, I would
> > think that makes the most sense.  IIRC the first iteration with only
> > the configuration was really about working around the (non-working)
> > pcre-jit---if we can self-detect and skip a non-working case, that
> > would allow us to drop end-user facing knobs, which is ideal.
>
> because that was proposed earlier[1] and wasn't accepted ;)
> [...]
> [1] https://public-inbox.org/git/20181209230024.43444-3-carenas@gmail.com/

For the record, I read
https://public-inbox.org/git/xmqqh8flkgs2.fsf@gitster-ct.c.googlers.com/
as encouraging a slightly more powerful argument in favor. Junio seemed
to hope that PCRE2's own `pcre2grep` would behave that way, and that
would give us plenty reason to just imitate it.

I don't know whether `pcre2grep` behaves that way, even if it does not,
I think the benefits of the auto fallback to the end user are
considerable.

Ciao,
Dscho
Ævar Arnfjörð Bjarmason July 31, 2019, 2:57 p.m. UTC | #8
On Wed, Jul 31 2019, Johannes Schindelin wrote:

> Hi,
>
> On Mon, 29 Jul 2019, Carlo Marcelo Arenas Belón wrote:
>
>>   $ git grep 'foo bar'
>>   fatal: Couldn't JIT the PCRE2 pattern 'foo bar', got '-48'
>
> My immediate reaction to this error message was: That's not helpful.
> What is `-48` supposed to mean? Why do we even think it sensible to
> throw such an error message at the end user? Can't we do a much better
> job translating that into something that makes actual sense without
> knowing implementation details?
>
> But then, I realized that -48 must be a well-known constant in PCRE2,
> and my reaction transformed into something much more hopeful: why don't
> we detect the situation where the JIT'ed code was not actually
> executable [*1*], and fall back to the non-JIT'ed code path ourselves,
> without troubling the end user (maybe warning, but maybe better not lest
> we annoy the user with something pointless)?
>
> Even after finding out that -48 disappointingly means
> PCRE2_ERROR_NOMEMORY (as opposed to something like
> PCRE2_ERROR_CANNOT_EXECUTE_JIT_CODE), I like the idea of not bothering
> end users and doing the sensible fallback under the hood.
>
> Ciao,
> Dscho
>
> Footnote *1*: Why anybody would think it sensible to build a PCRE2 with
> JIT on an OS that does not allow executing code that was written by the
> same process is beyond me. Or is there a mode in OpenBSD that *does*
> allow JIT'ed code to be executed?

We do detect if JIT isn't supported and fall back. That's what the
pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on) code in grep.c
does. This and is the subsequent pcre2_pattern_info() call is how PCRE
documents that you should do this.

What hasn't been supported is all of that saying "yes, I support JIT"
and the feature then fail whaling. I had not encountered that before.

So far that seems like because Carlo just built a completely broken PCRE
v2 package, so I don't know if that's worth supporting on our
side. I.e. this isn't something I think could plausibly happen in the
wild.

That should *not* be confused with me thinking other stuff Carlo's
raised is a non-issue, e.g. running into the JIT stack limit etc. Some
of that's clearly bugs in our/my grep.c code that need fixing.
Junio C Hamano July 31, 2019, 4:18 p.m. UTC | #9
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> For the record, I read
> https://public-inbox.org/git/xmqqh8flkgs2.fsf@gitster-ct.c.googlers.com/
> as encouraging a slightly more powerful argument in favor. Junio seemed
> to hope that PCRE2's own `pcre2grep` would behave that way, and that
> would give us plenty reason to just imitate it.
>
> I don't know whether `pcre2grep` behaves that way, even if it does not,
> I think the benefits of the auto fallback to the end user are
> considerable.

Thanks for digging that up ;-)  I do agree with what was said there.

JIT is merely an optimization, and we should be able to work without
it and should not even bother the users with warning messages when
we have to choose non-JIT codepath.  Those who care about debugging
can use a "--debug" option or something to figure out if their build
on a particular pattern is or is not using JIT.

I think I read somebody (Carlo?) made that argument in the thread
earlier, and I agree with that sentiment.
Carlo Marcelo Arenas Belón Aug. 4, 2019, 12:25 a.m. UTC | #10
On Wed, Jul 31, 2019 at 7:57 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> What hasn't been supported is all of that saying "yes, I support JIT"
> and the feature then fail whaling. I had not encountered that before.
>
> So far that seems like because Carlo just built a completely broken PCRE
> v2 package, so I don't know if that's worth supporting on our
> side. I.e. this isn't something I think could plausibly happen in the
> wild.

since you are in Debian please follow the instructions here:

  https://wiki.debian.org/SELinux/Setup

no need to rebuild git or pcre (but to enable selinux will need to
reboot twice), then type as root the following:

  # set enforce 1

Carlo

Patch
diff mbox series

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index c89fb569e3..895c6b34ec 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,6 +13,7 @@  SYNOPSIS
 	   [-v | --invert-match] [-h|-H] [--full-name]
 	   [-E | --extended-regexp] [-G | --basic-regexp]
 	   [-P | --perl-regexp]
+	   [-j | --[no]-pcre-jit]
 	   [-F | --fixed-strings] [-n | --line-number] [--column]
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [(-O | --open-files-in-pager) [<pager>]]
@@ -69,6 +70,12 @@  grep.fallbackToNoIndex::
 	If set to true, fall back to git grep --no-index if git grep
 	is executed outside of a git repository.  Defaults to false.
 
+pcre.jit::
+	If set to false, disable JIT when using PCRE.  Defaults to
+	true.
+	if set to -1 will try first to use JIT and fallback to the
+	interpreter instead of returning an error.
+
 
 OPTIONS
 -------
@@ -175,6 +182,10 @@  providing this option will cause it to die.
 	Use fixed strings for patterns (don't interpret pattern
 	as a regex).
 
+-j::
+--[no-]pcre-jit::
+	Diable JIT in PCRE with --no-pcre-jit.
+
 -n::
 --line-number::
 	Prefix the line number to matching lines.
diff --git a/builtin/grep.c b/builtin/grep.c
index 580fd38f41..b0e94875b2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -923,6 +923,10 @@  int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_BOOL_F(0, "ext-grep", &external_grep_allowed__ignored,
 			   N_("allow calling of grep(1) (ignored by this build)"),
 			   PARSE_OPT_NOCOMPLETE),
+		OPT_GROUP("PCRE"),
+		OPT_SET_INT('j', "pcre-jit", &opt.pcre_jit,
+			N_("when to use JIT with PCRE"),
+			1),
 		OPT_END()
 	};
 
diff --git a/grep.c b/grep.c
index c7c06ae08d..d58cad0257 100644
--- a/grep.c
+++ b/grep.c
@@ -56,6 +56,7 @@  void init_grep_defaults(struct repository *repo)
 	opt->repo = repo;
 	opt->relative = 1;
 	opt->pathname = 1;
+	opt->pcre_jit = 1;
 	opt->max_depth = -1;
 	opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
 	color_set(opt->colors[GREP_COLOR_CONTEXT], "");
@@ -125,6 +126,12 @@  int grep_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "pcre.jit")) {
+		int is_bool;
+		opt->pcre_jit = git_config_bool_or_int(var, value, &is_bool);
+		return 0;
+	}
+
 	if (!strcmp(var, "color.grep"))
 		opt->color = git_config_colorbool(var, value);
 	if (!strcmp(var, "color.grep.match")) {
@@ -163,6 +170,7 @@  void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	opt->pattern_tail = &opt->pattern_list;
 	opt->header_tail = &opt->header_list;
 
+	opt->pcre_jit = def->pcre_jit;
 	opt->only_matching = def->only_matching;
 	opt->color = def->color;
 	opt->extended_regexp_option = def->extended_regexp_option;
@@ -393,7 +401,8 @@  static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
 		die("%s", error);
 
 #ifdef GIT_PCRE1_USE_JIT
-	pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);
+	if (opt->pcre_jit)
+		pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);
 #endif
 }
 
@@ -489,11 +498,24 @@  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 		compile_regexp_failed(p, (const char *)&errbuf);
 	}
 
-	pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
+	if (opt->pcre_jit)
+		pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
+
 	if (p->pcre2_jit_on) {
 		jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
-		if (jitret)
-			die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
+		if (jitret) {
+			if ((opt->pcre_jit < 0) &&
+				jitret == PCRE2_ERROR_NOMEMORY) {
+				/*
+				 * JIT compiler isn't available but we can
+				 * still fallback to the interpreter
+				 */
+				p->pcre2_jit_on = 0;
+				return;
+			}
+			else
+				die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
+		}
 
 		/*
 		 * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just
diff --git a/grep.h b/grep.h
index c0c71eb4a9..fff152e606 100644
--- a/grep.h
+++ b/grep.h
@@ -151,6 +151,7 @@  struct grep_opt {
 	int allow_textconv;
 	int extended;
 	int use_reflog_filter;
+	int pcre_jit;
 	int pcre1;
 	int pcre2;
 	int relative;