{"thread":{"id":"49989","subject":"[RFC PATCH 0/2] fallback to interpreter if JIT fails with pcre","startedAt":"2018-12-09T23:01:52Z","lastAt":"2018-12-11T20:51:19Z","messageCount":13,"participants":["Carlo Marcelo Arenas Belón","Ævar Arnfjörð Bjarmason","brian m. carlson","Carlo Arenas","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"364889","messageId":"20181209230024.43444-1-carenas@gmail.com","threadId":"49989","inReplyTo":null,"subject":"[RFC PATCH 0/2] fallback to interpreter if JIT fails with pcre","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-12-09T23:00:22Z","receivedAt":"2018-12-09T23:01:52Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"while testing in NetBSD 8, was surprised to find that most test cases\nusing PCRE2 where failing with some cryptic error from git :\n\n fatal: Couldn't JIT the PCRE2 pattern '$PATTERN', got '-48'\n\ninterestingly enough, using a JIT enabled PCRE1 library (not the default)\nwill show a similar error but a different error code.\n\nthe underlying problem is the same though; NetBSD includes PAX support\nwhich restricts the use of memory that is both writeable and executable\nand that prevents the JIT to create a compiled expression to jump into,\nand while the \"fix\" for NetBSD is simple it would seem the user experience\ncould be improved if instead of aborting, git will instead return the matches\nusing the slower interpreter (which seem to be also the recomendation from\nthe library developers)\n\nit is important to note that the problem is not unique to NetBSD and had\nreproduced it in OpenBSD where working around the issue is more complicated\nas WˆX exceptions require a filesystem mount option, and I can see it being\nproblematic with linux (as shown by the open bug[1] and the development of\nan alternative allocator to workaround the issue with seLinux) and with\nmacOS (specially versions older than 10.14) where there are restrictions to\nthe number of maps allowed with those flags.\n\nI am also curious if expanding NO_LIBPCRE1_JIT as an option to disable JIT\nwith PCRE2 (with a different name) might be worth pursuing? as well as some\nways to narrow the failures that will trigger the fallback, but the later\nis likely to need library changes which might not be possible with the old\nversion anyway.\n\n[1] https://bugs.exim.org/show_bug.cgi?id=1749\n\nCarlo Marcelo Arenas Belón (2):\n grep: fallback to interpreter if JIT fails with pcre1\n grep: fallback to interpreter if JIT fails with pcre2\n\n Makefile | 12 ++++++------\n grep.c | 13 +++++++++++--\n 2 files changed, 17 insertions(+), 8 deletions(-)\n\n-- \n2.20.0\n\n"},{"id":"364890","messageId":"20181209230024.43444-2-carenas@gmail.com","threadId":"49989","inReplyTo":"20181209230024.43444-1-carenas@gmail.com","subject":"[RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-12-09T23:00:23Z","receivedAt":"2018-12-09T23:01:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"JIT support was added to 8.20 but the interface we rely on is only\nenabled after 8.32 so try to make the message clearer.\n\nin systems where there are restrictions against creating executable\npages programatically (like OpenBSD, NetBSD, macOS or seLinux) JIT\nwill fail, resulting in a error message to the user.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 12 ++++++------\n grep.c | 6 ++++++\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..62b0cb6ee6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -32,14 +32,14 @@ all::\n # USE_LIBPCRE is a synonym for USE_LIBPCRE2, define USE_LIBPCRE1\n # instead if you'd like to use the legacy version 1 of the PCRE\n # library. Support for version 1 will likely be removed in some future\n-# release of Git, as upstream has all but abandoned it.\n+# release of Git, as upstream is focusing all development for new\n+# features in the newer version instead.\n #\n # When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if the PCRE v1\n-# library is compiled without --enable-jit. We will auto-detect\n-# whether the version of the PCRE v1 library in use has JIT support at\n-# all, but we unfortunately can't auto-detect whether JIT support\n-# hasn't been compiled in in an otherwise JIT-supporting version. If\n-# you have link-time errors about a missing `pcre_jit_exec` define\n+# library is newer than 8.32 but compiled without --enable-jit or\n+# you want to disable JIT\n+#\n+# If you have link-time errors about a missing `pcre_jit_exec` define\n # this, or recompile PCRE v1 with --enable-jit.\n #\n # Define LIBPCREDIR=/foo/bar if your PCRE header and library files are\ndiff --git a/grep.c b/grep.c\nindex 4db1510d16..5ccc0421a1 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -405,6 +405,12 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \t\tdie(\"%s\", error);\n \n #ifdef GIT_PCRE1_USE_JIT\n+\tif (p->pcre1_extra_info &&\n+\t\t!(p->pcre1_extra_info->flags & PCRE_EXTRA_EXECUTABLE_JIT)) {\n+\t\t/* JIT failed so fallback to the interpreter */\n+\t\tp->pcre1_jit_on = 0;\n+\t\treturn;\n+\t}\n \tpcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);\n \tif (p->pcre1_jit_on == 1) {\n \t\tp->pcre1_jit_stack = pcre_jit_stack_alloc(1, 1024 * 1024);\n-- \n2.20.0\n\n"},{"id":"364891","messageId":"20181209230024.43444-3-carenas@gmail.com","threadId":"49989","inReplyTo":"20181209230024.43444-1-carenas@gmail.com","subject":"[RFC PATCH 2/2] grep: fallback to interpreter if JIT fails with pcre2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-12-09T23:00:24Z","receivedAt":"2018-12-09T23:01:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"starting with 10.23, and as a side effect of the work for bug1749[1] (grep\n-P crash with seLinux), pcre2grep was modified to ignore any errors from\npcre2_jit_compile so the interpreter could be used as a fallback\n\n[1] https://bugs.exim.org/show_bug.cgi?id=1749\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n grep.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 5ccc0421a1..c751c8cc74 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -530,8 +530,11 @@ 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 == 1) {\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/* JIT failed so fallback to the interpreter */\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.20.0\n\n"},{"id":"364892","messageId":"87r2eqxnru.fsf@evledraar.gmail.com","threadId":"49989","inReplyTo":"20181209230024.43444-2-carenas@gmail.com","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-09T23:51:01Z","receivedAt":"2018-12-09T23:51:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Dec 09 2018, Carlo Marcelo Arenas Belón wrote:\n\n[+CC pcre-dev]\n\n> JIT support was added to 8.20 but the interface we rely on is only\n> enabled after 8.32 so try to make the message clearer.\n>\n> in systems where there are restrictions against creating executable\n> pages programatically (like OpenBSD, NetBSD, macOS or seLinux) JIT\n> will fail, resulting in a error message to the user.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> Makefile | 12 ++++++------\n> grep.c | 6 ++++++\n> 2 files changed, 12 insertions(+), 6 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 1a44c811aa..62b0cb6ee6 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -32,14 +32,14 @@ all::\n> # USE_LIBPCRE is a synonym for USE_LIBPCRE2, define USE_LIBPCRE1\n> # instead if you'd like to use the legacy version 1 of the PCRE\n> # library. Support for version 1 will likely be removed in some future\n> -# release of Git, as upstream has all but abandoned it.\n> +# release of Git, as upstream is focusing all development for new\n> +# features in the newer version instead.\n\nI think whatever we do here it makes sense to split this into its own\npatch, since it doesn't have to do with this fallback mechanism.\n\nFWIW I was trying to word this in some way that very briefly described\nthe v1 v.s. v2 situation. Just saying \"new features\" doesn't quite\ncapture it, e.g. some bugs in v1 are closed with some resolution like\n\"this isn't trivial to fix, use v2 instead\".\n\n> # When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if the PCRE v1\n> -# library is compiled without --enable-jit. We will auto-detect\n> -# whether the version of the PCRE v1 library in use has JIT support at\n> -# all, but we unfortunately can't auto-detect whether JIT support\n> -# hasn't been compiled in in an otherwise JIT-supporting version. If\n> -# you have link-time errors about a missing `pcre_jit_exec` define\n> +# library is newer than 8.32 but compiled without --enable-jit or\n> +# you want to disable JIT\n> +#\n> +# If you have link-time errors about a missing `pcre_jit_exec` define\n> # this, or recompile PCRE v1 with --enable-jit.\n> #\n> # Define LIBPCREDIR=/foo/bar if your PCRE header and library files are\n> diff --git a/grep.c b/grep.c\n> index 4db1510d16..5ccc0421a1 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -405,6 +405,12 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n> \t\tdie(\"%s\", error);\n>\n> #ifdef GIT_PCRE1_USE_JIT\n> +\tif (p->pcre1_extra_info &&\n> +\t\t!(p->pcre1_extra_info->flags & PCRE_EXTRA_EXECUTABLE_JIT)) {\n> +\t\t/* JIT failed so fallback to the interpreter */\n> +\t\tp->pcre1_jit_on = 0;\n> +\t\treturn;\n> +\t}\n\nObviously this & what you have in 2/2 needs to be fixed in some way.\n\nIs the issue on SELinux, OpenBSD, NetBSD etc. *how* PCRE is creating the\nthe JIT'd code? I.e. presumably Google Chrome's JIT engine, Java JIT and\nthe like work on those setup, or not? I.e. is this something upstream\ncan/is likely to fix eventually?\n\nAre there cases where we can JIT, but fail for some entirely unrelated\nreason, and are now hiding the error?\n\nAre we mixing a condition where one some OS's or OS versions this just\nwon't work at all, and thus maybe should be something turned on in\nconfig.mak.uname, v.s. e.g. SELinux where presumably it'll dynamically\nchange.\n\nI'm inclined to suggest that we should have another ifdef here for \"if\nJIT fails I'd like it to die\", so that e.g. packages I build (for\ninternal use) don't silently slow down in the future, only for me to\nfind some months later that someone enabled an overzealous SELinux\npolicy and we swept this under the rug.\n\nBut maybe that's just dumb for some reason and we always need to do this\ndynamically...\n"},{"id":"364893","messageId":"20181210004252.GK890086@genre.crustytoothpaste.net","threadId":"49989","inReplyTo":"87r2eqxnru.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-10T00:42:52Z","receivedAt":"2018-12-10T00:44:05Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Dec 10, 2018 at 12:51:01AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Obviously this & what you have in 2/2 needs to be fixed in some way.\n> \n> Is the issue on SELinux, OpenBSD, NetBSD etc. *how* PCRE is creating the\n> the JIT'd code? I.e. presumably Google Chrome's JIT engine, Java JIT and\n> the like work on those setup, or not? I.e. is this something upstream\n> can/is likely to fix eventually?\n\nFrom the cover letter (but without testing), it seems like it would\nprobably be fine to first map the pages read-write to write the code and\nthen, once that's done, to map them read-executable. I know JIT\ncompilation does work on the BSDs, so presumably that's the technique to\nmake it do so.\n\nBoth versions of PCRE map pages both write and executable at the same\ntime, which is presumably where things go wrong. I assume it can be\nfixed, but whether that's easy in the context of PCRE, I wouldn't know.\n\n> Are we mixing a condition where one some OS's or OS versions this just\n> won't work at all, and thus maybe should be something turned on in\n> config.mak.uname, v.s. e.g. SELinux where presumably it'll dynamically\n> change.\n\nConsidering that some Linux users use PaX kernels with standard\ndistributions and that most BSD kernels can be custom-compiled with a\nvariety of options enabled or disabled, I think this is something we\nshould detect dynamically.\n\n> I'm inclined to suggest that we should have another ifdef here for \"if\n> JIT fails I'd like it to die\", so that e.g. packages I build (for\n> internal use) don't silently slow down in the future, only for me to\n> find some months later that someone enabled an overzealous SELinux\n> policy and we swept this under the rug.\n\nMy view is that JIT is a nice performance optimization, but it's\noptional. I honestly don't think it should even be exposed through the\nAPI: if it works, then things are faster, and if it doesn't, then\nthey're not. I don't see the value in an option for causing things to be\nbroken if someone improves the security of the system.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"364894","messageId":"CAPUEspjApDZ3MQr1D1Qu9UNq0QSB_9BKMUUogvABo2VJqao+xw@mail.gmail.com","threadId":"49989","inReplyTo":"20181210004252.GK890086@genre.crustytoothpaste.net","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-12-10T01:25:56Z","receivedAt":"2018-12-10T01:26:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sun, Dec 9, 2018 at 4:42 PM brian m. carlson\n wrote:\n>\n> On Mon, Dec 10, 2018 at 12:51:01AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > Obviously this & what you have in 2/2 needs to be fixed in some way.\n> >\n> > Is the issue on SELinux, OpenBSD, NetBSD etc. *how* PCRE is creating the\n> > the JIT'd code? I.e. presumably Google Chrome's JIT engine, Java JIT and\n> > the like work on those setup, or not? I.e. is this something upstream\n> > can/is likely to fix eventually?\n>\n> From the cover letter (but without testing), it seems like it would\n> probably be fine to first map the pages read-write to write the code and\n> then, once that's done, to map them read-executable. I know JIT\n> compilation does work on the BSDs, so presumably that's the technique to\n> make it do so.\n\nand that has been implemented (sljitProtExecAllocator.c) as part of the work\ntriggered by the bug I linked about [1], deep inside sljit (which is\nwhat pcre uses for JIT)\n\nthe code AS-IS wouldn't compile for the BSD[2] but that is easy to fix\nand sure works as expected but I am under the impression that is not\nsomething that can be considered as a solution as explained by the open issues\ndescribed with crashes after a fork()\n\nnote that changing the map from read-write to executable will be\nprevented by the\nsame policy so you have to create 2 maps (and therefore a backing file) and I\ndon't think there is a way to solve that in a foolproof way in a\nlibrary which is why\nI mentioned more work might be needed to define the right interfaces\nso it can be\nsolved by the application, and that is unlikely to happen with the old library.\n\nCarlo\n\n[1] https://bugs.exim.org/show_bug.cgi?id=1749\n[2] https://bugs.exim.org/show_bug.cgi?id=2155\n"},{"id":"364903","messageId":"xmqqtvjlkhlv.fsf@gitster-ct.c.googlers.com","threadId":"49989","inReplyTo":"20181210004252.GK890086@genre.crustytoothpaste.net","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-10T06:42:36Z","receivedAt":"2018-12-10T06:42:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Considering that some Linux users use PaX kernels with standard\n> distributions and that most BSD kernels can be custom-compiled with a\n> variety of options enabled or disabled, I think this is something we\n> should detect dynamically.\n> ...\n> My view is that JIT is a nice performance optimization, but it's\n> optional. I honestly don't think it should even be exposed through the\n> API: if it works, then things are faster, and if it doesn't, then\n> they're not. I don't see the value in an option for causing things to be\n> broken if someone improves the security of the system.\n\nI agree to both of these two points. Thanks for a thoughtful\ndiscussion, all.\n"},{"id":"364904","messageId":"xmqqpnu9khc3.fsf@gitster-ct.c.googlers.com","threadId":"49989","inReplyTo":"20181209230024.43444-2-carenas@gmail.com","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-10T06:48:28Z","receivedAt":"2018-12-10T06:48:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> JIT support was added to 8.20 but the interface we rely on is only\n> enabled after 8.32 so try to make the message clearer.\n\nCould you add some word before 8.20 and 8.32 (e.g. \"pcre library\nversion 8.20\", if that is what you are referring to, and if 8.32 is\nalso taken from the same context, adding such phrase to clarify the\ncontext only to 8.20 is sufficient)?\n\n> in systems where there are restrictions against creating executable\n> pages programatically (like OpenBSD, NetBSD, macOS or seLinux) JIT\n> will fail, resulting in a error message to the user.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n\nThe change to the code looked sensible.\n"},{"id":"364905","messageId":"xmqqlg4xkh28.fsf@gitster-ct.c.googlers.com","threadId":"49989","inReplyTo":"87r2eqxnru.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-10T06:54:23Z","receivedAt":"2018-12-10T06:54:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> @@ -32,14 +32,14 @@ all::\n>> # USE_LIBPCRE is a synonym for USE_LIBPCRE2, define USE_LIBPCRE1\n>> # instead if you'd like to use the legacy version 1 of the PCRE\n>> # library. Support for version 1 will likely be removed in some future\n>> -# release of Git, as upstream has all but abandoned it.\n>> +# release of Git, as upstream is focusing all development for new\n>> +# features in the newer version instead.\n>\n> I think whatever we do here it makes sense to split this into its own\n> patch, since it doesn't have to do with this fallback mechanism.\n>\n> FWIW I was trying to word this in some way that very briefly described\n> the v1 v.s. v2 situation. Just saying \"new features\" doesn't quite\n> capture it, e.g. some bugs in v1 are closed with some resolution like\n> \"this isn't trivial to fix, use v2 instead\".\n\nI actually think we should remove the paragraph that says \"Support\nfor version 1 will likely to be removed...\", as I do not see how it\nhelps the users. If they have both available, on the day they hear\nthat we are planning to remove pcre1 support and realize that they\nneed to plan to upgrade to the first version of Git that drops pcre1\nsupport, they can switch to pcre2 just fine, so telling them about\nour future that we do not even know definite plan yet does not help\nthem. It is quite unlikely, given that \"upstream has all but\nabandoned it\", that there only is pcre1 support without pcre2 for an\nobscure platform, and even if there are such platforms, the users on\nthem won't have much choice. Discouraging them from using pcre1\nwould not help them make better choices anyway.\n\n\n"},{"id":"364906","messageId":"xmqqh8flkgs2.fsf@gitster-ct.c.googlers.com","threadId":"49989","inReplyTo":"20181209230024.43444-3-carenas@gmail.com","subject":"Re: [RFC PATCH 2/2] grep: fallback to interpreter if JIT fails with pcre2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-10T07:00:29Z","receivedAt":"2018-12-10T07:00:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> starting with 10.23, and as a side effect of the work for bug1749[1] (grep\n> -P crash with seLinux), pcre2grep was modified to ignore any errors from\n> pcre2_jit_compile so the interpreter could be used as a fallback\n\nThat may (or may not---I do not know and I do not particularly feel\nthe need to know or care about pcre2grep that is somebody else's\nproject) describe what happened in the pcre2grep project, but it\nsolicits a \"so what?\" response without the rest of the sentence you\nomitted.\n\nI am guessing that the missing end of the sentence is \"we should\nfollow suit because ...\", i.e. something like\n\n Starting from 10.23 [of what??? pcre2grep's version, libpcre's\n version, or something else???], pcre2grep falls back to\n interpreted pcre when JIT compilation fails. We should follow\n suit in \"git grep\", because ...\n\n\n\n\n> [1] https://bugs.exim.org/show_bug.cgi?id=1749\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> grep.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 5ccc0421a1..c751c8cc74 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -530,8 +530,11 @@ 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 == 1) {\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/* JIT failed so fallback to the interpreter */\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":"364907","messageId":"87pnu9yekk.fsf@evledraar.gmail.com","threadId":"49989","inReplyTo":"20181210004252.GK890086@genre.crustytoothpaste.net","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-10T08:24:27Z","receivedAt":"2018-12-10T08:24:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 10 2018, brian m. carlson wrote:\n\n> On Mon, Dec 10, 2018 at 12:51:01AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> Obviously this & what you have in 2/2 needs to be fixed in some way.\n>>\n>> Is the issue on SELinux, OpenBSD, NetBSD etc. *how* PCRE is creating the\n>> the JIT'd code? I.e. presumably Google Chrome's JIT engine, Java JIT and\n>> the like work on those setup, or not? I.e. is this something upstream\n>> can/is likely to fix eventually?\n>\n> From the cover letter (but without testing), it seems like it would\n> probably be fine to first map the pages read-write to write the code and\n> then, once that's done, to map them read-executable. I know JIT\n> compilation does work on the BSDs, so presumably that's the technique to\n> make it do so.\n>\n> Both versions of PCRE map pages both write and executable at the same\n> time, which is presumably where things go wrong. I assume it can be\n> fixed, but whether that's easy in the context of PCRE, I wouldn't know.\n>\n>> Are we mixing a condition where one some OS's or OS versions this just\n>> won't work at all, and thus maybe should be something turned on in\n>> config.mak.uname, v.s. e.g. SELinux where presumably it'll dynamically\n>> change.\n>\n> Considering that some Linux users use PaX kernels with standard\n> distributions and that most BSD kernels can be custom-compiled with a\n> variety of options enabled or disabled, I think this is something we\n> should detect dynamically.\n\nRight. I'm asking whether we're mixing up cases where it can always be\ndetected at compile-time on some systems v.s. cases where it'll\npotentially change at runtime.\n\n>> I'm inclined to suggest that we should have another ifdef here for \"if\n>> JIT fails I'd like it to die\", so that e.g. packages I build (for\n>> internal use) don't silently slow down in the future, only for me to\n>> find some months later that someone enabled an overzealous SELinux\n>> policy and we swept this under the rug.\n>\n> My view is that JIT is a nice performance optimization, but it's\n> optional. I honestly don't think it should even be exposed through the\n> API: if it works, then things are faster, and if it doesn't, then\n> they're not. I don't see the value in an option for causing things to be\n> broken if someone improves the security of the system.\n\nFor many users that's definitely the case, but for others that's like\nsaying a RDBMS is still going to be functional if the \"ORDER BY\"\nfunction degrades to bubblesort. The JIT improves performance my\nmulti-hundred percents sometimes, so some users (e.g. me) rely on that\nnot being silently degraded.\n\nSo I'm wondering if we can have something like:\n\n if (!jit)\n if (must_have_jit)\n BUG(...); // Like currently\n else\n fallback(); // new behavior\n"},{"id":"365113","messageId":"CAPUEspjHs7+G+FHXjxb4rCcNLqaybbhZESik=14_9Q5h2HMzMA@mail.gmail.com","threadId":"49989","inReplyTo":"87pnu9yekk.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-12-11T20:11:36Z","receivedAt":"2018-12-11T20:11:52Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Dec 10, 2018 at 12:24 AM Ævar Arnfjörð Bjarmason\n wrote:\n> On Mon, Dec 10 2018, brian m. carlson wrote:\n> > Considering that some Linux users use PaX kernels with standard\n> > distributions and that most BSD kernels can be custom-compiled with a\n> > variety of options enabled or disabled, I think this is something we\n> > should detect dynamically.\n>\n> Right. I'm asking whether we're mixing up cases where it can always be\n> detected at compile-time on some systems v.s. cases where it'll\n> potentially change at runtime.\n\nthe closer we come to a system specific issues is with macOS where the\ncompiler (in some newer versions) is allocating the memory using the\nMAP_JIT flag, which seems was originally meant to be only used in iOS\nand has the strange characteristic of failing the mmap for versions\nolder than 10.14 if it was called more than once.\n\nIMHO as brian pointed out, this is better done at runtime.\n\n> >> I'm inclined to suggest that we should have another ifdef here for \"if\n> >> JIT fails I'd like it to die\", so that e.g. packages I build (for\n> >> internal use) don't silently slow down in the future, only for me to\n> >> find some months later that someone enabled an overzealous SELinux\n> >> policy and we swept this under the rug.\n> >\n> > My view is that JIT is a nice performance optimization, but it's\n> > optional. I honestly don't think it should even be exposed through the\n> > API: if it works, then things are faster, and if it doesn't, then\n> > they're not. I don't see the value in an option for causing things to be\n> > broken if someone improves the security of the system.\n>\n> For many users that's definitely the case, but for others that's like\n> saying a RDBMS is still going to be functional if the \"ORDER BY\"\n> function degrades to bubblesort. The JIT improves performance my\n> multi-hundred percents sometimes, so some users (e.g. me) rely on that\n> not being silently degraded.\n\nthe opposite is also true, specially considering that some old\nversions of pcre result in a segfault instead of an error message and\ntherefore since there is no way to disable JIT, the only option left\nis not to use `git grep -P` (or the equivalent git log call)\n\n> So I'm wondering if we can have something like:\n>\n> if (!jit)\n> if (must_have_jit)\n> BUG(...); // Like currently\n> else\n> fallback(); // new behavior\n\nI am wondering if something like a `git doctor` command might be an\ninteresting alternative to this.\n\nThis way we could (for ex: in NetBSD) give the user a hint of what to\ndo to make their git grep -P faster when we detect we are running the\nfallback, and might be useful as well to provide hints for\noptimizations that could be used in other cases (probably even\ndepending on the size of the git repository)\n\nFor your use case, you just need to add a crontab that will trigger an\nalarm if this command ever mentions PCRE\n\nCarlo\n"},{"id":"365118","messageId":"87zhtbn5xb.fsf@evledraar.gmail.com","threadId":"49989","inReplyTo":"CAPUEspjHs7+G+FHXjxb4rCcNLqaybbhZESik=14_9Q5h2HMzMA@mail.gmail.com","subject":"Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-11T20:51:12Z","receivedAt":"2018-12-11T20:51:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 11 2018, Carlo Arenas wrote:\n\n> On Mon, Dec 10, 2018 at 12:24 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> On Mon, Dec 10 2018, brian m. carlson wrote:\n>> > Considering that some Linux users use PaX kernels with standard\n>> > distributions and that most BSD kernels can be custom-compiled with a\n>> > variety of options enabled or disabled, I think this is something we\n>> > should detect dynamically.\n>>\n>> Right. I'm asking whether we're mixing up cases where it can always be\n>> detected at compile-time on some systems v.s. cases where it'll\n>> potentially change at runtime.\n>\n> the closer we come to a system specific issues is with macOS where the\n> compiler (in some newer versions) is allocating the memory using the\n> MAP_JIT flag, which seems was originally meant to be only used in iOS\n> and has the strange characteristic of failing the mmap for versions\n> older than 10.14 if it was called more than once.\n>\n> IMHO as brian pointed out, this is better done at runtime.\n\nSure. Just something I was wondering since it wasn't clear from the\npatch. Makes sense, if it's always runtime (or not worth the effort to\ndivide the two) let's do that.\n\n>> >> I'm inclined to suggest that we should have another ifdef here for \"if\n>> >> JIT fails I'd like it to die\", so that e.g. packages I build (for\n>> >> internal use) don't silently slow down in the future, only for me to\n>> >> find some months later that someone enabled an overzealous SELinux\n>> >> policy and we swept this under the rug.\n>> >\n>> > My view is that JIT is a nice performance optimization, but it's\n>> > optional. I honestly don't think it should even be exposed through the\n>> > API: if it works, then things are faster, and if it doesn't, then\n>> > they're not. I don't see the value in an option for causing things to be\n>> > broken if someone improves the security of the system.\n>>\n>> For many users that's definitely the case, but for others that's like\n>> saying a RDBMS is still going to be functional if the \"ORDER BY\"\n>> function degrades to bubblesort. The JIT improves performance my\n>> multi-hundred percents sometimes, so some users (e.g. me) rely on that\n>> not being silently degraded.\n>\n> the opposite is also true, specially considering that some old\n> versions of pcre result in a segfault instead of an error message and\n> therefore since there is no way to disable JIT, the only option left\n> is not to use `git grep -P` (or the equivalent git log call)\n\nRight, of course it segfaulting is a bug...\n\n>> So I'm wondering if we can have something like:\n>>\n>> if (!jit)\n>> if (must_have_jit)\n>> BUG(...); // Like currently\n>> else\n>> fallback(); // new behavior\n>\n> I am wondering if something like a `git doctor` command might be an\n> interesting alternative to this.\n>\n> This way we could (for ex: in NetBSD) give the user a hint of what to\n> do to make their git grep -P faster when we detect we are running the\n> fallback, and might be useful as well to provide hints for\n> optimizations that could be used in other cases (probably even\n> depending on the size of the git repository)\n\nSuch a command has been discussed before on-list. I think it's a good\nidea for the more fuzzy things like optimization suggests, but for the\ncase of expecting something at compile-time where not having that at\nruntime is a boolean state it's nicer to just die with a BUG(...).\n\n> For your use case, you just need to add a crontab that will trigger an\n> alarm if this command ever mentions PCRE\n\n...The reason I'd like it to die is because it neatly and naturally\nintegrates with all existing test infrastructure I have. I.e. build\npackage, run stress tests on all sorts of machines, see that it passes,\nand SELinux isn't ruining it or whatever.\n\nI know this works now (I always get PCRE v2 JIT) because it doesn't die\nor segfault. I'd like not to have it regress to having worse\nperformance.\n\nHaving a cronjob to test for \"does PCRE v2 JIT still work?\" is not as\neasy & isn't a drop-in solution.\n"}]}