Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails

From: Mathias Krause <minipli@grsecurity.net>
Date: Jan 30, 2023, 11:08 UTC
Message-ID: <9b5a1113-84f1-1651-bffc-6382462057dd@grsecurity.net>
In-Reply-To: <xmqqk0156z55.fsf@gitster.g>

On 29.01.23 18:15, Junio C Hamano wrote:

Show 11 quoted lines

> Mathias Krause <minipli@grsecurity.net> writes:
> 
>> ... While we might be able to compile the pattern and run it in
>> interpreter mode, it'll likely have a *much* higher runtime.
>> ...
>> So this grep run eat up ~9.5 *hours* of CPU time. Do we really want to
>> fall back to something like this for the pathological cases? ...Yeah, I
>> don't think so either.
> 
> You may not, but I do not agree with you at all.  The code should
> not outsmart the user in such a case.

It doesn't. My rhetoric question was just missing "automatically" to state that I would dislike an *automatic* fallback to the interpreter for *pathological cases.* But I'm fine with (and that's what this patch is all about!) a fallback to the interpreter for patterns that simply fail the JIT because it's broken. Sorry for the confusion.

Show 5 quoted lines

> Even if the pattern the user came up with is impossible to grok for
> a working JIT compiler, and it might be hard to grok for the
> interpreter, what is the next step you recommend the user if you
> refuse to fall back on the interprete?  "Rewrite it to please the
> JIT compiler"?

Not at all. A user is still free to disable the JIT and enforce using the interpreter by using the "(*NO_JIT)" prefix. My patch doesn't disable this behavior. My patch only tries to avoid having to specify it for "regular" patterns when the JIT is broken anyways.

The key here is that this would be a manual step (in contrast to an automatic fallback), i.e. we require explicit user consent to accept the worse runtime performance. And, IMHO, that should be acceptable from a usability point of view as this would only be required for the pathological cases an otherwise functional JIT simply cannot handle.

> If that is the best pattern the user can produce to solve the
> problem at hand, being able to give the user an answer in 9 hours is
> much better than not being able to give anything at all.

Sure, fully agree.

Show 7 quoted lines

> Maybe after waiting for 5 minutes, the user gets bored and ^C, or
> without killing it, open another terminal and try a different
> patern, and in 9 hours, perhaps comes up with an equivalent (or
> different but close enough) pattern that happens to run much faster,
> at which time the user may kill the original one.  In any of these
> cases, by refusing to run, the code is not doing any service to the
> user.

My patch doesn't make it worse than what 'git grep' would currently be doing. On the contrary, actually. It allows me to use PaX's MPROTECT and have a functional 'git grep' as well.

Maybe the missing piece here is simply something like below to make users more aware of the possibility to disable the JIT for the more complex cases?:

diff --git a/grep.c b/grep.c
index 59afc3f07fc9..1422f168b087 100644
--- a/grep.c
+++ b/grep.c
@@ -357,7 +357,8 @@ static void compile_pcre2_pattern(struct grep_pat
*p, const struct grep_opt *opt
                        p->pcre2_jit_on = 0;
                        return;
                } else if (jitret) {
-                       die("Couldn't JIT the PCRE2 pattern '%s', got
'%d'\n", p->pattern, jitret);
+                       die("Couldn't JIT the PCRE2 pattern '%s', got
'%d'%s\n", p->pattern, jitret,
+                           pcre2_jit_functional() ? "\nYou might retry
by prefixing the pattern with '(*NO_JIT)'" : "");
                }

                /*

(Sorry about the wrapped lines, my mailer is just broken. I'll make it a
proper patch, if such functionality is indeed wanted.)

Thanks,
Mathias