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

[Junio C Hamano <gitster@pobox.com>]

Mathias Krause <minipli@grsecurity.net> writes:

> As having a functional PCRE2 JIT compiler is a legitimate use case for
> performance reasons, we'll only do the fallback if the supposedly
> available JIT is found to be non-functional by attempting to JIT compile
> a very simple pattern. If this fails, JIT is deemed to be non-functional
> and we do the interpreter fallback. For all other cases, i.e. the simple
> pattern can be compiled but the user provided cannot, we fail hard as we
> do now as the reason for the failure must be the pattern itself.

I do not know if it is a good idea to rely on the "very simple
pattern".  The implementation of JIT could devise various ways to
succeed for such simple patterns without having writable-executable
piece of memory.  What happened to the earlier idea of falling back
to the interpreted codepath, which will catch any bad pattern that
has "the reason for the failure" by failing anyway?

> +static int pcre2_jit_functional(void)
> +{
> +	static int jit_working = -1;
> +	pcre2_code *code;
> +	size_t off;
> +	int err;
> +
> +	if (jit_working != -1)
> +		return jit_working;
> +
> +	/*
> +	 * Try to JIT compile a simple pattern to probe if the JIT is
> +	 * working in general. It might fail for systems where creating
> +	 * memory mappings for runtime code generation is restricted.
> +	 */
> +	code = pcre2_compile((PCRE2_SPTR)".", 1, 0, &err, &off, NULL);
> +	if (!code)
> +		return 0;
> +
> +	jit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0;
> +	pcre2_code_free(code);

I'd prefer not having to worry about: Or it might not fail for such
systems, as the pattern is too simple and future versions of
pcre2_compile() could have special case code.

> @@ -317,8 +342,23 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>  	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)
> +		if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {
> +			/*
> +			 * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)
> +			 * indicated JIT support, the library might still
> +			 * fail to generate JIT code for various reasons,
> +			 * e.g. when SELinux's 'deny_execmem' or PaX's
> +			 * MPROTECT prevent creating W|X memory mappings.
> +			 *
> +			 * Instead of faling hard, fall back to interpreter
> +			 * mode, just as if the pattern was prefixed with
> +			 * '(*NO_JIT)'.
> +			 */
> +			p->pcre2_jit_on = 0;
> +			return;

Yes, the "instead of failing hard, fall back" makes sense.  Just
that I do not see why the runtime test is a good thing to have.  In
short, we are not in the business of catching bugs in pcre2_jit
implementations, so if they say they cannot compile the pattern (I
would even say I doubt the point of checking the return code to
ensure it is NOMEMORY), it would be fine to let the interpreter
codepath to inspect the pattern and diagnose problems with it, or
take the slow match without JIT.

What am I missing?