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

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

Mathias Krause <minipli@grsecurity.net> writes:

> Such a change was already proposed 4 years ago [1] but wasn't merged for
> unknown reasons.
>
> 1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com

This part does not belong to the log message but should go below
three-dash line.  If you have a more concrete "it was rejected
because ...", to help judging why this version improves upon the
previous attempt and it is clear the reason for past rejection no
longer applies, that is a very different story, though.

The way I read the original thread (assuming that the lore archive
is showing all relevant messages there) is that RFC was reviewed
well and everybody was happy about the direction it took.  It even
received fairly concrete suggestions for the real, non-RFC version,
but that never materialized.

It is very clear that the posted patch was not yet in a mergeable
state as-is; "for unknown reasons" is less than helpful.

Now, we learned a more concrete reason, i.e. "it got tons of help
improving the draft into the final version, but the help was
discarded and the final version did not materialize", does the
attempt this time around improve on it sufficiently to make it
mergeable, or is it sufficiently different that it needs a fresh
review from scratch?  If the latter, then "for unknown reasons"
becomes even less relevant.

The rest of the proposed log message, and the change itself, both
look very reasonable and well explained, at least to me.

Thanks.


> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>	# tweaked changelog, added comment
> ---
>  grep.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 06eed694936c..f2ada528b21d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -317,8 +317,21 @@ 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)
> -			die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
> +		if (jitret) {
> +			/*
> +			 * 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;
> +		}
>  
>  		/*
>  		 * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just