Re: [PATCH] grep: fall back to interpreter mode if JIT fails

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Dec 16 2022, Carlo Arenas wrote:

[CC-ing pcre-dev@ for this "future error API" discussion]

> On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Mathias Krause <minipli@grsecurity.net> writes:
>>
>> > ... However, from a user's point of view a fall back to
>> > interpreter mode might still be desired in this case, as a failing
>> > 'git grep' is simply not acceptable, IMHO.
>>
>> "git grep" that silently produces a wrong result (by falling back
>> after a problem is detected) would not be acceptable, either.
>
> except that an error at this point only invalidates the use of JIT,
> so calling pcre2_jit_match() is invalid but calling pcre2_match() is not.
>
> the later is setup to be used later by the code that is added,

I think we could stumble ahead, but if this were to happen our
assumptions about how the API works have been invalidated.

The pcre2_jit_compile() doesn't promise to return a finite set of error
codes, but:

	[...]0 for success, or a negative error code otherwise[...]

But if new codes were added it's anyone's guess what state we'd be in,
so I think the safe thing is to BUG() out if we get as far as
pcre2_jit_compile() and don't get either PCRE2_ERROR_JIT_BADOPTION or
PCRE2_ERROR_NOMEMORY.

>> Receiving BADOPTION could be a sign that there is something wrong in
>> the input, not from the end-user but from the code, in which case
>> stopping with BUG() may be a more appropriate?
>
> The way PCRE handles this kind of errors internally is to instruct pcre2_match()
> to use the interpreter.
>
> While a BUG() might be a way to ensure the code is using the right set
> of options
> I would expect that the failure will be reported by pcre2_compile
> instead, with the
> only cases left, only being interna to PCRE (ex: JIT can't yet support
> a feature the
> interpreter has)

I agree that it's possible in general that an external library might
start returning a "benign" error code that we could recover from, so
BUG(...) would be overdoing it.

I just don't see that libpcre would do that in this case. In general the
JIT supports all patterns that the normal engine does, so if we're past
the "is it available?" hump it should work.

If it starts not doing that I'd think they'd do a major version upgrade,
or opt-in with a new flag etc.

Note that it already has a way of checking for "we tried to do the jit
thing, but it wasn't on in the end". See the code I added in
a25b9085043[1] (grep: fix segfault under -P + PCRE2 <=10.30 + (*NO_JIT),
2017-11-23), which comes right after the pcre2_jit_compile().

So not only would a BUG() biting us here require them to create a new
code for the state of "we have the JIT, but can't use it here" (for some
reason I can't imagine, as "PCRE2_ERROR_NOMEMORY" is already
"overloaded" to mean that).

It would also require them to invent a new "soft" failure mode for the
JIT, i.e. not the facility added in a25b9085043, where we can use the
JIT, but it's not on after all due to a "(*NO_JIT)" in the pattern
itself.

But I may be wrong, so I've CC'd pcre-dev@ to see if they have any
commentary on this proposed API paranoia. For them: The greater context
of this thread on the git ML is at [2].

1. https://github.com/git/git/commit/a25b9085043b8029169b4d5b172b78ca3f38fb37
2. https://lore.kernel.org/git/20221216121557.30714-1-minipli@grsecurity.net/