Re: [PATCH] grep: fall back to interpreter mode if JIT fails
- From
Mathias Krause <minipli@grsecurity.net>
- Date
- Dec 20, 2022, 20:40 UTC
- Message-ID
- <7a657644-cd7c-8638-55aa-667c528a5624@grsecurity.net>
- In-Reply-To
- <xmqq359fm06c.fsf@gitster.g>
Am 16.12.22 um 23:52 schrieb Junio C Hamano:
Show 18 quoted lines
> 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 had a follow-up RFC[1] half a year later, adding a config option and, after some more discussion, even command line switches[2]. But not just IMHO is that far too much, but even you suggested to simply revert back to the initial RFC and implement the automatic fallback[3], basically merging it with a proper changelog[4]. As that never happened, I took up the work and tried to do just that.
1. https://lore.kernel.org/git/20190728235427.41425-1-carenas@gmail.com/ 2. https://lore.kernel.org/git/20190729105955.44390-1-carenas@gmail.com/ 3. https://lore.kernel.org/git/xmqqh874vikk.fsf@gitster-ct.c.googlers.com/ 4. https://lore.kernel.org/git/xmqqef1zmkp5.fsf@gitster-ct.c.googlers.com/
Show 10 quoted lines
> 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.
Sorry for not digging up that information for the initial patch submission. I'll add it to v2.
> The rest of the proposed log message, and the change itself, both > look very reasonable and well explained, at least to me.
Thanks a lot for the detailed review. I'll update the commit log accordingly but will wait for more feedback to see on which solution we want to settle on.
Show 37 quoted lines
> 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