{"thread":{"id":"58967","subject":"[PATCH] grep: fall back to interpreter mode if JIT fails","startedAt":"2022-12-16T12:16:28Z","lastAt":"2023-01-31T21:05:22Z","messageCount":36,"participants":["Mathias Krause","Ævar Arnfjörð Bjarmason","Junio C Hamano","Carlo Arenas","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469173","messageId":"20221216121557.30714-1-minipli@grsecurity.net","threadId":"58967","inReplyTo":null,"subject":"[PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2022-12-16T12:15:57Z","receivedAt":"2022-12-16T12:16:28Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"From: Carlo Marcelo Arenas Belón \n\nUnder Linux systems with SELinux's 'deny_execmem' or PaX's MPROTECT\nenabled, the allocation of PCRE2's JIT rwx memory may be prohibited,\nmaking pcre2_jit_compile() fail with PCRE2_ERROR_NOMEMORY (-48):\n\n [user@fedora git]$ git grep -c PCRE2_JIT\n grep.c:1\n\n [user@fedora git]$ # Enable SELinux's W^X policy\n [user@fedora git]$ sudo semanage boolean -m -1 deny_execmem\n\n [user@fedora git]$ # JIT memory allocation fails, breaking 'git grep'\n [user@fedora git]$ git grep -c PCRE2_JIT\n fatal: Couldn't JIT the PCRE2 pattern 'PCRE2_JIT', got '-48'\n\nInstead of failing hard in this case and making 'git grep' unusable on\nsuch systems, simply fall back to interpreter mode, leading to a much\nbetter user experience.\n\nSuch a change was already proposed 4 years ago [1] but wasn't merged for\nunknown reasons.\n\n1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Mathias Krause \t# tweaked changelog, added comment\n---\n grep.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 06eed694936c..f2ada528b21d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -317,8 +317,21 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n \tif (p->pcre2_jit_on) {\n \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n-\t\tif (jitret)\n-\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n+\t\tif (jitret) {\n+\t\t\t/*\n+\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n+\t\t\t * indicated JIT support, the library might still\n+\t\t\t * fail to generate JIT code for various reasons,\n+\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n+\t\t\t * MPROTECT prevent creating W|X memory mappings.\n+\t\t\t *\n+\t\t\t * Instead of faling hard, fall back to interpreter\n+\t\t\t * mode, just as if the pattern was prefixed with\n+\t\t\t * '(*NO_JIT)'.\n+\t\t\t */\n+\t\t\tp->pcre2_jit_on = 0;\n+\t\t\treturn;\n+\t\t}\n \n \t\t/*\n \t\t * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just\n-- \n2.35.1\n\n"},{"id":"469182","messageId":"221216.86o7s31fyt.gmgdl@evledraar.gmail.com","threadId":"58967","inReplyTo":"20221216121557.30714-1-minipli@grsecurity.net","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-16T16:12:46Z","receivedAt":"2022-12-16T16:17:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 16 2022, Mathias Krause wrote:\n\n> From: Carlo Marcelo Arenas Belón \n>\n> Under Linux systems with SELinux's 'deny_execmem' or PaX's MPROTECT\n> enabled, the allocation of PCRE2's JIT rwx memory may be prohibited,\n> making pcre2_jit_compile() fail with PCRE2_ERROR_NOMEMORY (-48):\n>\n> [user@fedora git]$ git grep -c PCRE2_JIT\n> grep.c:1\n>\n> [user@fedora git]$ # Enable SELinux's W^X policy\n> [user@fedora git]$ sudo semanage boolean -m -1 deny_execmem\n>\n> [user@fedora git]$ # JIT memory allocation fails, breaking 'git grep'\n> [user@fedora git]$ git grep -c PCRE2_JIT\n> fatal: Couldn't JIT the PCRE2 pattern 'PCRE2_JIT', got '-48'\n>\n> Instead of failing hard in this case and making 'git grep' unusable on\n> such systems, simply fall back to interpreter mode, leading to a much\n> better user experience.\n>\n> Such a change was already proposed 4 years ago [1] but wasn't merged for\n> unknown reasons.\n\nYeah, it's unfortunate that it fell between the cracks, and it's good to\nhave such a fallback mechanism.\n\n> 1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> Signed-off-by: Mathias Krause \t# tweaked changelog, added comment\n> ---\n> grep.c | 17 +++++++++++++++--\n> 1 file changed, 15 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 06eed694936c..f2ada528b21d 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -317,8 +317,21 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n> \tif (p->pcre2_jit_on) {\n> \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n> -\t\tif (jitret)\n> -\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n> +\t\tif (jitret) {\n> +\t\t\t/*\n> +\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n> +\t\t\t * indicated JIT support, the library might still\n> +\t\t\t * fail to generate JIT code for various reasons,\n> +\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n> +\t\t\t * MPROTECT prevent creating W|X memory mappings.\n> +\t\t\t *\n> +\t\t\t * Instead of faling hard, fall back to interpreter\n> +\t\t\t * mode, just as if the pattern was prefixed with\n> +\t\t\t * '(*NO_JIT)'.\n> +\t\t\t */\n> +\t\t\tp->pcre2_jit_on = 0;\n> +\t\t\treturn;\n\nFrom my reading of the docs it returns two different codes:\nPCRE2_ERROR_JIT_BADOPTION or PCRE2_ERROR_NOMEMORY.\n\nThis change will start treating both the same, but we only want to allow\nthe latter, surely?\n\nSo shouldn't this be e.g.:\n\n\tjitret = pcre2_jit_compile(...);\n\tif (jitret == PCRE2_ERROR_NOMEMORY) {\n\t\t/* code you added here */\n\t} else if (jitret) {\n\t\tBUG(...);\n\t}\n\nI put a BUG() there, we could keep the die(), but\nPCRE2_ERROR_JIT_BADOPTION is really more appropriate as a BUG(), and if\nit starts returning any other codes our use of the API is also in some\nunknown state, so we should also BUG() out.\n"},{"id":"469186","messageId":"62a06c5b-9646-17f8-b4d5-39823d3cc25a@grsecurity.net","threadId":"58967","inReplyTo":"221216.86o7s31fyt.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2022-12-16T19:26:26Z","receivedAt":"2022-12-16T19:27:09Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"Am 16.12.22 um 17:12 schrieb Ævar Arnfjörð Bjarmason:\n> \n> On Fri, Dec 16 2022, Mathias Krause wrote:\n> \n>> From: Carlo Marcelo Arenas Belón \n>>\n>> Under Linux systems with SELinux's 'deny_execmem' or PaX's MPROTECT\n>> enabled, the allocation of PCRE2's JIT rwx memory may be prohibited,\n>> making pcre2_jit_compile() fail with PCRE2_ERROR_NOMEMORY (-48):\n>>\n>> [user@fedora git]$ git grep -c PCRE2_JIT\n>> grep.c:1\n>>\n>> [user@fedora git]$ # Enable SELinux's W^X policy\n>> [user@fedora git]$ sudo semanage boolean -m -1 deny_execmem\n>>\n>> [user@fedora git]$ # JIT memory allocation fails, breaking 'git grep'\n>> [user@fedora git]$ git grep -c PCRE2_JIT\n>> fatal: Couldn't JIT the PCRE2 pattern 'PCRE2_JIT', got '-48'\n>>\n>> Instead of failing hard in this case and making 'git grep' unusable on\n>> such systems, simply fall back to interpreter mode, leading to a much\n>> better user experience.\n>>\n>> Such a change was already proposed 4 years ago [1] but wasn't merged for\n>> unknown reasons.\n> \n> Yeah, it's unfortunate that it fell between the cracks, and it's good to\n> have such a fallback mechanism.\n\nI fully agree, obviously!\n\n>> 1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com\n>>\n>> Signed-off-by: Carlo Marcelo Arenas Belón \n>> Signed-off-by: Mathias Krause \t# tweaked changelog, added comment\n>> ---\n>> grep.c | 17 +++++++++++++++--\n>> 1 file changed, 15 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/grep.c b/grep.c\n>> index 06eed694936c..f2ada528b21d 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -317,8 +317,21 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>> \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n>> \tif (p->pcre2_jit_on) {\n>> \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n>> -\t\tif (jitret)\n>> -\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n>> +\t\tif (jitret) {\n>> +\t\t\t/*\n>> +\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n>> +\t\t\t * indicated JIT support, the library might still\n>> +\t\t\t * fail to generate JIT code for various reasons,\n>> +\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n>> +\t\t\t * MPROTECT prevent creating W|X memory mappings.\n>> +\t\t\t *\n>> +\t\t\t * Instead of faling hard, fall back to interpreter\n>> +\t\t\t * mode, just as if the pattern was prefixed with\n>> +\t\t\t * '(*NO_JIT)'.\n>> +\t\t\t */\n>> +\t\t\tp->pcre2_jit_on = 0;\n>> +\t\t\treturn;\n> \n> From my reading of the docs it returns two different codes:\n> PCRE2_ERROR_JIT_BADOPTION or PCRE2_ERROR_NOMEMORY.\n> \n> This change will start treating both the same, but we only want to allow\n> the latter, surely?\n> \n> So shouldn't this be e.g.:\n> \n> \tjitret = pcre2_jit_compile(...);\n> \tif (jitret == PCRE2_ERROR_NOMEMORY) {\n> \t\t/* code you added here */\n> \t} else if (jitret) {\n> \t\tBUG(...);\n> \t}\n\nI'm fine with it either way. In fact, just testing for the \"JIT memory\ncannot be allocated\" error might be more on point and could detect wrong\nAPI usage by the means of an error. However, from a user's point of view\na fall back to interpreter mode might still be desired in this case, as\na failing 'git grep' is simply not acceptable, IMHO.\n\n> I put a BUG() there, we could keep the die(), but\n> PCRE2_ERROR_JIT_BADOPTION is really more appropriate as a BUG(), and if\n> it starts returning any other codes our use of the API is also in some\n> unknown state, so we should also BUG() out.\n\nValid points. I'll give others some more time to churn in and if there\nare no strong objections, I'll change it like you suggest.\n\nThanks,\nMathias\n"},{"id":"469190","messageId":"xmqq359fm06c.fsf@gitster.g","threadId":"58967","inReplyTo":"20221216121557.30714-1-minipli@grsecurity.net","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-16T22:52:11Z","receivedAt":"2022-12-16T22:52:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathias Krause writes:\n\n> Such a change was already proposed 4 years ago [1] but wasn't merged for\n> unknown reasons.\n>\n> 1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com\n\nThis part does not belong to the log message but should go below\nthree-dash line. If you have a more concrete \"it was rejected\nbecause ...\", to help judging why this version improves upon the\nprevious attempt and it is clear the reason for past rejection no\nlonger applies, that is a very different story, though.\n\nThe way I read the original thread (assuming that the lore archive\nis showing all relevant messages there) is that RFC was reviewed\nwell and everybody was happy about the direction it took. It even\nreceived fairly concrete suggestions for the real, non-RFC version,\nbut that never materialized.\n\nIt is very clear that the posted patch was not yet in a mergeable\nstate as-is; \"for unknown reasons\" is less than helpful.\n\nNow, we learned a more concrete reason, i.e. \"it got tons of help\nimproving the draft into the final version, but the help was\ndiscarded and the final version did not materialize\", does the\nattempt this time around improve on it sufficiently to make it\nmergeable, or is it sufficiently different that it needs a fresh\nreview from scratch? If the latter, then \"for unknown reasons\"\nbecomes even less relevant.\n\nThe rest of the proposed log message, and the change itself, both\nlook very reasonable and well explained, at least to me.\n\nThanks.\n\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> Signed-off-by: Mathias Krause \t# tweaked changelog, added comment\n> ---\n> grep.c | 17 +++++++++++++++--\n> 1 file changed, 15 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 06eed694936c..f2ada528b21d 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -317,8 +317,21 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n> \tif (p->pcre2_jit_on) {\n> \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n> -\t\tif (jitret)\n> -\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n> +\t\tif (jitret) {\n> +\t\t\t/*\n> +\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n> +\t\t\t * indicated JIT support, the library might still\n> +\t\t\t * fail to generate JIT code for various reasons,\n> +\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n> +\t\t\t * MPROTECT prevent creating W|X memory mappings.\n> +\t\t\t *\n> +\t\t\t * Instead of faling hard, fall back to interpreter\n> +\t\t\t * mode, just as if the pattern was prefixed with\n> +\t\t\t * '(*NO_JIT)'.\n> +\t\t\t */\n> +\t\t\tp->pcre2_jit_on = 0;\n> +\t\t\treturn;\n> +\t\t}\n> \n> \t\t/*\n> \t\t * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just\n"},{"id":"469193","messageId":"xmqqlen7kksr.fsf@gitster.g","threadId":"58967","inReplyTo":"62a06c5b-9646-17f8-b4d5-39823d3cc25a@grsecurity.net","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-16T23:09:40Z","receivedAt":"2022-12-16T23:10:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathias Krause writes:\n\n> ... However, from a user's point of view a fall back to\n> interpreter mode might still be desired in this case, as a failing\n> 'git grep' is simply not acceptable, IMHO.\n\n\"git grep\" that silently produces a wrong result (by falling back\nafter a problem is detected) would not be acceptable, either.\nReceiving BADOPTION could be a sign that there is something wrong in\nthe input, not from the end-user but from the code, in which case\nstopping with BUG() may be a more appropriate?\n\n>> I put a BUG() there, we could keep the die(), but\n>> PCRE2_ERROR_JIT_BADOPTION is really more appropriate as a BUG(), and if\n>> it starts returning any other codes our use of the API is also in some\n>> unknown state, so we should also BUG() out.\n>\n> Valid points. I'll give others some more time to churn in and if there\n> are no strong objections, I'll change it like you suggest.\n\nYup, sounds quite sensible.\n"},{"id":"469202","messageId":"CAPUEspjqgSOS4KKw2nTaRYdiCFo4Ok6OfmKrqb+Mjq+oXn5nsg@mail.gmail.com","threadId":"58967","inReplyTo":"xmqqlen7kksr.fsf@gitster.g","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2022-12-17T02:50:41Z","receivedAt":"2022-12-17T02:51:45Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano wrote:\n>\n> Mathias Krause writes:\n>\n> > ... However, from a user's point of view a fall back to\n> > interpreter mode might still be desired in this case, as a failing\n> > 'git grep' is simply not acceptable, IMHO.\n>\n> \"git grep\" that silently produces a wrong result (by falling back\n> after a problem is detected) would not be acceptable, either.\n\nexcept that an error at this point only invalidates the use of JIT,\nso calling pcre2_jit_match() is invalid but calling pcre2_match() is not.\n\nthe later is setup to be used later by the code that is added,\n\n> Receiving BADOPTION could be a sign that there is something wrong in\n> the input, not from the end-user but from the code, in which case\n> stopping with BUG() may be a more appropriate?\n\nThe way PCRE handles this kind of errors internally is to instruct pcre2_match()\nto use the interpreter.\n\nWhile a BUG() might be a way to ensure the code is using the right set\nof options\nI would expect that the failure will be reported by pcre2_compile\ninstead, with the\nonly cases left, only being interna to PCRE (ex: JIT can't yet support\na feature the\ninterpreter has)\n\nCarlo\n"},{"id":"469265","messageId":"221219.86bknz21qj.gmgdl@evledraar.gmail.com","threadId":"58967","inReplyTo":"CAPUEspjqgSOS4KKw2nTaRYdiCFo4Ok6OfmKrqb+Mjq+oXn5nsg@mail.gmail.com","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T09:00:11Z","receivedAt":"2022-12-19T09:15:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 16 2022, Carlo Arenas wrote:\n\n[CC-ing pcre-dev@ for this \"future error API\" discussion]\n\n> On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano wrote:\n>>\n>> Mathias Krause writes:\n>>\n>> > ... However, from a user's point of view a fall back to\n>> > interpreter mode might still be desired in this case, as a failing\n>> > 'git grep' is simply not acceptable, IMHO.\n>>\n>> \"git grep\" that silently produces a wrong result (by falling back\n>> after a problem is detected) would not be acceptable, either.\n>\n> except that an error at this point only invalidates the use of JIT,\n> so calling pcre2_jit_match() is invalid but calling pcre2_match() is not.\n>\n> the later is setup to be used later by the code that is added,\n\nI think we could stumble ahead, but if this were to happen our\nassumptions about how the API works have been invalidated.\n\nThe pcre2_jit_compile() doesn't promise to return a finite set of error\ncodes, but:\n\n\t[...]0 for success, or a negative error code otherwise[...]\n\nBut if new codes were added it's anyone's guess what state we'd be in,\nso I think the safe thing is to BUG() out if we get as far as\npcre2_jit_compile() and don't get either PCRE2_ERROR_JIT_BADOPTION or\nPCRE2_ERROR_NOMEMORY.\n\n>> Receiving BADOPTION could be a sign that there is something wrong in\n>> the input, not from the end-user but from the code, in which case\n>> stopping with BUG() may be a more appropriate?\n>\n> The way PCRE handles this kind of errors internally is to instruct pcre2_match()\n> to use the interpreter.\n>\n> While a BUG() might be a way to ensure the code is using the right set\n> of options\n> I would expect that the failure will be reported by pcre2_compile\n> instead, with the\n> only cases left, only being interna to PCRE (ex: JIT can't yet support\n> a feature the\n> interpreter has)\n\nI agree that it's possible in general that an external library might\nstart returning a \"benign\" error code that we could recover from, so\nBUG(...) would be overdoing it.\n\nI just don't see that libpcre would do that in this case. In general the\nJIT supports all patterns that the normal engine does, so if we're past\nthe \"is it available?\" hump it should work.\n\nIf it starts not doing that I'd think they'd do a major version upgrade,\nor opt-in with a new flag etc.\n\nNote that it already has a way of checking for \"we tried to do the jit\nthing, but it wasn't on in the end\". See the code I added in\na25b9085043[1] (grep: fix segfault under -P + PCRE2 <=10.30 + (*NO_JIT),\n2017-11-23), which comes right after the pcre2_jit_compile().\n\nSo not only would a BUG() biting us here require them to create a new\ncode for the state of \"we have the JIT, but can't use it here\" (for some\nreason I can't imagine, as \"PCRE2_ERROR_NOMEMORY\" is already\n\"overloaded\" to mean that).\n\nIt would also require them to invent a new \"soft\" failure mode for the\nJIT, i.e. not the facility added in a25b9085043, where we can use the\nJIT, but it's not on after all due to a \"(*NO_JIT)\" in the pattern\nitself.\n\nBut I may be wrong, so I've CC'd pcre-dev@ to see if they have any\ncommentary on this proposed API paranoia. For them: The greater context\nof this thread on the git ML is at [2].\n\n1. https://github.com/git/git/commit/a25b9085043b8029169b4d5b172b78ca3f38fb37\n2. https://lore.kernel.org/git/20221216121557.30714-1-minipli@grsecurity.net/\n"},{"id":"469391","messageId":"2b04b19a-a2bd-3dd5-6f21-ed0b0ad3e02f@grsecurity.net","threadId":"58967","inReplyTo":"221219.86bknz21qj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2022-12-20T19:29:59Z","receivedAt":"2022-12-20T19:30:49Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"Am 19.12.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:\n> \n> On Fri, Dec 16 2022, Carlo Arenas wrote:\n> \n> [CC-ing pcre-dev@ for this \"future error API\" discussion]\n> \n>> On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano wrote:\n>>>\n>>> Mathias Krause writes:\n>>>\n>>>> ... However, from a user's point of view a fall back to\n>>>> interpreter mode might still be desired in this case, as a failing\n>>>> 'git grep' is simply not acceptable, IMHO.\n>>>\n>>> \"git grep\" that silently produces a wrong result (by falling back\n>>> after a problem is detected) would not be acceptable, either.\n>>\n>> except that an error at this point only invalidates the use of JIT,\n>> so calling pcre2_jit_match() is invalid but calling pcre2_match() is not.\n>>\n>> the later is setup to be used later by the code that is added,\n> \n> I think we could stumble ahead, but if this were to happen our\n> assumptions about how the API works have been invalidated.\n\nWell, pcre2_jit_compile() might fail for internal reasons, e.g.\npcre2jit(3) states: \"[...] If a pattern is too big, a call to\npcre2_jit_compile() returns PCRE2_ERROR_NOMEMORY.\"\n\nFor example, the following fails for me:\n$ git grep -P \"$(perl -e 'print \"(.)\" x 4000')\" -- grep.c\nfatal: Couldn't JIT the PCRE2 pattern '(.)(.)(.)(.)…\n\nBut explicitly disabling JIT makes it \"work\":\n$ git grep -P \"(*NO_JIT)$(perl -e 'print \"(.)\" x 4000')\" -- grep.c\n$\n\nIt's a made up example and might even be intended behavior by git, but\nit also proves a point Carlo already mentioned, a failing call to\npcre2_jit_compile() only invalidates the use of the JIT engine. We can\nstill use and fall back to the interpreter.\n\nIt would be used anyway if PCRE2 was compiled without JIT support, so I\ndon't see any issues with falling back to interpreter mode if the JIT\ncompilation fails -- for whatever reason.\n\n> The pcre2_jit_compile() doesn't promise to return a finite set of error\n> codes, but:\n> \n> \t[...]0 for success, or a negative error code otherwise[...]\n> \n> But if new codes were added it's anyone's guess what state we'd be in,\n> so I think the safe thing is to BUG() out if we get as far as\n> pcre2_jit_compile() and don't get either PCRE2_ERROR_JIT_BADOPTION or\n> PCRE2_ERROR_NOMEMORY.\n\nBut why BUG()? JIT is an optimization that might fail for PCRE2 internal\nreasons. Why should we make 'git grep' fail too in this case when we can\nhandle it just fine by attempting to use the interpreter?\n\nIf the pattern is really bogus, the interpreter will complain as well\nand we'll error out. But failing just because the JIT engine can't\nhandle the pattern? Doesn't sound right to me.\n\n>>> Receiving BADOPTION could be a sign that there is something wrong in\n>>> the input, not from the end-user but from the code, in which case\n>>> stopping with BUG() may be a more appropriate?\n>>\n>> The way PCRE handles this kind of errors internally is to instruct pcre2_match()\n>> to use the interpreter.\n>>\n>> While a BUG() might be a way to ensure the code is using the right set\n>> of options\n>> I would expect that the failure will be reported by pcre2_compile\n>> instead, with the\n>> only cases left, only being interna to PCRE (ex: JIT can't yet support\n>> a feature the\n>> interpreter has)\n> \n> I agree that it's possible in general that an external library might\n> start returning a \"benign\" error code that we could recover from, so\n> BUG(...) would be overdoing it.\n\nAnd I think that's the case here: JIT is an optimization that might not\nbe available under all circumstances, as, for example, under SELinux's\n'deny_execmem' setting. So we need to have a backup plan for such\nsystems anyway. Why not always try to use the interpreter if JIT\ncompilation fails?\n\n> I just don't see that libpcre would do that in this case. In general the\n> JIT supports all patterns that the normal engine does, so if we're past\n> the \"is it available?\" hump it should work.\n\nLeaving my above example aside, showing that JIT cannot handle very long\npatterns the interpreter mode does, it is, in fact, unfortunate that\nlibpcre2 announces JIT support when it could know better that it does not.\n\nIn fact, I also try to fix this misbehavior in PCRE2/SLJIT[1,2], but the\nmaintainer thinks it's a bug in git which should be fixed by handling\nerrors from pcre2_jit_compile() and falling back to interpreter mode[3].\n\nWhile I still think this can pretty much be fixed in PCRE2 alone,\nhandling possible PCRE2 JIT errors in git and falling back to\ninterpreter mode is still a sensible thing to do.\n\n[1] https://github.com/PCRE2Project/pcre2/pull/157\n[2] https://github.com/zherczeg/sljit/pull/136\n[3] https://github.com/zherczeg/sljit/pull/136#issuecomment-1307254018\n\n> If it starts not doing that I'd think they'd do a major version upgrade,\n> or opt-in with a new flag etc.\n> \n> Note that it already has a way of checking for \"we tried to do the jit\n> thing, but it wasn't on in the end\". See the code I added in\n> a25b9085043[1] (grep: fix segfault under -P + PCRE2 <=10.30 + (*NO_JIT),\n> 2017-11-23), which comes right after the pcre2_jit_compile().\n\nYeah, that's required as well, unfortunately. :/\n\n> So not only would a BUG() biting us here require them to create a new\n> code for the state of \"we have the JIT, but can't use it here\" (for some\n> reason I can't imagine, as \"PCRE2_ERROR_NOMEMORY\" is already\n> \"overloaded\" to mean that).\n> \n> It would also require them to invent a new \"soft\" failure mode for the\n> JIT, i.e. not the facility added in a25b9085043, where we can use the\n> JIT, but it's not on after all due to a \"(*NO_JIT)\" in the pattern\n> itself.\n\nWe should really treat PCRE2 JIT as an *optional* optimization that\nmight not be available for certain cases. For these we should, IMHO,\nsimply use the interpreter mode, instead of bugging users with a BUG() /\ndie().\n\n> But I may be wrong, so I've CC'd pcre-dev@ to see if they have any\n> commentary on this proposed API paranoia. For them: The greater context\n> of this thread on the git ML is at [2].\n> \n> 1. https://github.com/git/git/commit/a25b9085043b8029169b4d5b172b78ca3f38fb37\n> 2. https://lore.kernel.org/git/20221216121557.30714-1-minipli@grsecurity.net/\n\nThanks,\nMathias\n"},{"id":"469396","messageId":"7a657644-cd7c-8638-55aa-667c528a5624@grsecurity.net","threadId":"58967","inReplyTo":"xmqq359fm06c.fsf@gitster.g","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2022-12-20T20:40:47Z","receivedAt":"2022-12-20T20:40:57Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"Am 16.12.22 um 23:52 schrieb Junio C Hamano:\n> Mathias Krause writes:\n> \n>> Such a change was already proposed 4 years ago [1] but wasn't merged for\n>> unknown reasons.\n>>\n>> 1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com\n> \n> This part does not belong to the log message but should go below\n> three-dash line. If you have a more concrete \"it was rejected\n> because ...\", to help judging why this version improves upon the\n> previous attempt and it is clear the reason for past rejection no\n> longer applies, that is a very different story, though.\n> \n> The way I read the original thread (assuming that the lore archive\n> is showing all relevant messages there) is that RFC was reviewed\n> well and everybody was happy about the direction it took. It even\n> received fairly concrete suggestions for the real, non-RFC version,\n> but that never materialized.\n\nIt had a follow-up RFC[1] half a year later, adding a config option and,\nafter some more discussion, even command line switches[2]. But not just\nIMHO is that far too much, but even you suggested to simply revert back\nto the initial RFC and implement the automatic fallback[3], basically\nmerging it with a proper changelog[4]. As that never happened, I took up\nthe work and tried to do just that.\n\n1. https://lore.kernel.org/git/20190728235427.41425-1-carenas@gmail.com/\n2. https://lore.kernel.org/git/20190729105955.44390-1-carenas@gmail.com/\n3. https://lore.kernel.org/git/xmqqh874vikk.fsf@gitster-ct.c.googlers.com/\n4. https://lore.kernel.org/git/xmqqef1zmkp5.fsf@gitster-ct.c.googlers.com/\n\n> It is very clear that the posted patch was not yet in a mergeable\n> state as-is; \"for unknown reasons\" is less than helpful.\n> \n> Now, we learned a more concrete reason, i.e. \"it got tons of help\n> improving the draft into the final version, but the help was\n> discarded and the final version did not materialize\", does the\n> attempt this time around improve on it sufficiently to make it\n> mergeable, or is it sufficiently different that it needs a fresh\n> review from scratch? If the latter, then \"for unknown reasons\"\n> becomes even less relevant.\n\nSorry for not digging up that information for the initial patch\nsubmission. I'll add it to v2.\n\n> The rest of the proposed log message, and the change itself, both\n> look very reasonable and well explained, at least to me.\n\nThanks a lot for the detailed review. I'll update the commit log\naccordingly but will wait for more feedback to see on which solution we\nwant to settle on.\n\n> Thanks.\n> \n> \n>> Signed-off-by: Carlo Marcelo Arenas Belón \n>> Signed-off-by: Mathias Krause \t# tweaked changelog, added comment\n>> ---\n>> grep.c | 17 +++++++++++++++--\n>> 1 file changed, 15 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/grep.c b/grep.c\n>> index 06eed694936c..f2ada528b21d 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -317,8 +317,21 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>> \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n>> \tif (p->pcre2_jit_on) {\n>> \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n>> -\t\tif (jitret)\n>> -\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n>> +\t\tif (jitret) {\n>> +\t\t\t/*\n>> +\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n>> +\t\t\t * indicated JIT support, the library might still\n>> +\t\t\t * fail to generate JIT code for various reasons,\n>> +\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n>> +\t\t\t * MPROTECT prevent creating W|X memory mappings.\n>> +\t\t\t *\n>> +\t\t\t * Instead of faling hard, fall back to interpreter\n>> +\t\t\t * mode, just as if the pattern was prefixed with\n>> +\t\t\t * '(*NO_JIT)'.\n>> +\t\t\t */\n>> +\t\t\tp->pcre2_jit_on = 0;\n>> +\t\t\treturn;\n>> +\t\t}\n>> \n>> \t\t/*\n>> \t\t * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just\n"},{"id":"469401","messageId":"221220.86bknxwy9t.gmgdl@evledraar.gmail.com","threadId":"58967","inReplyTo":"2b04b19a-a2bd-3dd5-6f21-ed0b0ad3e02f@grsecurity.net","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-20T21:11:28Z","receivedAt":"2022-12-20T21:39:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 20 2022, Mathias Krause wrote:\n\n[De-CC-ing pcre-dev@, since this part is all git-specific]\n\n> Am 19.12.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:\n>> \n>> On Fri, Dec 16 2022, Carlo Arenas wrote:\n>> \n>> [CC-ing pcre-dev@ for this \"future error API\" discussion]\n>> \n>>> On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano wrote:\n>>>>\n>>>> Mathias Krause writes:\n>>>>\n>>>>> ... However, from a user's point of view a fall back to\n>>>>> interpreter mode might still be desired in this case, as a failing\n>>>>> 'git grep' is simply not acceptable, IMHO.\n>>>>\n>>>> \"git grep\" that silently produces a wrong result (by falling back\n>>>> after a problem is detected) would not be acceptable, either.\n>>>\n>>> except that an error at this point only invalidates the use of JIT,\n>>> so calling pcre2_jit_match() is invalid but calling pcre2_match() is not.\n>>>\n>>> the later is setup to be used later by the code that is added,\n>> \n>> I think we could stumble ahead, but if this were to happen our\n>> assumptions about how the API works have been invalidated.\n>\n> Well, pcre2_jit_compile() might fail for internal reasons, e.g.\n> pcre2jit(3) states: \"[...] If a pattern is too big, a call to\n> pcre2_jit_compile() returns PCRE2_ERROR_NOMEMORY.\"\n>\n> For example, the following fails for me:\n> $ git grep -P \"$(perl -e 'print \"(.)\" x 4000')\" -- grep.c\n> fatal: Couldn't JIT the PCRE2 pattern '(.)(.)(.)(.)…\n>\n> But explicitly disabling JIT makes it \"work\":\n> $ git grep -P \"(*NO_JIT)$(perl -e 'print \"(.)\" x 4000')\" -- grep.c\n> $\n>\n> It's a made up example and might even be intended behavior by git, but\n> it also proves a point Carlo already mentioned, a failing call to\n> pcre2_jit_compile() only invalidates the use of the JIT engine. We can\n> still use and fall back to the interpreter.\n\nWe should arguably do this, I hadn't bothered because I haven't been\nable to find anything except pathological patterns where it matters, and\nsilently falling back in those cases will suck a lot more IMO.\n\nIf you are using such a pathological pattern it's almost always a better\nidea to adjust your crazy pattern.\n\nSo I think in the *general* case we really should just keep this, and\n*maybe* suggest the user try with (*NO_JIT) in the pattern.\n\nBut silently falling back kind of sucks, but unfortunately pcre2 doesn't\nprovide a way to say \"failed because of SELinux\" v.s. \"failed because\nthe pattern is crazy\", except that we could try to compile a known-good\npattern with the JIT, to disambiguate the two.\n\nAnyway, if this is your goal you should really lead with that, not with\nfixing a relatively obscure SELinux edge case...\n\n> It would be used anyway if PCRE2 was compiled without JIT support, so I\n> don't see any issues with falling back to interpreter mode if the JIT\n> compilation fails -- for whatever reason.\n\nIt's the \"for whatever reason\" that I take issue with. We'd be in an\nunknown state with the API behaving differently than we expect, and\nreturning unknown codes. That's different than the *known* error codes\n(e.g. \"no memory\", oven though it's meaning is apparently overloaded to\nthe point of near-uselessness).\n\n>> The pcre2_jit_compile() doesn't promise to return a finite set of error\n>> codes, but:\n>> \n>> \t[...]0 for success, or a negative error code otherwise[...]\n>> \n>> But if new codes were added it's anyone's guess what state we'd be in,\n>> so I think the safe thing is to BUG() out if we get as far as\n>> pcre2_jit_compile() and don't get either PCRE2_ERROR_JIT_BADOPTION or\n>> PCRE2_ERROR_NOMEMORY.\n>\n> But why BUG()? JIT is an optimization that might fail for PCRE2 internal\n> reasons. Why should we make 'git grep' fail too in this case when we can\n> handle it just fine by attempting to use the interpreter?\n>\n> If the pattern is really bogus, the interpreter will complain as well\n> and we'll error out. But failing just because the JIT engine can't\n> handle the pattern? Doesn't sound right to me.\n\nSee above, we're failing because our assumptions about how to use the\nAPI have broken down at that point. We usually bug out in those cases.\n\n>>>> Receiving BADOPTION could be a sign that there is something wrong in\n>>>> the input, not from the end-user but from the code, in which case\n>>>> stopping with BUG() may be a more appropriate?\n>>>\n>>> The way PCRE handles this kind of errors internally is to instruct pcre2_match()\n>>> to use the interpreter.\n>>>\n>>> While a BUG() might be a way to ensure the code is using the right set\n>>> of options\n>>> I would expect that the failure will be reported by pcre2_compile\n>>> instead, with the\n>>> only cases left, only being interna to PCRE (ex: JIT can't yet support\n>>> a feature the\n>>> interpreter has)\n>> \n>> I agree that it's possible in general that an external library might\n>> start returning a \"benign\" error code that we could recover from, so\n>> BUG(...) would be overdoing it.\n>\n> And I think that's the case here: JIT is an optimization that might not\n> be available under all circumstances, as, for example, under SELinux's\n> 'deny_execmem' setting. So we need to have a backup plan for such\n> systems anyway. Why not always try to use the interpreter if JIT\n> compilation fails?\n\nSee above, but maybe it's the least sucky thing (and definitely\nsimpler). I'm mainly checking that we're doing that we want here, and\nthat we're going into it with eyes open.\n\nThat we're now discussing a topic entirely different from SELinux on a\nthread where we're (according to the commit message) fixing pcre2 where\nthe JIT is \"unusable on such systems\" is my main concern here. \n\n>> So not only would a BUG() biting us here require them to create a new\n>> code for the state of \"we have the JIT, but can't use it here\" (for some\n>> reason I can't imagine, as \"PCRE2_ERROR_NOMEMORY\" is already\n>> \"overloaded\" to mean that).\n>> \n>> It would also require them to invent a new \"soft\" failure mode for the\n>> JIT, i.e. not the facility added in a25b9085043, where we can use the\n>> JIT, but it's not on after all due to a \"(*NO_JIT)\" in the pattern\n>> itself.\n>\n> We should really treat PCRE2 JIT as an *optional* optimization that\n> might not be available for certain cases. For these we should, IMHO,\n> simply use the interpreter mode, instead of bugging users with a BUG() /\n> die().\n\nTo summarize some of the above, I think performance also matters, we\nhave cases where:\n\n A. We could use the non-JIT\n B. We could use the JIT, and it's a *lot* faster\n C. We can't use the JIT at all\n D. We can't use the JIT because we run into its limits\n\nI think it's fair to die on \"D\" as in practice you only (I think!) run\ninto it on pathological patterns, but yes, another option would be to\nfall back to \"A\".\n\nBut thinking you're doing \"B\" and not wanting to implicitly fall back to\n\"A\" is also a valid use-case.\n\nSo I'm inclined to suggest that we should be less helpful with automatic\nfallbacks, and just suggest a \"try it with '(*NO_JIT)'\" advice() or\nsomething.\n\nBut as noted above needing to always disable an apparently \"available\"\nJIT on some systems (SELinux) does throw a monkey wrench into that\nparticular suggestion :(\n\nSo I'm not sure, I'm mainly trying to encourage you to think through the\nedge cases, and to summarize the full impact of the change in a re-roll.\n"},{"id":"470622","messageId":"f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net","threadId":"58967","inReplyTo":"221220.86bknxwy9t.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-18T14:22:12Z","receivedAt":"2023-01-18T14:34:10Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 20.12.22 22:11, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Dec 20 2022, Mathias Krause wrote:\n> \n> [De-CC-ing pcre-dev@, since this part is all git-specific]\n> \n>> Am 19.12.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:\n>>>\n>>> On Fri, Dec 16 2022, Carlo Arenas wrote:\n>>>\n>>> [CC-ing pcre-dev@ for this \"future error API\" discussion]\n>>>\n>>>> On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano wrote:\n>>>>>\n>>>>> Mathias Krause writes:\n>>>>>\n>>>>>> ... However, from a user's point of view a fall back to\n>>>>>> interpreter mode might still be desired in this case, as a failing\n>>>>>> 'git grep' is simply not acceptable, IMHO.\n>>>>>\n>>>>> \"git grep\" that silently produces a wrong result (by falling back\n>>>>> after a problem is detected) would not be acceptable, either.\n>>>>\n>>>> except that an error at this point only invalidates the use of JIT,\n>>>> so calling pcre2_jit_match() is invalid but calling pcre2_match() is not.\n>>>>\n>>>> the later is setup to be used later by the code that is added,\n>>>\n>>> I think we could stumble ahead, but if this were to happen our\n>>> assumptions about how the API works have been invalidated.\n>>\n>> Well, pcre2_jit_compile() might fail for internal reasons, e.g.\n>> pcre2jit(3) states: \"[...] If a pattern is too big, a call to\n>> pcre2_jit_compile() returns PCRE2_ERROR_NOMEMORY.\"\n>>\n>> For example, the following fails for me:\n>> $ git grep -P \"$(perl -e 'print \"(.)\" x 4000')\" -- grep.c\n>> fatal: Couldn't JIT the PCRE2 pattern '(.)(.)(.)(.)…\n>>\n>> But explicitly disabling JIT makes it \"work\":\n>> $ git grep -P \"(*NO_JIT)$(perl -e 'print \"(.)\" x 4000')\" -- grep.c\n>> $\n>>\n>> It's a made up example and might even be intended behavior by git, but\n>> it also proves a point Carlo already mentioned, a failing call to\n>> pcre2_jit_compile() only invalidates the use of the JIT engine. We can\n>> still use and fall back to the interpreter.\n> \n> We should arguably do this, I hadn't bothered because I haven't been\n> able to find anything except pathological patterns where it matters, and\n> silently falling back in those cases will suck a lot more IMO.\n> \n> If you are using such a pathological pattern it's almost always a better\n> idea to adjust your crazy pattern.\n> \n> So I think in the *general* case we really should just keep this, and\n> *maybe* suggest the user try with (*NO_JIT) in the pattern.\n\nExcept for the case I'm trying to address, where we simply cannot detect\n*why* the JIT is failing from the error code alone. It'll error out with\nPCRE2_ERROR_NOMEMORY for the case where the JIT fails to allocate W|X\nmemory as well as for the case where the pattern is crazy.\n\n> But silently falling back kind of sucks, but unfortunately pcre2 doesn't\n> provide a way to say \"failed because of SELinux\" v.s. \"failed because\n> the pattern is crazy\", except that we could try to compile a known-good\n> pattern with the JIT, to disambiguate the two.\n\nExactly, so what about something like this:\n\nIf JIT is generally available, try to JIT the user supplied pattern:\n1/ If it fails with PCRE2_ERROR_NOMEMORY, try to compile a know good\n pattern, e.g. \".\":\n 1a/ If this fails with PCRE2_ERROR_NOMEMORY as well, fall back to\n interpreter, as JIT is non-functional because of SELinux / PaX.\n 1b/ If not, it's a \"crazy\" pattern, suggest '(*NO_JIT)'.\n2/ If it succeeds or fails with a different error, continue as of now,\n i.e. use JIT on success or die() on error, optionally suggesting\n '(*NO_JIT)'.\n\nThat should handle the case you're concerned about and only fall back to\ninterpreter mode if JIT won't be functional anyway. Admitted, this would\nalso allow crazy patterns, but there's simply no way to detect these\nunder such constraints.\n\n> Anyway, if this is your goal you should really lead with that, not with\n> fixing a relatively obscure SELinux edge case...\n\nIt's just that I'm suffering from that \"obscure SELinux edge case\", just\nnot under SELinux but PaX MPROTECT. I only mentioned SELinux to state,\nit's not only an issue for PaX but regular systems as well. So it's not\na hypothetical case I like to get handled, but my work environment to\nget usable again.\n\n>> It would be used anyway if PCRE2 was compiled without JIT support, so I\n>> don't see any issues with falling back to interpreter mode if the JIT\n>> compilation fails -- for whatever reason.\n> \n> It's the \"for whatever reason\" that I take issue with. We'd be in an\n> unknown state with the API behaving differently than we expect, and\n> returning unknown codes. That's different than the *known* error codes\n> (e.g. \"no memory\", oven though it's meaning is apparently overloaded to\n> the point of near-uselessness).\n> \n>>> The pcre2_jit_compile() doesn't promise to return a finite set of error\n>>> codes, but:\n>>>\n>>> \t[...]0 for success, or a negative error code otherwise[...]\n>>>\n>>> But if new codes were added it's anyone's guess what state we'd be in,\n>>> so I think the safe thing is to BUG() out if we get as far as\n>>> pcre2_jit_compile() and don't get either PCRE2_ERROR_JIT_BADOPTION or\n>>> PCRE2_ERROR_NOMEMORY.\n>>\n>> But why BUG()? JIT is an optimization that might fail for PCRE2 internal\n>> reasons. Why should we make 'git grep' fail too in this case when we can\n>> handle it just fine by attempting to use the interpreter?\n>>\n>> If the pattern is really bogus, the interpreter will complain as well\n>> and we'll error out. But failing just because the JIT engine can't\n>> handle the pattern? Doesn't sound right to me.\n> \n> See above, we're failing because our assumptions about how to use the\n> API have broken down at that point. We usually bug out in those cases.\n> \n>>>>> Receiving BADOPTION could be a sign that there is something wrong in\n>>>>> the input, not from the end-user but from the code, in which case\n>>>>> stopping with BUG() may be a more appropriate?\n>>>>\n>>>> The way PCRE handles this kind of errors internally is to instruct pcre2_match()\n>>>> to use the interpreter.\n>>>>\n>>>> While a BUG() might be a way to ensure the code is using the right set\n>>>> of options\n>>>> I would expect that the failure will be reported by pcre2_compile\n>>>> instead, with the\n>>>> only cases left, only being interna to PCRE (ex: JIT can't yet support\n>>>> a feature the\n>>>> interpreter has)\n>>>\n>>> I agree that it's possible in general that an external library might\n>>> start returning a \"benign\" error code that we could recover from, so\n>>> BUG(...) would be overdoing it.\n>>\n>> And I think that's the case here: JIT is an optimization that might not\n>> be available under all circumstances, as, for example, under SELinux's\n>> 'deny_execmem' setting. So we need to have a backup plan for such\n>> systems anyway. Why not always try to use the interpreter if JIT\n>> compilation fails?\n> \n> See above, but maybe it's the least sucky thing (and definitely\n> simpler). I'm mainly checking that we're doing that we want here, and\n> that we're going into it with eyes open.\n> \n> That we're now discussing a topic entirely different from SELinux on a\n> thread where we're (according to the commit message) fixing pcre2 where\n> the JIT is \"unusable on such systems\" is my main concern here. \n\nYeah, I overlooked that angle initially, but it's a valid concern.\nHowever, limiting the functional change of falling back to interpreter\nmode on \"JIT's broken anyway\" systems only should address these and get\nme a functional 'git grep' again.\n\n>>> So not only would a BUG() biting us here require them to create a new\n>>> code for the state of \"we have the JIT, but can't use it here\" (for some\n>>> reason I can't imagine, as \"PCRE2_ERROR_NOMEMORY\" is already\n>>> \"overloaded\" to mean that).\n>>>\n>>> It would also require them to invent a new \"soft\" failure mode for the\n>>> JIT, i.e. not the facility added in a25b9085043, where we can use the\n>>> JIT, but it's not on after all due to a \"(*NO_JIT)\" in the pattern\n>>> itself.\n>>\n>> We should really treat PCRE2 JIT as an *optional* optimization that\n>> might not be available for certain cases. For these we should, IMHO,\n>> simply use the interpreter mode, instead of bugging users with a BUG() /\n>> die().\n> \n> To summarize some of the above, I think performance also matters, we\n> have cases where:\n> \n> A. We could use the non-JIT\n> B. We could use the JIT, and it's a *lot* faster\n> C. We can't use the JIT at all\n> D. We can't use the JIT because we run into its limits\n> \n> I think it's fair to die on \"D\" as in practice you only (I think!) run\n> into it on pathological patterns, but yes, another option would be to\n> fall back to \"A\".\n> \n> But thinking you're doing \"B\" and not wanting to implicitly fall back to\n> \"A\" is also a valid use-case.\n\nAgreed. My above sketched handling should do that, as in not falling\nback to interpreter mode when the JIT would be functional per se, but\njust failed on this particular pattern.\n\n> So I'm inclined to suggest that we should be less helpful with automatic\n> fallbacks, and just suggest a \"try it with '(*NO_JIT)'\" advice() or\n> something.\n\nWell, that's a real pain from a user's (my) point of view. Trust me, I'm\nsuffering from this, otherwise I wouldn't have brought up the issue ;)\n\n> But as noted above needing to always disable an apparently \"available\"\n> JIT on some systems (SELinux) does throw a monkey wrench into that\n> particular suggestion :(\n\nYep.\n\n> So I'm not sure, I'm mainly trying to encourage you to think through the\n> edge cases, and to summarize the full impact of the change in a re-roll.\n\nYeah, I agree. The implied fallback to the interpreter, even for the\nmore obscure cases should have been mentioned in the changelog, but I\noverlooked / ignored that case initially.\n\nMy take from the discussion is to do a re-roll with something like this:\n\n if (p->pcre2_jit_on) {\n jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {\n /* JIT's non-functional because of SELinux / PaX */\n p->pcre2_jit_on = 0;\n return;\n } else if (jitret) {\n die(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\"\n \"Try prefixing the pattern with '(*NO_JIT)'\\n\",\n p->pattern, jitret);\n }\n ...\n }\n\n...with pcre2_jit_functional() being something like this:\n\n static int pcre2_jit_functional(void)\n {\n pcre2_code *code;\n size_t off;\n int err;\n\n code = pcre2_compile((PCRE2_SPTR)\".\", 1, 0, &err, &off, NULL);\n if (!code)\n return 0;\n\n err = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE);\n pcre2_code_free(code);\n\n return err == 0;\n }\n\n\nThanks,\nMathias\n"},{"id":"470626","messageId":"230118.86lelzx2c4.gmgdl@evledraar.gmail.com","threadId":"58967","inReplyTo":"f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-18T15:44:54Z","receivedAt":"2023-01-18T16:01:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 18 2023, Mathias Krause wrote:\n\n> On 20.12.22 22:11, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Tue, Dec 20 2022, Mathias Krause wrote:\n>> \n>> [De-CC-ing pcre-dev@, since this part is all git-specific]\n>> \n>>> Am 19.12.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:\n>>>>\n>>>> On Fri, Dec 16 2022, Carlo Arenas wrote:\n>>>>\n>>>> [CC-ing pcre-dev@ for this \"future error API\" discussion]\n>>>>\n>>>>> On Fri, Dec 16, 2022 at 3:09 PM Junio C Hamano wrote:\n>>>>>>\n>>>>>> Mathias Krause writes:\n>>>>>>\n>>>>>>> ... However, from a user's point of view a fall back to\n>>>>>>> interpreter mode might still be desired in this case, as a failing\n>>>>>>> 'git grep' is simply not acceptable, IMHO.\n>>>>>>\n>>>>>> \"git grep\" that silently produces a wrong result (by falling back\n>>>>>> after a problem is detected) would not be acceptable, either.\n>>>>>\n>>>>> except that an error at this point only invalidates the use of JIT,\n>>>>> so calling pcre2_jit_match() is invalid but calling pcre2_match() is not.\n>>>>>\n>>>>> the later is setup to be used later by the code that is added,\n>>>>\n>>>> I think we could stumble ahead, but if this were to happen our\n>>>> assumptions about how the API works have been invalidated.\n>>>\n>>> Well, pcre2_jit_compile() might fail for internal reasons, e.g.\n>>> pcre2jit(3) states: \"[...] If a pattern is too big, a call to\n>>> pcre2_jit_compile() returns PCRE2_ERROR_NOMEMORY.\"\n>>>\n>>> For example, the following fails for me:\n>>> $ git grep -P \"$(perl -e 'print \"(.)\" x 4000')\" -- grep.c\n>>> fatal: Couldn't JIT the PCRE2 pattern '(.)(.)(.)(.)…\n>>>\n>>> But explicitly disabling JIT makes it \"work\":\n>>> $ git grep -P \"(*NO_JIT)$(perl -e 'print \"(.)\" x 4000')\" -- grep.c\n>>> $\n>>>\n>>> It's a made up example and might even be intended behavior by git, but\n>>> it also proves a point Carlo already mentioned, a failing call to\n>>> pcre2_jit_compile() only invalidates the use of the JIT engine. We can\n>>> still use and fall back to the interpreter.\n>> \n>> We should arguably do this, I hadn't bothered because I haven't been\n>> able to find anything except pathological patterns where it matters, and\n>> silently falling back in those cases will suck a lot more IMO.\n>> \n>> If you are using such a pathological pattern it's almost always a better\n>> idea to adjust your crazy pattern.\n>> \n>> So I think in the *general* case we really should just keep this, and\n>> *maybe* suggest the user try with (*NO_JIT) in the pattern.\n>\n> Except for the case I'm trying to address, where we simply cannot detect\n> *why* the JIT is failing from the error code alone. It'll error out with\n> PCRE2_ERROR_NOMEMORY for the case where the JIT fails to allocate W|X\n> memory as well as for the case where the pattern is crazy.\n\n*nod*, though I see you saw that I addressed that below.\n\n>> But silently falling back kind of sucks, but unfortunately pcre2 doesn't\n>> provide a way to say \"failed because of SELinux\" v.s. \"failed because\n>> the pattern is crazy\", except that we could try to compile a known-good\n>> pattern with the JIT, to disambiguate the two.\n>\n> Exactly, so what about something like this:\n>\n> If JIT is generally available, try to JIT the user supplied pattern:\n> 1/ If it fails with PCRE2_ERROR_NOMEMORY, try to compile a know good\n> pattern, e.g. \".\":\n> 1a/ If this fails with PCRE2_ERROR_NOMEMORY as well, fall back to\n> interpreter, as JIT is non-functional because of SELinux / PaX.\n> 1b/ If not, it's a \"crazy\" pattern, suggest '(*NO_JIT)'.\n> 2/ If it succeeds or fails with a different error, continue as of now,\n> i.e. use JIT on success or die() on error, optionally suggesting\n> '(*NO_JIT)'.\n>\n> That should handle the case you're concerned about and only fall back to\n> interpreter mode if JIT won't be functional anyway. Admitted, this would\n> also allow crazy patterns, but there's simply no way to detect these\n> under such constraints.\n\nThat sounds good, i.e. we could narrow the JIT falling back case to\nthese SELinux cases and the like, distinct from generic internal errors.\n\nMaybe it's too much paranoia, but it should work & get rid of the\nambiguity.\n\n>> Anyway, if this is your goal you should really lead with that, not with\n>> fixing a relatively obscure SELinux edge case...\n>\n> It's just that I'm suffering from that \"obscure SELinux edge case\", just\n> not under SELinux but PaX MPROTECT. I only mentioned SELinux to state,\n> it's not only an issue for PaX but regular systems as well. So it's not\n> a hypothetical case I like to get handled, but my work environment to\n> get usable again.\n\nRight, I didn't mean to imply it was, just that for the rest of us the\nmore general effect of this change outside of SELinux is of more general\ninterest.\n\n>>> It would be used anyway if PCRE2 was compiled without JIT support, so I\n>>> don't see any issues with falling back to interpreter mode if the JIT\n>>> compilation fails -- for whatever reason.\n>> \n>> It's the \"for whatever reason\" that I take issue with. We'd be in an\n>> unknown state with the API behaving differently than we expect, and\n>> returning unknown codes. That's different than the *known* error codes\n>> (e.g. \"no memory\", oven though it's meaning is apparently overloaded to\n>> the point of near-uselessness).\n>> \n>>>> The pcre2_jit_compile() doesn't promise to return a finite set of error\n>>>> codes, but:\n>>>>\n>>>> \t[...]0 for success, or a negative error code otherwise[...]\n>>>>\n>>>> But if new codes were added it's anyone's guess what state we'd be in,\n>>>> so I think the safe thing is to BUG() out if we get as far as\n>>>> pcre2_jit_compile() and don't get either PCRE2_ERROR_JIT_BADOPTION or\n>>>> PCRE2_ERROR_NOMEMORY.\n>>>\n>>> But why BUG()? JIT is an optimization that might fail for PCRE2 internal\n>>> reasons. Why should we make 'git grep' fail too in this case when we can\n>>> handle it just fine by attempting to use the interpreter?\n>>>\n>>> If the pattern is really bogus, the interpreter will complain as well\n>>> and we'll error out. But failing just because the JIT engine can't\n>>> handle the pattern? Doesn't sound right to me.\n>> \n>> See above, we're failing because our assumptions about how to use the\n>> API have broken down at that point. We usually bug out in those cases.\n>> \n>>>>>> Receiving BADOPTION could be a sign that there is something wrong in\n>>>>>> the input, not from the end-user but from the code, in which case\n>>>>>> stopping with BUG() may be a more appropriate?\n>>>>>\n>>>>> The way PCRE handles this kind of errors internally is to instruct pcre2_match()\n>>>>> to use the interpreter.\n>>>>>\n>>>>> While a BUG() might be a way to ensure the code is using the right set\n>>>>> of options\n>>>>> I would expect that the failure will be reported by pcre2_compile\n>>>>> instead, with the\n>>>>> only cases left, only being interna to PCRE (ex: JIT can't yet support\n>>>>> a feature the\n>>>>> interpreter has)\n>>>>\n>>>> I agree that it's possible in general that an external library might\n>>>> start returning a \"benign\" error code that we could recover from, so\n>>>> BUG(...) would be overdoing it.\n>>>\n>>> And I think that's the case here: JIT is an optimization that might not\n>>> be available under all circumstances, as, for example, under SELinux's\n>>> 'deny_execmem' setting. So we need to have a backup plan for such\n>>> systems anyway. Why not always try to use the interpreter if JIT\n>>> compilation fails?\n>> \n>> See above, but maybe it's the least sucky thing (and definitely\n>> simpler). I'm mainly checking that we're doing that we want here, and\n>> that we're going into it with eyes open.\n>> \n>> That we're now discussing a topic entirely different from SELinux on a\n>> thread where we're (according to the commit message) fixing pcre2 where\n>> the JIT is \"unusable on such systems\" is my main concern here. \n>\n> Yeah, I overlooked that angle initially, but it's a valid concern.\n> However, limiting the functional change of falling back to interpreter\n> mode on \"JIT's broken anyway\" systems only should address these and get\n> me a functional 'git grep' again.\n\n*nod*\n\n>>>> So not only would a BUG() biting us here require them to create a new\n>>>> code for the state of \"we have the JIT, but can't use it here\" (for some\n>>>> reason I can't imagine, as \"PCRE2_ERROR_NOMEMORY\" is already\n>>>> \"overloaded\" to mean that).\n>>>>\n>>>> It would also require them to invent a new \"soft\" failure mode for the\n>>>> JIT, i.e. not the facility added in a25b9085043, where we can use the\n>>>> JIT, but it's not on after all due to a \"(*NO_JIT)\" in the pattern\n>>>> itself.\n>>>\n>>> We should really treat PCRE2 JIT as an *optional* optimization that\n>>> might not be available for certain cases. For these we should, IMHO,\n>>> simply use the interpreter mode, instead of bugging users with a BUG() /\n>>> die().\n>> \n>> To summarize some of the above, I think performance also matters, we\n>> have cases where:\n>> \n>> A. We could use the non-JIT\n>> B. We could use the JIT, and it's a *lot* faster\n>> C. We can't use the JIT at all\n>> D. We can't use the JIT because we run into its limits\n>> \n>> I think it's fair to die on \"D\" as in practice you only (I think!) run\n>> into it on pathological patterns, but yes, another option would be to\n>> fall back to \"A\".\n>> \n>> But thinking you're doing \"B\" and not wanting to implicitly fall back to\n>> \"A\" is also a valid use-case.\n>\n> Agreed. My above sketched handling should do that, as in not falling\n> back to interpreter mode when the JIT would be functional per se, but\n> just failed on this particular pattern.\n>\n>> So I'm inclined to suggest that we should be less helpful with automatic\n>> fallbacks, and just suggest a \"try it with '(*NO_JIT)'\" advice() or\n>> something.\n>\n> Well, that's a real pain from a user's (my) point of view. Trust me, I'm\n> suffering from this, otherwise I wouldn't have brought up the issue ;)\n\nThat's fair enough, falling back in the \"D\" case sounds good.\n\n>> But as noted above needing to always disable an apparently \"available\"\n>> JIT on some systems (SELinux) does throw a monkey wrench into that\n>> particular suggestion :(\n>\n> Yep.\n>\n>> So I'm not sure, I'm mainly trying to encourage you to think through the\n>> edge cases, and to summarize the full impact of the change in a re-roll.\n>\n> Yeah, I agree. The implied fallback to the interpreter, even for the\n> more obscure cases should have been mentioned in the changelog, but I\n> overlooked / ignored that case initially.\n>\n> My take from the discussion is to do a re-roll with something like this:\n>\n> if (p->pcre2_jit_on) {\n> jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n> if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {\n> /* JIT's non-functional because of SELinux / PaX */\n> p->pcre2_jit_on = 0;\n> return;\n> } else if (jitret) {\n> die(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\"\n> \"Try prefixing the pattern with '(*NO_JIT)'\\n\",\n> p->pattern, jitret);\n> }\n> ...\n> }\n>\n> ...with pcre2_jit_functional() being something like this:\n>\n> static int pcre2_jit_functional(void)\n> {\n> pcre2_code *code;\n> size_t off;\n> int err;\n>\n> code = pcre2_compile((PCRE2_SPTR)\".\", 1, 0, &err, &off, NULL);\n> if (!code)\n> return 0;\n>\n> err = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE);\n> pcre2_code_free(code);\n>\n> return err == 0;\n> }\n\nThis seems sensible as pseudocode, although please don't add another\npcre2_compile() entry point (as e.g. passing NULL here will bypass the\ncontext we carefully set up, and if we have a custom allocator...).\n\nInstead you could just re-enter the API itself via\ncompile_grep_patterns(), or perhaps lower via\ncompile_pcre2_pattern(). Then add some flag to \"struct grep_opt\" to\nindicate that we shouldn't die() or BUG() there, but just run a \"jit\ntest\".\n\nThis code also treats all failures of pcre2_jit_compile() the same, but\nhere we only want PCRE2_ERROR_NOMEMORY.\n\n\n\n\n"},{"id":"470706","messageId":"1b119c0a-a720-be19-1d26-6d581282fc92@grsecurity.net","threadId":"58967","inReplyTo":"230118.86lelzx2c4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] grep: fall back to interpreter mode if JIT fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-19T09:19:42Z","receivedAt":"2023-01-19T09:19:50Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 18.01.23 16:44, Ævar Arnfjörð Bjarmason wrote:\n>>> [snip]\n>>> But silently falling back kind of sucks, but unfortunately pcre2 doesn't\n>>> provide a way to say \"failed because of SELinux\" v.s. \"failed because\n>>> the pattern is crazy\", except that we could try to compile a known-good\n>>> pattern with the JIT, to disambiguate the two.\n>>\n>> Exactly, so what about something like this:\n>>\n>> If JIT is generally available, try to JIT the user supplied pattern:\n>> 1/ If it fails with PCRE2_ERROR_NOMEMORY, try to compile a know good\n>> pattern, e.g. \".\":\n>> 1a/ If this fails with PCRE2_ERROR_NOMEMORY as well, fall back to\n>> interpreter, as JIT is non-functional because of SELinux / PaX.\n>> 1b/ If not, it's a \"crazy\" pattern, suggest '(*NO_JIT)'.\n>> 2/ If it succeeds or fails with a different error, continue as of now,\n>> i.e. use JIT on success or die() on error, optionally suggesting\n>> '(*NO_JIT)'.\n>>\n>> That should handle the case you're concerned about and only fall back to\n>> interpreter mode if JIT won't be functional anyway. Admitted, this would\n>> also allow crazy patterns, but there's simply no way to detect these\n>> under such constraints.\n> \n> That sounds good, i.e. we could narrow the JIT falling back case to\n> these SELinux cases and the like, distinct from generic internal errors.\n> \n> Maybe it's too much paranoia, but it should work & get rid of the\n> ambiguity.\n\nNah, it's fine.\n\n>>> [snip]\n>>> See above, but maybe it's the least sucky thing (and definitely\n>>> simpler). I'm mainly checking that we're doing that we want here, and\n>>> that we're going into it with eyes open.\n>>>\n>>> That we're now discussing a topic entirely different from SELinux on a\n>>> thread where we're (according to the commit message) fixing pcre2 where\n>>> the JIT is \"unusable on such systems\" is my main concern here. \n>>\n>> Yeah, I overlooked that angle initially, but it's a valid concern.\n>> However, limiting the functional change of falling back to interpreter\n>> mode on \"JIT's broken anyway\" systems only should address these and get\n>> me a functional 'git grep' again.\n> \n> *nod*\n> \n\n>>>>> [snip]\n>>>\n>>> To summarize some of the above, I think performance also matters, we\n>>> have cases where:\n>>>\n>>> A. We could use the non-JIT\n>>> B. We could use the JIT, and it's a *lot* faster\n>>> C. We can't use the JIT at all\n>>> D. We can't use the JIT because we run into its limits\n>>>\n>>> I think it's fair to die on \"D\" as in practice you only (I think!) run\n>>> into it on pathological patterns, but yes, another option would be to\n>>> fall back to \"A\".\n>>>\n>>> But thinking you're doing \"B\" and not wanting to implicitly fall back to\n>>> \"A\" is also a valid use-case.\n>>\n>> Agreed. My above sketched handling should do that, as in not falling\n>> back to interpreter mode when the JIT would be functional per se, but\n>> just failed on this particular pattern.\n>>\n>>> So I'm inclined to suggest that we should be less helpful with automatic\n>>> fallbacks, and just suggest a \"try it with '(*NO_JIT)'\" advice() or\n>>> something.\n>>\n>> Well, that's a real pain from a user's (my) point of view. Trust me, I'm\n>> suffering from this, otherwise I wouldn't have brought up the issue ;)\n> \n> That's fair enough, falling back in the \"D\" case sounds good.\n> \n>>> But as noted above needing to always disable an apparently \"available\"\n>>> JIT on some systems (SELinux) does throw a monkey wrench into that\n>>> particular suggestion :(\n>>\n>> Yep.\n>>\n>>> So I'm not sure, I'm mainly trying to encourage you to think through the\n>>> edge cases, and to summarize the full impact of the change in a re-roll.\n>>\n>> Yeah, I agree. The implied fallback to the interpreter, even for the\n>> more obscure cases should have been mentioned in the changelog, but I\n>> overlooked / ignored that case initially.\n>>\n>> My take from the discussion is to do a re-roll with something like this:\n>>\n>> if (p->pcre2_jit_on) {\n>> jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n>> if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {\n>> /* JIT's non-functional because of SELinux / PaX */\n>> p->pcre2_jit_on = 0;\n>> return;\n>> } else if (jitret) {\n>> die(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\"\n>> \"Try prefixing the pattern with '(*NO_JIT)'\\n\",\n>> p->pattern, jitret);\n>> }\n>> ...\n>> }\n>>\n>> ...with pcre2_jit_functional() being something like this:\n>>\n>> static int pcre2_jit_functional(void)\n>> {\n>> pcre2_code *code;\n>> size_t off;\n>> int err;\n>>\n>> code = pcre2_compile((PCRE2_SPTR)\".\", 1, 0, &err, &off, NULL);\n>> if (!code)\n>> return 0;\n>>\n>> err = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE);\n>> pcre2_code_free(code);\n>>\n>> return err == 0;\n>> }\n> \n> This seems sensible as pseudocode, although please don't add another\n> pcre2_compile() entry point (as e.g. passing NULL here will bypass the\n> context we carefully set up, and if we have a custom allocator...).\n\nThat was intentional, actually. I specifically want to use the most\nbasic API to test for \"general JIT availability.\" That test should tell\nif PCRE2 JIT is working /in general/, not specifically how we make use\nof it in git, which might not. However, that would be a different error,\ni.e. not because PCRE2 failed to allocate JIT memory but us using the\nAPI wrong, hitting limitations, etc.\n\nMaking use of the PCRE2 context we set up in compile_pcre2_pattern()\n(and thereby possibly generating errors because of it) could mask API\nusage errors resulting from these and may lead to a false fallback to\ninterpreter mode when we want to die() instead?\n\n> Instead you could just re-enter the API itself via\n> compile_grep_patterns(), or perhaps lower via\n> compile_pcre2_pattern(). Then add some flag to \"struct grep_opt\" to\n> indicate that we shouldn't die() or BUG() there, but just run a \"jit\n> test\".\n\nThis feels kinda hacky and overkill, actually. A dedicated simplistic\ntest looks much cleaner, IMHO.\n\n> This code also treats all failures of pcre2_jit_compile() the same, but\n> here we only want PCRE2_ERROR_NOMEMORY.\n\nDoes it really matter? I mean, we could change the final test to 'err !=\nPCRE2_ERROR_NOMEMORY' but we also return early when we're unable to\ngenerate PCRE2 code for the \".\" pattern -- not strictly a JIT error as well.\n\nI'd say instead of splitting hairs, pcre2_jit_functional() should stay\nas above, as a failure to compile \".\" as a pattern can only happen (a)\nunder very tight memory constraints or (b) for JIT internal reasons,\nwhich would boil down to not being able to allocate the required\nresources to generate JIT code --- it's such a simple pattern it's not\nexpected to fail in any other way. Case (a) can be ignored, IMHO, as\nthat would lead to follow up errors soon anyway.\n\n\nThanks,\nMathias\n"},{"id":"471089","messageId":"20230127154952.485913-1-minipli@grsecurity.net","threadId":"58967","inReplyTo":"20221216121557.30714-1-minipli@grsecurity.net","subject":"[PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-27T15:49:52Z","receivedAt":"2023-01-27T15:50:00Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"Under Linux systems with SELinux's 'deny_execmem' or PaX's MPROTECT\nenabled, the allocation of PCRE2's JIT rwx memory may be prohibited,\nmaking pcre2_jit_compile() fail with PCRE2_ERROR_NOMEMORY (-48):\n\n [user@fedora git]$ git grep -c PCRE2_JIT\n grep.c:1\n\n [user@fedora git]$ # Enable SELinux's W^X policy\n [user@fedora git]$ sudo semanage boolean -m -1 deny_execmem\n\n [user@fedora git]$ # JIT memory allocation fails, breaking 'git grep'\n [user@fedora git]$ git grep -c PCRE2_JIT\n fatal: Couldn't JIT the PCRE2 pattern 'PCRE2_JIT', got '-48'\n\nInstead of failing hard in this case and making 'git grep' unusable on\nsuch systems, simply fall back to interpreter mode, leading to a much\nbetter user experience.\n\nAs having a functional PCRE2 JIT compiler is a legitimate use case for\nperformance reasons, we'll only do the fallback if the supposedly\navailable JIT is found to be non-functional by attempting to JIT compile\na very simple pattern. If this fails, JIT is deemed to be non-functional\nand we do the interpreter fallback. For all other cases, i.e. the simple\npattern can be compiled but the user provided cannot, we fail hard as we\ndo now as the reason for the failure must be the pattern itself.\n\nCc: Carlo Marcelo Arenas Belón \nSigned-off-by: Mathias Krause \n---\n\nThis patch is based on a previous attempt proposed by Carlo already 4\nyears ago[1]. However, it wasn't applied as there were still ongoing\ndiscussions about how to handle and possibly avoid the automatic\nfallback.\n\nA follow-up RFC had been posted half a year later[2], adding a config\noption and, after some more discussion, even command line switches[3].\nBut, after all, it was agreed on that this is far too much and Junio\nsuggested to simply revert back to the initial RFC and implement the\nautomatic fallback[4], basically merging it with a proper changelog[5].\nAs that never happened, I took up the work and tried to do just that.\n\nThis, again, lead to some discussion but, fortunately, less about the\nconfig knobs, but more about the implications of such a change. I tried\nto address Ævar's concerns about always falling back to the interpreter\nand limited it to the problematic case I want to get solved.\n\n1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com\n2. https://lore.kernel.org/git/20190728235427.41425-1-carenas@gmail.com/\n3. https://lore.kernel.org/git/20190729105955.44390-1-carenas@gmail.com/\n4. https://lore.kernel.org/git/xmqqh874vikk.fsf@gitster-ct.c.googlers.com/\n5. https://lore.kernel.org/git/xmqqef1zmkp5.fsf@gitster-ct.c.googlers.com/\n\nChanges in v2:\n\nThe current version takes a conservative approach by only implementing\nthe fallback to interpreter mode when the runtime test for basic JIT\nsupport fails as well, indicating the inability of PCRE2's memory\nallocator to acquire a W|X mappings for runtime code generation.\n\npcre2_jit_functional() very much intentional calls pcre2_compile()\nwithout making use of the context set up in compile_pcre2_pattern() to\nexclude any errors related to that -- a wrong combination of options\nmaking pcre2_jit_compile() fail for these reasons instead of the memory\nmapping error we try to detect. This ensures we're doing a dumb simple\nJIT compile test to probe its general runtime availability and not\nwrongly fall back to interpreter mode because the option combination\nwe're trying to make use of isn't supported by the JIT.\n\nI also changed the author to myself as the current state has little in\ncommon with what Carlo once proposed.\n---\n grep.c | 42 +++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 06eed694936c..59afc3f07fc9 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -262,6 +262,31 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n \tfree(pointer);\n }\n \n+static int pcre2_jit_functional(void)\n+{\n+\tstatic int jit_working = -1;\n+\tpcre2_code *code;\n+\tsize_t off;\n+\tint err;\n+\n+\tif (jit_working != -1)\n+\t\treturn jit_working;\n+\n+\t/*\n+\t * Try to JIT compile a simple pattern to probe if the JIT is\n+\t * working in general. It might fail for systems where creating\n+\t * memory mappings for runtime code generation is restricted.\n+\t */\n+\tcode = pcre2_compile((PCRE2_SPTR)\".\", 1, 0, &err, &off, NULL);\n+\tif (!code)\n+\t\treturn 0;\n+\n+\tjit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0;\n+\tpcre2_code_free(code);\n+\n+\treturn jit_working;\n+}\n+\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tint error;\n@@ -317,8 +342,23 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n \tif (p->pcre2_jit_on) {\n \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n-\t\tif (jitret)\n+\t\tif (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {\n+\t\t\t/*\n+\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n+\t\t\t * indicated JIT support, the library might still\n+\t\t\t * fail to generate JIT code for various reasons,\n+\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n+\t\t\t * MPROTECT prevent creating W|X memory mappings.\n+\t\t\t *\n+\t\t\t * Instead of faling hard, fall back to interpreter\n+\t\t\t * mode, just as if the pattern was prefixed with\n+\t\t\t * '(*NO_JIT)'.\n+\t\t\t */\n+\t\t\tp->pcre2_jit_on = 0;\n+\t\t\treturn;\n+\t\t} else if (jitret) {\n \t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n+\t\t}\n \n \t\t/*\n \t\t * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just\n-- \n2.39.0\n\n"},{"id":"471090","messageId":"xmqqbkmk9bsn.fsf@gitster.g","threadId":"58967","inReplyTo":"20230127154952.485913-1-minipli@grsecurity.net","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-27T16:34:16Z","receivedAt":"2023-01-27T16:34:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathias Krause writes:\n\n> As having a functional PCRE2 JIT compiler is a legitimate use case for\n> performance reasons, we'll only do the fallback if the supposedly\n> available JIT is found to be non-functional by attempting to JIT compile\n> a very simple pattern. If this fails, JIT is deemed to be non-functional\n> and we do the interpreter fallback. For all other cases, i.e. the simple\n> pattern can be compiled but the user provided cannot, we fail hard as we\n> do now as the reason for the failure must be the pattern itself.\n\nI do not know if it is a good idea to rely on the \"very simple\npattern\". The implementation of JIT could devise various ways to\nsucceed for such simple patterns without having writable-executable\npiece of memory. What happened to the earlier idea of falling back\nto the interpreted codepath, which will catch any bad pattern that\nhas \"the reason for the failure\" by failing anyway?\n\n> +static int pcre2_jit_functional(void)\n> +{\n> +\tstatic int jit_working = -1;\n> +\tpcre2_code *code;\n> +\tsize_t off;\n> +\tint err;\n> +\n> +\tif (jit_working != -1)\n> +\t\treturn jit_working;\n> +\n> +\t/*\n> +\t * Try to JIT compile a simple pattern to probe if the JIT is\n> +\t * working in general. It might fail for systems where creating\n> +\t * memory mappings for runtime code generation is restricted.\n> +\t */\n> +\tcode = pcre2_compile((PCRE2_SPTR)\".\", 1, 0, &err, &off, NULL);\n> +\tif (!code)\n> +\t\treturn 0;\n> +\n> +\tjit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0;\n> +\tpcre2_code_free(code);\n\nI'd prefer not having to worry about: Or it might not fail for such\nsystems, as the pattern is too simple and future versions of\npcre2_compile() could have special case code.\n\n> @@ -317,8 +342,23 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n> \tif (p->pcre2_jit_on) {\n> \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n> -\t\tif (jitret)\n> +\t\tif (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {\n> +\t\t\t/*\n> +\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n> +\t\t\t * indicated JIT support, the library might still\n> +\t\t\t * fail to generate JIT code for various reasons,\n> +\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n> +\t\t\t * MPROTECT prevent creating W|X memory mappings.\n> +\t\t\t *\n> +\t\t\t * Instead of faling hard, fall back to interpreter\n> +\t\t\t * mode, just as if the pattern was prefixed with\n> +\t\t\t * '(*NO_JIT)'.\n> +\t\t\t */\n> +\t\t\tp->pcre2_jit_on = 0;\n> +\t\t\treturn;\n\nYes, the \"instead of failing hard, fall back\" makes sense. Just\nthat I do not see why the runtime test is a good thing to have. In\nshort, we are not in the business of catching bugs in pcre2_jit\nimplementations, so if they say they cannot compile the pattern (I\nwould even say I doubt the point of checking the return code to\nensure it is NOMEMORY), it would be fine to let the interpreter\ncodepath to inspect the pattern and diagnose problems with it, or\ntake the slow match without JIT.\n\nWhat am I missing?\n"},{"id":"471094","messageId":"xmqq1qnfancf.fsf@gitster.g","threadId":"58967","inReplyTo":"xmqqbkmk9bsn.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-27T17:39:28Z","receivedAt":"2023-01-27T17:39:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Yes, the \"instead of failing hard, fall back\" makes sense. Just\n> that I do not see why the runtime test is a good thing to have. In\n> short, we are not in the business of catching bugs in pcre2_jit\n> implementations, so if they say they cannot compile the pattern (I\n> would even say I doubt the point of checking the return code to\n> ensure it is NOMEMORY), it would be fine to let the interpreter\n> codepath to inspect the pattern and diagnose problems with it, or\n> take the slow match without JIT.\n>\n> What am I missing?\n\nNote that I've seen and recently re-read the discussion that leads to\nhttps://lore.kernel.org/git/f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net/\n\nI suspect that this auto-probe is related to solving \"the user\nthinks JIT is in use but because of failing JIT the user's pattern\nis getting horrible performance\" somehow. But I do not think a hard\nfailure is a good approach to help users in such a situation.\n\nAfter such a failure, the user can prefix \"(*NO_JIT)\" to the pattern\nand retry, or give up the operation altogether and not get a useful\nresult, but wouldn't it be far more helpful to just fallback as if\n(*NO_JIT) was on from the beginning?\n\nAlso I notice that p->pcre2_jit_on is per \"struct grep_pat\", so it\nis not like \"once we see a pathological pattern, we turn off JIT\ncompletely for other patterns\", right? That is, if you have\n\n git grep -P -e \"$A\" -e \"$B\"\n\nand we fail to compile \"$A\" (for whatever reason), we could still\n(attempt to) compile \"$B\". Perhaps $A was too complex or was\nincompatible with JIT combined with other options, but $B may be\neasy enough to still be JITtable, in which case we would match with\nthe JITted version of $B with interpreted version of $A, instead of\nfailing, right?\n\nTHanks.\n"},{"id":"471095","messageId":"xmqqtu0b95oy.fsf@gitster.g","threadId":"58967","inReplyTo":"xmqq1qnfancf.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-27T18:46:05Z","receivedAt":"2023-01-27T18:46:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> What am I missing?\n>\n> Note that I've seen and recently re-read the discussion that leads to\n> https://lore.kernel.org/git/f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net/\n>\n> I suspect that this auto-probe is related to solving \"the user\n> thinks JIT is in use but because of failing JIT the user's pattern\n> is getting horrible performance\" somehow. But I do not think a hard\n> failure is a good approach to help users in such a situation.\n\nI guess what I am saying is that the previous one that has been\nqueued on 'seen' may be better. It should cover your original\n\"SELinux and other mechanisms can render JIT unusable because they\ndo not allow dynamic generation of code\" use case.\n\nThanks.\n\n"},{"id":"471123","messageId":"d9f368bb-ff22-5294-3974-e0017f512c1a@grsecurity.net","threadId":"58967","inReplyTo":"xmqqbkmk9bsn.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-29T12:28:38Z","receivedAt":"2023-01-29T12:28:46Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 27.01.23 17:34, Junio C Hamano wrote:\n> Mathias Krause writes:\n> \n>> As having a functional PCRE2 JIT compiler is a legitimate use case for\n>> performance reasons, we'll only do the fallback if the supposedly\n>> available JIT is found to be non-functional by attempting to JIT compile\n>> a very simple pattern. If this fails, JIT is deemed to be non-functional\n>> and we do the interpreter fallback. For all other cases, i.e. the simple\n>> pattern can be compiled but the user provided cannot, we fail hard as we\n>> do now as the reason for the failure must be the pattern itself.\n> \n> I do not know if it is a good idea to rely on the \"very simple\n> pattern\". The implementation of JIT could devise various ways to\n> succeed for such simple patterns without having writable-executable\n> piece of memory.\n\nWell, if PCRE2 JIT ever changes to optimize this case, we would be back\nto the error I'm seeing right now. But I doubt that PCRE2 will be doing\noptimizations like that. The current implementation does the JIT memory\nallocation test very early, even before looking at the pattern:\n\nhttps://github.com/PCRE2Project/pcre2/blob/pcre2-10.42/src/pcre2_jit_compile.c#L14450-L14465\n\nBut I can add a call to pcre2_pattern_info(PCRE2_INFO_JITSIZE) if you\nreally like me to, but IMHO it's not needed.\n\n> What happened to the earlier idea of falling back\n> to the interpreted codepath, which will catch any bad pattern that\n> has \"the reason for the failure\" by failing anyway?\n\nÆvar's concerns about always falling back to the interpreter mode made\nme change the patch like this. Basically what he's concerned about are\ntwo things:\n1/ \"Crazy patterns\" that fail the JIT but will work for the interpreter\ncan be a serve performance regression.\n2/ Always falling back to interpreter mode might mask JIT API usage\nerrors, we'd like to see.\n\nWhile 1/ could also be seen as a limitation of current 'git grep', I\nshare Ævar's extended runtime regression concerns. If, for example, some\nweb interface offers users to supply arbitrary grep patterns, abusing\nthe interpreter mode fallback will consume significant more CPU\nresources than it does right now (which simply fails with an error).\n\n> \n>> +static int pcre2_jit_functional(void)\n>> +{\n>> +\tstatic int jit_working = -1;\n>> +\tpcre2_code *code;\n>> +\tsize_t off;\n>> +\tint err;\n>> +\n>> +\tif (jit_working != -1)\n>> +\t\treturn jit_working;\n>> +\n>> +\t/*\n>> +\t * Try to JIT compile a simple pattern to probe if the JIT is\n>> +\t * working in general. It might fail for systems where creating\n>> +\t * memory mappings for runtime code generation is restricted.\n>> +\t */\n>> +\tcode = pcre2_compile((PCRE2_SPTR)\".\", 1, 0, &err, &off, NULL);\n>> +\tif (!code)\n>> +\t\treturn 0;\n>> +\n>> +\tjit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0;\n>> +\tpcre2_code_free(code);\n> \n> I'd prefer not having to worry about: Or it might not fail for such\n> systems, as the pattern is too simple and future versions of\n> pcre2_compile() could have special case code.\n\nSee above, it's unlikely to happen.\n\n> \n>> @@ -317,8 +342,23 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>> \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n>> \tif (p->pcre2_jit_on) {\n>> \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n>> -\t\tif (jitret)\n>> +\t\tif (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {\n>> +\t\t\t/*\n>> +\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n>> +\t\t\t * indicated JIT support, the library might still\n>> +\t\t\t * fail to generate JIT code for various reasons,\n>> +\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n>> +\t\t\t * MPROTECT prevent creating W|X memory mappings.\n>> +\t\t\t *\n>> +\t\t\t * Instead of faling hard, fall back to interpreter\n>> +\t\t\t * mode, just as if the pattern was prefixed with\n>> +\t\t\t * '(*NO_JIT)'.\n>> +\t\t\t */\n>> +\t\t\tp->pcre2_jit_on = 0;\n>> +\t\t\treturn;\n> \n> Yes, the \"instead of failing hard, fall back\" makes sense. Just\n> that I do not see why the runtime test is a good thing to have.\n\nIt prevents the fallback from being abused and introducing new\nregressions. So it's good to have.\n\n> In\n> short, we are not in the business of catching bugs in pcre2_jit\n> implementations, so if they say they cannot compile the pattern (I\n> would even say I doubt the point of checking the return code to\n> ensure it is NOMEMORY), it would be fine to let the interpreter\n> codepath to inspect the pattern and diagnose problems with it, or\n> take the slow match without JIT.\n\nYeah, unfortunately they're not gonna fix what's a bug, IMHO. They think\nit's a feature: https://github.com/PCRE2Project/pcre2/pull/157\n\nAnyhow, the error code is very well documented, see pcre2_jit_compile(3)\n\"\"\"\n [...] The function can also return PCRE2_ERROR_NOMEMORY if\n JIT is unable to allocate executable memory for the compiler,\n even if it was because of a system security restriction.\n\"\"\"\n\nAnd that's very much in line with what the test in pcre2_jit_compile()'s\ncurrent implementation does.\n\n> What am I missing?\n\nPlease have a look at Ævar's reasoning here:\nhttps://lore.kernel.org/git/221220.86bknxwy9t.gmgdl@evledraar.gmail.com/\n\nThanks,\nMathias\n"},{"id":"471124","messageId":"adb5a43a-5081-4f60-d1ea-2a6511f858c0@grsecurity.net","threadId":"58967","inReplyTo":"xmqq1qnfancf.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-29T13:36:35Z","receivedAt":"2023-01-29T13:36:41Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 27.01.23 18:39, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Yes, the \"instead of failing hard, fall back\" makes sense. Just\n>> that I do not see why the runtime test is a good thing to have. In\n>> short, we are not in the business of catching bugs in pcre2_jit\n>> implementations, so if they say they cannot compile the pattern (I\n>> would even say I doubt the point of checking the return code to\n>> ensure it is NOMEMORY), it would be fine to let the interpreter\n>> codepath to inspect the pattern and diagnose problems with it, or\n>> take the slow match without JIT.\n>>\n>> What am I missing?\n> \n> Note that I've seen and recently re-read the discussion that leads to\n> https://lore.kernel.org/git/f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net/\n\nAhh, so ignore my last advise in the previous Email. You already read it.\n\n> I suspect that this auto-probe is related to solving \"the user\n> thinks JIT is in use but because of failing JIT the user's pattern\n> is getting horrible performance\" somehow. But I do not think a hard\n> failure is a good approach to help users in such a situation.\n\nYes, it's exactly trying to detect this case and not cause a regression\nfor users expecting 'git grep' to make use of the JIT.\n\nSo, beside the W|X memory allocation error, other errors are likely only\nto point out limitations of the JIT compiler, e.g. the example I gave in\nhttps://lore.kernel.org/all/2b04b19a-a2bd-3dd5-6f21-ed0b0ad3e02f@grsecurity.net/,\nwhich is, admitted, a made up example that can easily be worked around\nby manually prefixing it with '(*NO_JIT)'. It would be a pain having to\ndo that for *every* pattern, but only doing it for the pathological\ncases should be fine. Otherwise more users would have run into it\nalready and complained about it, no?\n\n> After such a failure, the user can prefix \"(*NO_JIT)\" to the pattern\n> and retry, or give up the operation altogether and not get a useful\n> result, but wouldn't it be far more helpful to just fallback as if\n> (*NO_JIT) was on from the beginning?\n\nSure, if it would be required for *every*, i.e. \"normal\" patterns. But\nalways doing it even for abusive ones doesn't feel right.\n\n> Also I notice that p->pcre2_jit_on is per \"struct grep_pat\", so it\n> is not like \"once we see a pathological pattern, we turn off JIT\n> completely for other patterns\", right? That is, if you have\n> \n> git grep -P -e \"$A\" -e \"$B\"\n> \n> and we fail to compile \"$A\" (for whatever reason), we could still\n> (attempt to) compile \"$B\". Perhaps $A was too complex or was\n> incompatible with JIT combined with other options, but $B may be\n> easy enough to still be JITtable, in which case we would match with\n> the JITted version of $B with interpreted version of $A, instead of\n> failing, right?\n\nThe current version of git would fail hard if it fails to JIT compile\n\"$A\". My patch doesn't change that behavior and that's intentional, as I\nshare Ævar's thinking about falling back to the interpreter mode for\n\"complex patterns\" (which means pathological cases, really) is a bad\nidea. While we might be able to compile the pattern and run it in\ninterpreter mode, it'll likely have a *much* higher runtime.\n\nJust to get you a glimpse of how much longer, this is what it takes\nrunning the pathological pattern from above's example on git.git:\n\n $ time git grep -P \"(*NO_JIT)$(perl -e 'print \"(.)\" x 4000')\"\n Binary file git-gui/macosx/git-gui.icns matches\n Binary file t/t5000/pax.tar matches\n Binary file t/t5004/big-pack.zip matches\n Binary file t/t5004/empty-with-pax-header.tar matches\n\n real\t44m42,150s\n user\t577m14,623s\n sys\t0m46,210s\n\n\nSo this grep run eat up ~9.5 *hours* of CPU time. Do we really want to\nfall back to something like this for the pathological cases? ...Yeah, I\ndon't think so either.\n\nThanks,\nMathias\n"},{"id":"471125","messageId":"da549056-f6ab-aa71-a0fd-bf55ef6fd6b9@grsecurity.net","threadId":"58967","inReplyTo":"xmqqtu0b95oy.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-29T13:37:09Z","receivedAt":"2023-01-29T13:37:14Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 27.01.23 19:46, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>>> What am I missing?\n>>\n>> Note that I've seen and recently re-read the discussion that leads to\n>> https://lore.kernel.org/git/f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net/\n>>\n>> I suspect that this auto-probe is related to solving \"the user\n>> thinks JIT is in use but because of failing JIT the user's pattern\n>> is getting horrible performance\" somehow. But I do not think a hard\n>> failure is a good approach to help users in such a situation.\n> \n> I guess what I am saying is that the previous one that has been\n> queued on 'seen' may be better. It should cover your original\n> \"SELinux and other mechanisms can render JIT unusable because they\n> do not allow dynamic generation of code\" use case.\n\nIt clearly does cover my use case but it has a bad impact on the runtime\nof pathological patterns. But if you think that's not an issue, I'll\nupdate the changelog of v1 accordingly and resent it.\n\nThanks,\nMathias\n"},{"id":"471126","messageId":"xmqqk0156z55.fsf@gitster.g","threadId":"58967","inReplyTo":"adb5a43a-5081-4f60-d1ea-2a6511f858c0@grsecurity.net","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-29T17:15:02Z","receivedAt":"2023-01-29T17:15:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathias Krause writes:\n\n> ... While we might be able to compile the pattern and run it in\n> interpreter mode, it'll likely have a *much* higher runtime.\n> ...\n> So this grep run eat up ~9.5 *hours* of CPU time. Do we really want to\n> fall back to something like this for the pathological cases? ...Yeah, I\n> don't think so either.\n\nYou may not, but I do not agree with you at all. The code should\nnot outsmart the user in such a case.\n\nEven if the pattern the user came up with is impossible to grok for\na working JIT compiler, and it might be hard to grok for the\ninterpreter, what is the next step you recommend the user if you\nrefuse to fall back on the interprete? \"Rewrite it to please the\nJIT compiler\"?\n\nIf that is the best pattern the user can produce to solve the\nproblem at hand, being able to give the user an answer in 9 hours is\nmuch better than not being able to give anything at all. \n\nMaybe after waiting for 5 minutes, the user gets bored and ^C, or\nwithout killing it, open another terminal and try a different\npatern, and in 9 hours, perhaps comes up with an equivalent (or\ndifferent but close enough) pattern that happens to run much faster,\nat which time the user may kill the original one. In any of these\ncases, by refusing to run, the code is not doing any service to the\nuser.\n"},{"id":"471137","messageId":"230130.867cx4s2j4.gmgdl@evledraar.gmail.com","threadId":"58967","inReplyTo":"xmqqk0156z55.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-30T10:56:42Z","receivedAt":"2023-01-30T11:08:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jan 29 2023, Junio C Hamano wrote:\n\n> Mathias Krause writes:\n>\n>> ... While we might be able to compile the pattern and run it in\n>> interpreter mode, it'll likely have a *much* higher runtime.\n>> ...\n>> So this grep run eat up ~9.5 *hours* of CPU time. Do we really want to\n>> fall back to something like this for the pathological cases? ...Yeah, I\n>> don't think so either.\n>\n> You may not, but I do not agree with you at all. The code should\n> not outsmart the user in such a case.\n\nIt's the falling back in the nominal case that would be outsmarting the\nuser.\n\nIf I compile libpcre2 with JIT support I'm expecting Git to use that,\nand not fall back in those cases where the JIT engine would give up.\n\n> Even if the pattern the user came up with is impossible to grok for\n> a working JIT compiler, and it might be hard to grok for the\n> interpreter, what is the next step you recommend the user if you\n> refuse to fall back on the interprete? \"Rewrite it to please the\n> JIT compiler\"?\n\nI'd argue that it's pretty much impossible to unintentionally write such\npathological patterns, the edge cases where e.g. the JIT would run out\nof resources v.s. the normal engine are a non-issue for any \"normal\"\nuse.\n\nPathological regexes are pretty much only interesting to anyone in the\ncontext of DoS attacks where they're being used to cause intentional\nslowdowns.\n\nHere we're discussing an orthagonal case where the \"JIT fails\", but\nrather than some pathological pattern it's because SELinux has made it\nnot work at runtime, and we're trying to tease the two cases apart.\n\n> If that is the best pattern the user can produce to solve the\n> problem at hand, being able to give the user an answer in 9 hours is\n> much better than not being able to give anything at all.\n\nSpeed is a feature in itself, and in a lot of cases (e.g. user-supplied\npatterns vulnerable to a DoS attack) continuing on the slow path is much\nworse.\n\nEven just using my terminal for ad-hoc \"git grep\", I'd *much* rather get\nan early error about the pattern exceeding JIT resources than continuing\non the fallback path.\n\nIf I had somehow written one by accident (and this is stretching\ncredulity) you can usually apply some minor tweaks to the pattern, and\nthen execute it in seconds instead of minutes/hours.\n\n> Maybe after waiting for 5 minutes, the user gets bored and ^C, or\n> without killing it, open another terminal and try a different\n> patern, and in 9 hours, perhaps comes up with an equivalent (or\n> different but close enough) pattern that happens to run much faster,\n> at which time the user may kill the original one. In any of these\n> cases, by refusing to run, the code is not doing any service to the\n> user.\n\nI don't think this is plausible at all per the above, and that we\nshouldn't harm realistic use-cases to satisfy hypothetical ones.\n"},{"id":"471138","messageId":"9b5a1113-84f1-1651-bffc-6382462057dd@grsecurity.net","threadId":"58967","inReplyTo":"xmqqk0156z55.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-30T11:08:45Z","receivedAt":"2023-01-30T11:08:52Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 29.01.23 18:15, Junio C Hamano wrote:\n> Mathias Krause writes:\n> \n>> ... While we might be able to compile the pattern and run it in\n>> interpreter mode, it'll likely have a *much* higher runtime.\n>> ...\n>> So this grep run eat up ~9.5 *hours* of CPU time. Do we really want to\n>> fall back to something like this for the pathological cases? ...Yeah, I\n>> don't think so either.\n> \n> You may not, but I do not agree with you at all. The code should\n> not outsmart the user in such a case.\n\nIt doesn't. My rhetoric question was just missing \"automatically\" to\nstate that I would dislike an *automatic* fallback to the interpreter\nfor *pathological cases.* But I'm fine with (and that's what this patch\nis all about!) a fallback to the interpreter for patterns that simply\nfail the JIT because it's broken. Sorry for the confusion.\n\n> Even if the pattern the user came up with is impossible to grok for\n> a working JIT compiler, and it might be hard to grok for the\n> interpreter, what is the next step you recommend the user if you\n> refuse to fall back on the interprete? \"Rewrite it to please the\n> JIT compiler\"?\n\nNot at all. A user is still free to disable the JIT and enforce using\nthe interpreter by using the \"(*NO_JIT)\" prefix. My patch doesn't\ndisable this behavior. My patch only tries to avoid having to specify it\nfor \"regular\" patterns when the JIT is broken anyways.\n\nThe key here is that this would be a manual step (in contrast to an\nautomatic fallback), i.e. we require explicit user consent to accept the\nworse runtime performance. And, IMHO, that should be acceptable from a\nusability point of view as this would only be required for the\npathological cases an otherwise functional JIT simply cannot handle.\n\n> If that is the best pattern the user can produce to solve the\n> problem at hand, being able to give the user an answer in 9 hours is\n> much better than not being able to give anything at all.\n\nSure, fully agree.\n\n> Maybe after waiting for 5 minutes, the user gets bored and ^C, or\n> without killing it, open another terminal and try a different\n> patern, and in 9 hours, perhaps comes up with an equivalent (or\n> different but close enough) pattern that happens to run much faster,\n> at which time the user may kill the original one. In any of these\n> cases, by refusing to run, the code is not doing any service to the\n> user.\n\nMy patch doesn't make it worse than what 'git grep' would currently be\ndoing. On the contrary, actually. It allows me to use PaX's MPROTECT and\nhave a functional 'git grep' as well.\n\nMaybe the missing piece here is simply something like below to make\nusers more aware of the possibility to disable the JIT for the more\ncomplex cases?:\n\ndiff --git a/grep.c b/grep.c\nindex 59afc3f07fc9..1422f168b087 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -357,7 +357,8 @@ static void compile_pcre2_pattern(struct grep_pat\n*p, const struct grep_opt *opt\n p->pcre2_jit_on = 0;\n return;\n } else if (jitret) {\n- die(\"Couldn't JIT the PCRE2 pattern '%s', got\n'%d'\\n\", p->pattern, jitret);\n+ die(\"Couldn't JIT the PCRE2 pattern '%s', got\n'%d'%s\\n\", p->pattern, jitret,\n+ pcre2_jit_functional() ? \"\\nYou might retry\nby prefixing the pattern with '(*NO_JIT)'\" : \"\");\n }\n\n /*\n\n(Sorry about the wrapped lines, my mailer is just broken. I'll make it a\nproper patch, if such functionality is indeed wanted.)\n\nThanks,\nMathias\n"},{"id":"471145","messageId":"xmqq8rhj504i.fsf@gitster.g","threadId":"58967","inReplyTo":"230130.867cx4s2j4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-30T18:49:01Z","receivedAt":"2023-01-30T18:49:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> If I compile libpcre2 with JIT support I'm expecting Git to use that,\n> and not fall back in those cases where the JIT engine would give up.\n\nThe thing is, the reason why their Git has JIT enabled pcre2 for\nmany users is not because they choose to compile their own Git for\nthemselves because they wanted to play with JIT. To them, their\ndistro and/or their employer gave a precompiled Git, in the hope\nthat with JIT would be faster than without JIT when JIT is usable.\n\nIn that context, \"Speed is a feature in itself\" is correct but\n\"failing fast, forcing the user to try different things\" is not a\n\"Speed\" feature at all. It may be interesting only for those who\nare curious to see what pattern was rejected by JIT. It is\nespecially true as (1) we are willing to fall back to interpreter in\nthe SELinux senario, and (2) for normal users who want to use Git,\nand not necessarily interested in playing with JIT, there is no\nother recourse than prefixing \"I do not want this JITted\" to their\npattern ANYWAY. Why fail fast and force the user to take the only\nrecourse manually, when the machinery already knows what the user's\nonly viable alternative is (i.e. falling back to the interpreter)?\n\n> Pathological regexes are pretty much only interesting to anyone in the\n> context of DoS attacks where they're being used to cause intentional\n> slowdowns.\n\nExactly.\n\n> Here we're discussing an orthagonal case where the \"JIT fails\", but\n> rather than some pathological pattern it's because SELinux has made it\n> not work at runtime, and we're trying to tease the two cases apart.\n\ns/and we're/but you're/. And I do not think you want to.\n\n> I don't think this is plausible at all per the above, and that we\n> shouldn't harm realistic use-cases to satisfy hypothetical ones.\n\nTo me, what you are advocating is exactly the hypothetical ones that\nharm end-users who did not choose to enable JIT themselves. When JIT\nfails for whatever reason (including the SELinux senario) for them,\nthey do not need to be told by Git failing, when the interpreter can\ngive them the correct answer. Wanting to see the result of the\noperation they asked Git to do, while allowing Git to use clever\noptimizations WHEN ABLE, is what I see as realistic use-cases.\n\n"},{"id":"471146","messageId":"xmqq357r4zvk.fsf@gitster.g","threadId":"58967","inReplyTo":"9b5a1113-84f1-1651-bffc-6382462057dd@grsecurity.net","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-30T18:54:23Z","receivedAt":"2023-01-30T18:54:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathias Krause writes:\n\n> My patch doesn't make it worse than what 'git grep' would currently be\n> doing. On the contrary, actually. It allows me to use PaX's MPROTECT and\n> have a functional 'git grep' as well.\n\nI know. But then without the \"why did it fail?\" logic (i.e. the v1\npatch), it does not make it worse than the current code, either, and\nof course allows you to use JIT-enabled pcre2 even where JIT is\nimpossible due to MPROTECT and whatever reasson.\n\n> Maybe the missing piece here is simply something like below to make\n> users more aware of the possibility to disable the JIT for the more\n> complex cases?:\n\nIf we were to keep that \"die\", it is absolutely required, I would\nthink. Users who got their Git with JIT-enabled pcre2 may be\nviewing JIT merely as \"a clever optimization the implementation is\nallowed to use when able\", without knowing and more importantly\nwithout wanting to know how to disable it from within their\npatterns.\n\nBut can't we drop that die() if we took the v1 route?\n\n> diff --git a/grep.c b/grep.c\n> index 59afc3f07fc9..1422f168b087 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -357,7 +357,8 @@ static void compile_pcre2_pattern(struct grep_pat\n> *p, const struct grep_opt *opt\n> p->pcre2_jit_on = 0;\n> return;\n> } else if (jitret) {\n> - die(\"Couldn't JIT the PCRE2 pattern '%s', got\n> '%d'\\n\", p->pattern, jitret);\n> + die(\"Couldn't JIT the PCRE2 pattern '%s', got\n> '%d'%s\\n\", p->pattern, jitret,\n> + pcre2_jit_functional() ? \"\\nYou might retry\n> by prefixing the pattern with '(*NO_JIT)'\" : \"\");\n> }\n>\n> /*\n>\n> (Sorry about the wrapped lines, my mailer is just broken. I'll make it a\n> proper patch, if such functionality is indeed wanted.)\n>\n> Thanks,\n> Mathias\n"},{"id":"471152","messageId":"xmqqlelj3hvk.fsf@gitster.g","threadId":"58967","inReplyTo":"xmqq357r4zvk.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-30T20:08:31Z","receivedAt":"2023-01-30T20:08:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If we were to keep that \"die\", it is absolutely required, I would\n> think. Users who got their Git with JIT-enabled pcre2 may be\n> viewing JIT merely as \"a clever optimization the implementation is\n> allowed to use when able\", without knowing and more importantly\n> without wanting to know how to disable it from within their\n> patterns.\n>\n> But can't we drop that die() if we took the v1 route?\n\nHaving said all that, I do not mind queuing v2 if the \"use *NO_JIT\nto disable\" is added to the message to help users who are forced to\nredo the query.\n\nAnd in practice, it shouldn't make that much difference, because the\nonly scenario (other than the SELinux-like situation where JIT is\ncompiled in but does not work at all) that the difference may matter\nwould happen when a non-trivial portion of the patterns users use\nare not workable with JIT, but if that were the case, we would have\nwritten JIT off as not mature enough and not yet usable long time\nago. So, in practice, patterns refused by JIT would be a very tiny\nminority to matter in real life, and \"failing fast to inconvenience\nusers\" would not be too bad.\n\nSo while I still think v1's simplicity is the right thing to have\nhere, I think it is waste of our braincell to compare v1 vs v2. As\nv2 gives smaller incremental behaviour change perceived by end\nusers, if somebody really wanted to, I'd expect that a low-hanging\nfruit #leftoverbit on top of such a patch, after the dust settles,\nwould be to\n\n (1) rename pcre2_jit_functional() to fall_back_to_interpreter() or\n something,\n\n (2) add a configuration variable to tell fall_back_to_interpreter()\n that any form of JIT error is allowed to fall back to\n interpreter().\n\nand such a patch will essentially give back the simplicity of v1 to\nfolks who opt into the configuration.\n\nThanks.\n"},{"id":"471153","messageId":"xmqqk0131zxi.fsf@gitster.g","threadId":"58967","inReplyTo":"xmqqlelj3hvk.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-30T21:21:29Z","receivedAt":"2023-01-30T21:21:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Having said all that, I do not mind queuing v2 if the \"use *NO_JIT\n> to disable\" is added to the message to help users who are forced to\n> redo the query.\n\nIn the meantime, here is what I plan to apply on top of v2 while\nqueuing it. The message given to die() should lack the terminating\nLF, and the overlong line can and should be split at operator\nboundary.\n\nThanks.\n\n grep.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git c/grep.c w/grep.c\nindex 59afc3f07f..42f184bd09 100644\n--- c/grep.c\n+++ w/grep.c\n@@ -357,7 +357,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\tp->pcre2_jit_on = 0;\n \t\t\treturn;\n \t\t} else if (jitret) {\n-\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n+\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'%s\",\n+\t\t\t p->pattern, jitret,\n+\t\t\t pcre2_jit_functional() \n+\t\t\t ? \"\\nPerhaps prefix (*NO_GIT) to your pattern?\" \n+\t\t\t : \"\");\n \t\t}\n \n \t\t/*\n"},{"id":"471154","messageId":"55c75313-79d4-1c5b-951b-5d1e75553441@ramsayjones.plus.com","threadId":"58967","inReplyTo":"xmqqk0131zxi.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-01-30T22:30:12Z","receivedAt":"2023-01-30T22:30:33Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 30/01/2023 21:21, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Having said all that, I do not mind queuing v2 if the \"use *NO_JIT\n>> to disable\" is added to the message to help users who are forced to\n>> redo the query.\n> \n> In the meantime, here is what I plan to apply on top of v2 while\n> queuing it. The message given to die() should lack the terminating\n> LF, and the overlong line can and should be split at operator\n> boundary.\n> \n> Thanks.\n> \n> grep.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git c/grep.c w/grep.c\n> index 59afc3f07f..42f184bd09 100644\n> --- c/grep.c\n> +++ w/grep.c\n> @@ -357,7 +357,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \t\t\tp->pcre2_jit_on = 0;\n> \t\t\treturn;\n> \t\t} else if (jitret) {\n> -\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n> +\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'%s\",\n> +\t\t\t p->pattern, jitret,\n> +\t\t\t pcre2_jit_functional() \n> +\t\t\t ? \"\\nPerhaps prefix (*NO_GIT) to your pattern?\" \n\ns/NO_GIT/NO_JIT/ ? :)\n\nATB,\nRamsay Jones\n\n> +\t\t\t : \"\");\n> \t\t}\n> \n> \t\t/*\n"},{"id":"471157","messageId":"xmqq5ycn1u3j.fsf@gitster.g","threadId":"58967","inReplyTo":"55c75313-79d4-1c5b-951b-5d1e75553441@ramsayjones.plus.com","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-30T23:27:28Z","receivedAt":"2023-01-30T23:27:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n>> +\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'%s\",\n>> +\t\t\t p->pattern, jitret,\n>> +\t\t\t pcre2_jit_functional() \n>> +\t\t\t ? \"\\nPerhaps prefix (*NO_GIT) to your pattern?\" \n>\n> s/NO_GIT/NO_JIT/ ? :)\n\nIndeed. Thanks.\n"},{"id":"471159","messageId":"580c2754-5bc3-760e-c85b-62b44baf7e13@grsecurity.net","threadId":"58967","inReplyTo":"xmqqlelj3hvk.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-31T07:30:12Z","receivedAt":"2023-01-31T07:30:18Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 30.01.23 21:08, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> If we were to keep that \"die\", it is absolutely required, I would\n>> think. Users who got their Git with JIT-enabled pcre2 may be\n>> viewing JIT merely as \"a clever optimization the implementation is\n>> allowed to use when able\", without knowing and more importantly\n>> without wanting to know how to disable it from within their\n>> patterns.\n>>\n>> But can't we drop that die() if we took the v1 route?\n> \n> Having said all that, I do not mind queuing v2 if the \"use *NO_JIT\n> to disable\" is added to the message to help users who are forced to\n> redo the query.\n> \n> And in practice, it shouldn't make that much difference, because the\n> only scenario (other than the SELinux-like situation where JIT is\n> compiled in but does not work at all) that the difference may matter\n> would happen when a non-trivial portion of the patterns users use\n> are not workable with JIT, but if that were the case, we would have\n> written JIT off as not mature enough and not yet usable long time\n> ago. So, in practice, patterns refused by JIT would be a very tiny\n> minority to matter in real life, and \"failing fast to inconvenience\n> users\" would not be too bad.\n\nExactly!\n\n> So while I still think v1's simplicity is the right thing to have\n> here, I think it is waste of our braincell to compare v1 vs v2. As\n> v2 gives smaller incremental behaviour change perceived by end\n> users, if somebody really wanted to, I'd expect that a low-hanging\n> fruit #leftoverbit on top of such a patch, after the dust settles,\n> would be to\n> \n> (1) rename pcre2_jit_functional() to fall_back_to_interpreter() or\n> something,\n> \n> (2) add a configuration variable to tell fall_back_to_interpreter()\n> that any form of JIT error is allowed to fall back to\n> interpreter().\n> \n> and such a patch will essentially give back the simplicity of v1 to\n> folks who opt into the configuration.\n\nFair enough. But aside from the W|X memory allocation denial exception\nis the likelihood to run into the limitations of PCRE2's JIT requiring\nthe interpreter fallback so little (as otherwise we'd see it in the past\nalready), I think, the demand for such a knob is basically nonexistent.\n\nThanks,\nMathias\n"},{"id":"471160","messageId":"d1c4cbad-bbb8-d610-5e27-970d96dd7a74@grsecurity.net","threadId":"58967","inReplyTo":"xmqqk0131zxi.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-31T07:48:54Z","receivedAt":"2023-01-31T07:49:38Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 30.01.23 22:21, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Having said all that, I do not mind queuing v2 if the \"use *NO_JIT\n>> to disable\" is added to the message to help users who are forced to\n>> redo the query.\n> \n> In the meantime, here is what I plan to apply on top of v2 while\n> queuing it. The message given to die() should lack the terminating\n> LF, and the overlong line can and should be split at operator\n> boundary.\n> \n> Thanks.\n> \n> grep.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git c/grep.c w/grep.c\n> index 59afc3f07f..42f184bd09 100644\n> --- c/grep.c\n> +++ w/grep.c\n> @@ -357,7 +357,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \t\t\tp->pcre2_jit_on = 0;\n> \t\t\treturn;\n> \t\t} else if (jitret) {\n> -\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n> +\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'%s\",\n> +\t\t\t p->pattern, jitret,\n> +\t\t\t pcre2_jit_functional() \n> +\t\t\t ? \"\\nPerhaps prefix (*NO_GIT) to your pattern?\" \n> +\t\t\t : \"\");\n> \t\t}\n> \n> \t\t/*\n\nLooks sensible, but maybe something like below would be even better?\n\ndiff --git a/grep.c b/grep.c\nindex 59afc3f07fc9..e0144ba77e7a 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -357,7 +357,13 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\tp->pcre2_jit_on = 0;\n \t\t\treturn;\n \t\t} else if (jitret) {\n-\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n+\t\t\tint do_clip = p->patternlen > 64;\n+\t\t\tint clip_len = do_clip ? 64 : p->patternlen;\n+\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%.*s'%s, got '%d'%s\",\n+\t\t\t clip_len, p->pattern, do_clip ? \"...\" : \"\", jitret,\n+\t\t\t pcre2_jit_functional()\n+\t\t\t ? \"\\nPerhaps prefix (*NO_JIT) to your pattern?\"\n+\t\t\t : \"\");\n \t\t}\n \n \t\t/*\n\nIt'll ensure, git will be printing the hint even for very long patterns,\nlike the one I was testing this with (\"$(perl -e 'print \"(.)\" x 4000')\").\n\nThanks,\nMathias\n"},{"id":"471163","messageId":"230131.86y1pjqedk.gmgdl@evledraar.gmail.com","threadId":"58967","inReplyTo":"xmqq8rhj504i.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-31T08:34:34Z","receivedAt":"2023-01-31T08:53:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 30 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> If I compile libpcre2 with JIT support I'm expecting Git to use that,\n>> and not fall back in those cases where the JIT engine would give up.\n>\n> The thing is, the reason why their Git has JIT enabled pcre2 for\n> many users is not because they choose to compile their own Git for\n> themselves because they wanted to play with JIT. To them, their\n> distro and/or their employer gave a precompiled Git, in the hope\n> that with JIT would be faster than without JIT when JIT is usable.\n>\n> In that context, \"Speed is a feature in itself\" is correct but\n> \"failing fast, forcing the user to try different things\" is not a\n> \"Speed\" feature at all. It may be interesting only for those who\n> are curious to see what pattern was rejected by JIT. It is\n> especially true as (1) we are willing to fall back to interpreter in\n> the SELinux senario, and (2) for normal users who want to use Git,\n> and not necessarily interested in playing with JIT, there is no\n> other recourse than prefixing \"I do not want this JITted\" to their\n> pattern ANYWAY. Why fail fast and force the user to take the only\n> recourse manually, when the machinery already knows what the user's\n> only viable alternative is (i.e. falling back to the interpreter)?\n\nBecause we have an issue with (1), but not (2). How would (2) happen? So\nfar I've only seen intentionally pathological patterns designed to\ntrigger the JIT's limits. I don't think it's worth DWYM-ing that path,\nwhen we're having to assume a lot about the \"M\" part of that.\n\n>> Pathological regexes are pretty much only interesting to anyone in the\n>> context of DoS attacks where they're being used to cause intentional\n>> slowdowns.\n>\n> Exactly.\n>\n>> Here we're discussing an orthagonal case where the \"JIT fails\", but\n>> rather than some pathological pattern it's because SELinux has made it\n>> not work at runtime, and we're trying to tease the two cases apart.\n>\n> s/and we're/but you're/. And I do not think you want to.\n\nThat s/// is fair, but brings me back to my question above of why we're\ntrying to solve (2) here.\n\n>> I don't think this is plausible at all per the above, and that we\n>> shouldn't harm realistic use-cases to satisfy hypothetical ones.\n>\n> To me, what you are advocating is exactly the hypothetical ones that\n> harm end-users who did not choose to enable JIT themselves. When JIT\n> fails for whatever reason (including the SELinux senario) for them,\n> they do not need to be told by Git failing, when the interpreter can\n> give them the correct answer. Wanting to see the result of the\n> operation they asked Git to do, while allowing Git to use clever\n> optimizations WHEN ABLE, is what I see as realistic use-cases.\n\nI'm saying that the \"JIT fails for whatever reason\" is\nhypothetical. It'll fail because of:\n\n - The (1) case, where we're categorically unable to run the JIT. Then\n we should proceed as if the JIT isn't available (as we do when it's\n e.g. not compiled into PCRE).\n\n - The pattern is pathological enough that it's about to take eons to\n execute it (2).\n\n The lack of bug reports about \"hey, my existing 'git grep' pattern\n failed\" when the JIT was shipped with v2.14.0 shows that this doesn't\n happen in practice.\n\n - The case where the API is returning some new error code that's\n unknown to us, let's call that (3).\n\n"},{"id":"471190","messageId":"xmqqtu06zmg2.fsf@gitster.g","threadId":"58967","inReplyTo":"d1c4cbad-bbb8-d610-5e27-970d96dd7a74@grsecurity.net","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-31T16:41:01Z","receivedAt":"2023-01-31T16:41:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathias Krause writes:\n\n> Looks sensible, but maybe something like below would be even better?\n\nWhen I say \"in the meantime\", I expect it not to be the final one.\nThis time, it was meant as a mere reminder to me while I wait for\nthe (hopefully final) reroll.\n\nThanks.\n"},{"id":"471194","messageId":"fb4c20a8-1bab-5905-5d56-90efad135a97@grsecurity.net","threadId":"58967","inReplyTo":"xmqqtu06zmg2.fsf@gitster.g","subject":"Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-31T18:34:14Z","receivedAt":"2023-01-31T18:35:04Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 31.01.23 17:41, Junio C Hamano wrote:\n> Mathias Krause writes:\n> \n>> Looks sensible, but maybe something like below would be even better?\n> \n> When I say \"in the meantime\", I expect it not to be the final one.\n> This time, it was meant as a mere reminder to me while I wait for\n> the (hopefully final) reroll.\n\nGot it, will send v3 (the final one, as three times is the charm, they\nsay) integrating the change to die() in a moment.\n\nThanks,\nMathias\n"},{"id":"471195","messageId":"20230131185611.520311-1-minipli@grsecurity.net","threadId":"58967","inReplyTo":"20230127154952.485913-1-minipli@grsecurity.net","subject":"[PATCH v3] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2023-01-31T18:56:11Z","receivedAt":"2023-01-31T18:56:34Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"Under Linux systems with SELinux's 'deny_execmem' or PaX's MPROTECT\nenabled, the allocation of PCRE2's JIT rwx memory may be prohibited,\nmaking pcre2_jit_compile() fail with PCRE2_ERROR_NOMEMORY (-48):\n\n [user@fedora git]$ git grep -c PCRE2_JIT\n grep.c:1\n\n [user@fedora git]$ # Enable SELinux's W^X policy\n [user@fedora git]$ sudo semanage boolean -m -1 deny_execmem\n\n [user@fedora git]$ # JIT memory allocation fails, breaking 'git grep'\n [user@fedora git]$ git grep -c PCRE2_JIT\n fatal: Couldn't JIT the PCRE2 pattern 'PCRE2_JIT', got '-48'\n\nInstead of failing hard in this case and making 'git grep' unusable on\nsuch systems, simply fall back to interpreter mode, leading to a much\nbetter user experience.\n\nAs having a functional PCRE2 JIT compiler is a legitimate use case for\nperformance reasons, we'll only do the fallback if the supposedly\navailable JIT is found to be non-functional by attempting to JIT compile\na very simple pattern. If this fails, JIT is deemed to be non-functional\nand we do the interpreter fallback. For all other cases, i.e. the simple\npattern can be compiled but the user provided cannot, we fail hard as we\ndo now as the reason for the failure must be the pattern itself. To aid\nusers in helping themselves change the error message to include a hint\nabout the '(*NO_JIT)' prefix. Also clip the pattern at 64 characters to\nensure the hint will be seen by the user and not internally truncated by\nthe die() function.\n\nCc: Carlo Marcelo Arenas Belón \nSigned-off-by: Mathias Krause \n---\nv2: https://lore.kernel.org/git/20230127154952.485913-1-minipli@grsecurity.net/\n\nChanges in v3:\n\nMention the possibility to prefix a failing pattern with '(*NO_JIT)' in\ncase we run into the JIT's limitations, as per Junio. Also clip the\nprinted pattern to ensure the hint actually gets printed.\n---\n grep.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 06eed694936c..c6fd44e3ec94 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -262,6 +262,31 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n \tfree(pointer);\n }\n \n+static int pcre2_jit_functional(void)\n+{\n+\tstatic int jit_working = -1;\n+\tpcre2_code *code;\n+\tsize_t off;\n+\tint err;\n+\n+\tif (jit_working != -1)\n+\t\treturn jit_working;\n+\n+\t/*\n+\t * Try to JIT compile a simple pattern to probe if the JIT is\n+\t * working in general. It might fail for systems where creating\n+\t * memory mappings for runtime code generation is restricted.\n+\t */\n+\tcode = pcre2_compile((PCRE2_SPTR)\".\", 1, 0, &err, &off, NULL);\n+\tif (!code)\n+\t\treturn 0;\n+\n+\tjit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0;\n+\tpcre2_code_free(code);\n+\n+\treturn jit_working;\n+}\n+\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tint error;\n@@ -317,8 +342,29 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n \tif (p->pcre2_jit_on) {\n \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n-\t\tif (jitret)\n-\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%s', got '%d'\\n\", p->pattern, jitret);\n+\t\tif (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {\n+\t\t\t/*\n+\t\t\t * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)\n+\t\t\t * indicated JIT support, the library might still\n+\t\t\t * fail to generate JIT code for various reasons,\n+\t\t\t * e.g. when SELinux's 'deny_execmem' or PaX's\n+\t\t\t * MPROTECT prevent creating W|X memory mappings.\n+\t\t\t *\n+\t\t\t * Instead of faling hard, fall back to interpreter\n+\t\t\t * mode, just as if the pattern was prefixed with\n+\t\t\t * '(*NO_JIT)'.\n+\t\t\t */\n+\t\t\tp->pcre2_jit_on = 0;\n+\t\t\treturn;\n+\t\t} else if (jitret) {\n+\t\t\tint need_clip = p->patternlen > 64;\n+\t\t\tint clip_len = need_clip ? 64 : p->patternlen;\n+\t\t\tdie(\"Couldn't JIT the PCRE2 pattern '%.*s'%s, got '%d'%s\",\n+\t\t\t clip_len, p->pattern, need_clip ? \"...\" : \"\", jitret,\n+\t\t\t pcre2_jit_functional()\n+\t\t\t ? \"\\nPerhaps prefix (*NO_JIT) to your pattern?\"\n+\t\t\t : \"\");\n+\t\t}\n \n \t\t/*\n \t\t * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just\n-- \n2.39.1\n\n"},{"id":"471204","messageId":"xmqqedraxvna.fsf@gitster.g","threadId":"58967","inReplyTo":"20230131185611.520311-1-minipli@grsecurity.net","subject":"Re: [PATCH v3] grep: fall back to interpreter if JIT memory allocation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-31T21:05:13Z","receivedAt":"2023-01-31T21:05:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathias Krause writes:\n\n> Mention the possibility to prefix a failing pattern with '(*NO_JIT)' in\n> case we run into the JIT's limitations, as per Junio. Also clip the\n> printed pattern to ensure the hint actually gets printed.\n\nLooking good. Will queue. Thanks.\n"}]}