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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 19, 2022, 09:00 UTC
Message-ID: <221219.86bknz21qj.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAPUEspjqgSOS4KKw2nTaRYdiCFo4Ok6OfmKrqb+Mjq+oXn5nsg@mail.gmail.com>

On Fri, Dec 16 2022, Carlo Arenas wrote:

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

Show 15 quoted lines

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

Show 14 quoted lines

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