{"thread":{"id":"47119","subject":"git grep -P fatal: pcre_exec failed with error code -8","startedAt":"2017-11-05T00:15:46Z","lastAt":"2017-11-06T14:04:10Z","messageCount":7,"participants":["Дилян Палаузов","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"331834","messageId":"635a9bd4-2aa9-4415-2756-b82370c55798@aegee.org","threadId":"47119","inReplyTo":null,"subject":"git grep -P fatal: pcre_exec failed with error code -8","fromName":"Дилян Палаузов","fromEmail":"dilyan.palauzov@aegee.org","sentAt":"2017-11-05T00:06:21Z","receivedAt":"2017-11-05T00:15:46Z","isPatch":false,"sender":{"key":"dilyan.palauzov@aegee.org","avatar":null},"body":"Hello,\n\nwith git 2.14.3 linked with libpcre.so.1.2.9 when I do:\n   git clone https://github.com/django/django\n   cd django\n   git grep -P \"if.*([^\\s])+\\s+and\\s+\\1\" \ndjango/contrib/admin/static/admin/js/vendor/select2/select2.full.min.js\nthe output is:\n   fatal: pcre_exec failed with error code -8\n\n\n(But not with\ngit clone https://github.com/select2/select2\ncd select2\ngit grep -P \"if.*([^\\s])+\\s+and\\s+\\1\" dist/js/select2.full.min.js\n\nas the two select2.full.min.js files differ slightly in their size)\n\nAny ideas?\n\nRegards\n   Dilian\n"},{"id":"331839","messageId":"20171105021623.yi46w2awwy7p3q6e@sigill.intra.peff.net","threadId":"47119","inReplyTo":"635a9bd4-2aa9-4415-2756-b82370c55798@aegee.org","subject":"Re: git grep -P fatal: pcre_exec failed with error code -8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-05T02:16:23Z","receivedAt":"2017-11-05T02:16:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 05, 2017 at 01:06:21AM +0100, Дилян Палаузов wrote:\n\n> with git 2.14.3 linked with libpcre.so.1.2.9 when I do:\n>   git clone https://github.com/django/django\n>   cd django\n>   git grep -P \"if.*([^\\s])+\\s+and\\s+\\1\"\n> django/contrib/admin/static/admin/js/vendor/select2/select2.full.min.js\n> the output is:\n>   fatal: pcre_exec failed with error code -8\n\nCode -8 is PCRE_ERROR_MATCHLIMIT. And \"man pcreapi\" has this to say:\n\n  The match_limit field provides a means of preventing PCRE from\n  using up a vast amount of resources when running patterns that\n  are not going to match, but which have a very large number of\n  possibilities in their search trees. The classic example is a\n  pattern that uses nested unlimited repeats.\n\n  Internally, pcre_exec() uses a function called match(), which\n  it calls repeatedly (sometimes recursively). The limit set by\n  match_limit is imposed on the number of times this function is\n  called during a match, which has the effect of limiting the\n  amount of backtracking that can take place. For patterns that\n  are not anchored, the count restarts from zero for each posi‐\n  tion in the subject string.\n\n  When pcre_exec() is called with a pattern that was successfully\n  studied with a JIT option, the way that the matching is exe‐\n  cuted is entirely different. However, there is still the pos‐\n  sibility of runaway matching that goes on for a very long time,\n  and so the match_limit value is also used in this case (but in\n  a different way) to limit how long the matching can continue.\n\n  The default value for the limit can be set when PCRE is built;\n  the default default is 10 million, which handles all but the\n  most extreme cases. You can override the default by suppling\n  pcre_exec() with a pcre_extra block in which match_limit is\n  set, and PCRE_EXTRA_MATCH_LIMIT is set in the flags field. If\n  the limit is exceeded, pcre_exec() returns PCRE_ERROR_MATCH‐\n  LIMIT.\n\nSo your pattern is just really expensive and is running afoul of pcre's\nbacktracking limits (and it's not helped by the fact that the file is\nbasically one giant line).\n\nThere's no way to ask Git to specify a larger match_limit to pcre, but\nyou might be able to construct your pattern in a way that involves less\nbacktracking. It looks like you're trying to find things like \"if foo\nand foo\"?\n\nShould the captured term actually be \"([^\\s]+)\" (with the \"+\" on the\n_inside_ of the capture? Or maybe I'm just misunderstanding your goal.\n\n-Peff\n"},{"id":"331859","messageId":"4e2bc579-429f-9927-4502-5929e5235740@aegee.org","threadId":"47119","inReplyTo":"20171105021623.yi46w2awwy7p3q6e@sigill.intra.peff.net","subject":"Re: git grep -P fatal: pcre_exec failed with error code -8","fromName":"Дилян Палаузов","fromEmail":"dilyan.palauzov@aegee.org","sentAt":"2017-11-05T09:41:17Z","receivedAt":"2017-11-05T09:41:26Z","isPatch":false,"sender":{"key":"dilyan.palauzov@aegee.org","avatar":null},"body":"Hello,\n\nthanks for your answer.\n\nI understand that the PCRE's stack can get exhausted for some files, but \nin such cases, git grep shall proceed with the other files, and print at \nthe end/stderr for which files the pattern was not applied.  Such \nbehaviour would be more usefull than the current one.\n\nRegards\n   Dilian\n\nOn 11/05/2017 03:16 AM, Jeff King wrote:\n> On Sun, Nov 05, 2017 at 01:06:21AM +0100, Дилян Палаузов wrote:\n> \n>> with git 2.14.3 linked with libpcre.so.1.2.9 when I do:\n>>    git clone https://github.com/django/django\n>>    cd django\n>>    git grep -P \"if.*([^\\s])+\\s+and\\s+\\1\"\n>> django/contrib/admin/static/admin/js/vendor/select2/select2.full.min.js\n>> the output is:\n>>    fatal: pcre_exec failed with error code -8\n> \n> Code -8 is PCRE_ERROR_MATCHLIMIT. And \"man pcreapi\" has this to say:\n> \n>    The match_limit field provides a means of preventing PCRE from\n>    using up a vast amount of resources when running patterns that\n>    are not going to match, but which have a very large number of\n>    possibilities in their search trees. The classic example is a\n>    pattern that uses nested unlimited repeats.\n> \n>    Internally, pcre_exec() uses a function called match(), which\n>    it calls repeatedly (sometimes recursively). The limit set by\n>    match_limit is imposed on the number of times this function is\n>    called during a match, which has the effect of limiting the\n>    amount of backtracking that can take place. For patterns that\n>    are not anchored, the count restarts from zero for each posi‐\n>    tion in the subject string.\n> \n>    When pcre_exec() is called with a pattern that was successfully\n>    studied with a JIT option, the way that the matching is exe‐\n>    cuted is entirely different. However, there is still the pos‐\n>    sibility of runaway matching that goes on for a very long time,\n>    and so the match_limit value is also used in this case (but in\n>    a different way) to limit how long the matching can continue.\n> \n>    The default value for the limit can be set when PCRE is built;\n>    the default default is 10 million, which handles all but the\n>    most extreme cases. You can override the default by suppling\n>    pcre_exec() with a pcre_extra block in which match_limit is\n>    set, and PCRE_EXTRA_MATCH_LIMIT is set in the flags field. If\n>    the limit is exceeded, pcre_exec() returns PCRE_ERROR_MATCH‐\n>    LIMIT.\n> \n> So your pattern is just really expensive and is running afoul of pcre's\n> backtracking limits (and it's not helped by the fact that the file is\n> basically one giant line).\n> \n> There's no way to ask Git to specify a larger match_limit to pcre, but\n> you might be able to construct your pattern in a way that involves less\n> backtracking. It looks like you're trying to find things like \"if foo\n> and foo\"?\n> \n> Should the captured term actually be \"([^\\s]+)\" (with the \"+\" on the\n> _inside_ of the capture? Or maybe I'm just misunderstanding your goal.\n> \n> -Peff\n> \n"},{"id":"331913","messageId":"20171106103125.fwtrxv6zycrbihcv@sigill.intra.peff.net","threadId":"47119","inReplyTo":"4e2bc579-429f-9927-4502-5929e5235740@aegee.org","subject":"Re: git grep -P fatal: pcre_exec failed with error code -8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-06T10:31:26Z","receivedAt":"2017-11-06T10:31:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 05, 2017 at 10:41:17AM +0100, Дилян Палаузов wrote:\n\n> I understand that the PCRE's stack can get exhausted for some files, but in\n> such cases, git grep shall proceed with the other files, and print at the\n> end/stderr for which files the pattern was not applied.  Such behaviour\n> would be more usefull than the current one.\n\nYes, I had a similar thought. It does feel a little funny for us to\nbasically treat an error as \"no match\" for non-interactive use, but then\nthe current behavior works out to be more or less the same (we return an\nerror code which most shell scripts would interpret as failure).\n\nIOW, I think something like this is probably the right direction:\n\ndiff --git a/grep.c b/grep.c\nindex ce6a48e634..2c152e5908 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -427,7 +427,7 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n \t}\n \n \tif (ret < 0 && ret != PCRE_ERROR_NOMATCH)\n-\t\tdie(\"pcre_exec failed with error code %d\", ret);\n+\t\twarning(\"pcre_exec failed with error code %d\", ret);\n \tif (ret > 0) {\n \t\tret = 0;\n \t\tmatch->rm_so = ovector[0];\n\nbut possibly:\n\n  1. It would be nice to report the filename that we couldn't match on.\n     But we don't know it at this level of the code (and it might not be\n     a file at all that we are matching). So probably we'd want to pass\n     the error much further up the call stack. This is tricky as there\n     are multiple regex libraries we can use, and the return value gets\n     normalized to 1/0 for hit/not-hit long before we get as far as\n     something that knows the filename.\n\n     We might need to do something invasive like adding an extra\n     parameter to hold the error message, and passing it through the\n     whole stack.\n\n  2. We should still try to exit with an exit code other than \"1\" to\n     indicate we hit an error besides \"no lines were found\".\n\n  3. Other regex libraries might need similar treatment. Probably\n     pcre2match() needs it. It doesn't look like regexec() can ever\n     return an error besides REG_NOMATCH.\n\n-Peff\n"},{"id":"331925","messageId":"878tfj62y2.fsf@evledraar.booking.com","threadId":"47119","inReplyTo":"20171106103125.fwtrxv6zycrbihcv@sigill.intra.peff.net","subject":"Re: git grep -P fatal: pcre_exec failed with error code -8","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-11-06T11:50:45Z","receivedAt":"2017-11-06T11:50:54Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 06 2017, Jeff King jotted:\n\n> On Sun, Nov 05, 2017 at 10:41:17AM +0100, Дилян Палаузов wrote:\n>\n>> I understand that the PCRE's stack can get exhausted for some files, but in\n>> such cases, git grep shall proceed with the other files, and print at the\n>> end/stderr for which files the pattern was not applied.  Such behaviour\n>> would be more usefull than the current one.\n>\n> Yes, I had a similar thought. It does feel a little funny for us to\n> basically treat an error as \"no match\" for non-interactive use, but then\n> the current behavior works out to be more or less the same (we return an\n> error code which most shell scripts would interpret as failure).\n>\n> IOW, I think something like this is probably the right direction:\n>\n> diff --git a/grep.c b/grep.c\n> index ce6a48e634..2c152e5908 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -427,7 +427,7 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n>  \t}\n>\n>  \tif (ret < 0 && ret != PCRE_ERROR_NOMATCH)\n> -\t\tdie(\"pcre_exec failed with error code %d\", ret);\n> +\t\twarning(\"pcre_exec failed with error code %d\", ret);\n>  \tif (ret > 0) {\n>  \t\tret = 0;\n>  \t\tmatch->rm_so = ovector[0];\n>\n> but possibly:\n>\n>   1. It would be nice to report the filename that we couldn't match on.\n>      But we don't know it at this level of the code (and it might not be\n>      a file at all that we are matching). So probably we'd want to pass\n>      the error much further up the call stack. This is tricky as there\n>      are multiple regex libraries we can use, and the return value gets\n>      normalized to 1/0 for hit/not-hit long before we get as far as\n>      something that knows the filename.\n>\n>      We might need to do something invasive like adding an extra\n>      parameter to hold the error message, and passing it through the\n>      whole stack.\n>\n>   2. We should still try to exit with an exit code other than \"1\" to\n>      indicate we hit an error besides \"no lines were found\".\n>\n>   3. Other regex libraries might need similar treatment. Probably\n>      pcre2match() needs it. It doesn't look like regexec() can ever\n>      return an error besides REG_NOMATCH.\n>\n> -Peff\n\nSome replies to the thread in general, didn't want to spread this out\ninto different replies.\n\n * Yes this sucks.\n\n * Just emitting a warning without an appropriate exit code would suck\n   more, would break batch jobs & whatnot that expcept certain results\n   from grep.\n\n * As you point out it would be nice to print out the file name we\n   didn't match on, we'd need to pass the grep_source struct down\n   further, it goes as far as grep_source_1 but stops there and isn't\n   passed to e.g. look_ahead(), which calls patmatch() which calls the\n   engine-specific matcher and would need to report the error. We could\n   just do this, would slow down things a bit (probably trivally) but we\n   could emit better error messages in genreal.\n\n * You can adjust these limts in PCRE in Git, although it doesn't help\n   in this case, you just add (*LIMIT_MATCH=NUM) or\n   (*LIMIT_RECURSION=NUM) (or both) to the start of the pattern. See\n   pcresyntax(3) or pcre2syntax(3) man pages depending on what version\n   you have installed.\n\n * While regexec() won't return an error its version of dealing with\n   this is (at least under glibc) to balloon CPU/memory use until the\n   OOMkiller kills git (although not on this particular pattern).\n"},{"id":"331926","messageId":"20171106122411.dhi2ltyegzquebhk@sigill.intra.peff.net","threadId":"47119","inReplyTo":"878tfj62y2.fsf@evledraar.booking.com","subject":"Re: git grep -P fatal: pcre_exec failed with error code -8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-06T12:24:11Z","receivedAt":"2017-11-06T12:24:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 06, 2017 at 12:50:45PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Some replies to the thread in general, didn't want to spread this out\n> into different replies.\n> \n>  * Yes this sucks.\n> \n>  * Just emitting a warning without an appropriate exit code would suck\n>    more, would break batch jobs & whatnot that expcept certain results\n>    from grep.\n\nThat was my first thought, too, but something that does:\n\n  git grep foo && echo found\n\nwould behave basically the same. Do you mean here scripts that actually\ndo:\n\n  git grep foo\n  case \"$?\" in\n  0) echo found ;;\n  1) echo not found ;;\n  *) echo wtf? ;;\n  esac\n\nI agree it would be nice to at least have _some_ way to distinguish\nbetween those final two cases.\n\nThough something like \"git log --grep\" is even more complicated. We\nperhaps _would_ want to distinguish between:\n\n  - match (in which case we print the commit)\n\n  - no match (in which case we do not)\n\n  - error (in which case we do not print, but also mark the exit code as\n    failing)\n\n>  * As you point out it would be nice to print out the file name we\n>    didn't match on, we'd need to pass the grep_source struct down\n>    further, it goes as far as grep_source_1 but stops there and isn't\n>    passed to e.g. look_ahead(), which calls patmatch() which calls the\n>    engine-specific matcher and would need to report the error. We could\n>    just do this, would slow down things a bit (probably trivally) but we\n>    could emit better error messages in genreal.\n\nI'm not sure if the grep_source has enough information for all cases.\nE.g., if you hit an error while grepping in commit headers, you'd\nprobably want to mention the oid of the commit. There's an \"identifier\"\nfield in the grep_source, but it's opaque.\n\nThe caller may also want to do more things than just print an error\n(like the exit code adjustment I mentioned above). Which implies to me\nwe should be passing the error information up, not trying to bring the\ncontext down.\n\n>  * You can adjust these limts in PCRE in Git, although it doesn't help\n>    in this case, you just add (*LIMIT_MATCH=NUM) or\n>    (*LIMIT_RECURSION=NUM) (or both) to the start of the pattern. See\n>    pcresyntax(3) or pcre2syntax(3) man pages depending on what version\n>    you have installed.\n\nI saw that in the pcre manual, but I had the impression you can't\nactually raise the limits above the defaults with that, only lower them.\n\n>  * While regexec() won't return an error its version of dealing with\n>    this is (at least under glibc) to balloon CPU/memory use until the\n>    OOMkiller kills git (although not on this particular pattern).\n\nSo in a sense our current behavior with pcre is the same. We just have\nto provoke the death ourselves. ;)\n\n-Peff\n"},{"id":"331927","messageId":"877ev35wrx.fsf@evledraar.booking.com","threadId":"47119","inReplyTo":"20171106122411.dhi2ltyegzquebhk@sigill.intra.peff.net","subject":"Re: git grep -P fatal: pcre_exec failed with error code -8","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-11-06T14:04:02Z","receivedAt":"2017-11-06T14:04:10Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 06 2017, Jeff King jotted:\n\n> On Mon, Nov 06, 2017 at 12:50:45PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Some replies to the thread in general, didn't want to spread this out\n>> into different replies.\n>>\n>>  * Yes this sucks.\n>>\n>>  * Just emitting a warning without an appropriate exit code would suck\n>>    more, would break batch jobs & whatnot that expcept certain results\n>>    from grep.\n>\n> That was my first thought, too, but something that does:\n>\n>   git grep foo && echo found\n>\n> would behave basically the same. Do you mean here scripts that actually\n> do:\n>\n>   git grep foo\n>   case \"$?\" in\n>   0) echo found ;;\n>   1) echo not found ;;\n>   *) echo wtf? ;;\n>   esac\n>\n> I agree it would be nice to at least have _some_ way to distinguish\n> between those final two cases.\n\nMaybe we should emit a different exit code, but I just had in mind that\nwe could continue but the same non-zero as when the grep fails now, but\nwith an additional warning.\n\n> Though something like \"git log --grep\" is even more complicated. We\n> perhaps _would_ want to distinguish between:\n>\n>   - match (in which case we print the commit)\n>\n>   - no match (in which case we do not)\n>\n>   - error (in which case we do not print, but also mark the exit code as\n>     failing)\n>\n>>  * As you point out it would be nice to print out the file name we\n>>    didn't match on, we'd need to pass the grep_source struct down\n>>    further, it goes as far as grep_source_1 but stops there and isn't\n>>    passed to e.g. look_ahead(), which calls patmatch() which calls the\n>>    engine-specific matcher and would need to report the error. We could\n>>    just do this, would slow down things a bit (probably trivally) but we\n>>    could emit better error messages in genreal.\n>\n> I'm not sure if the grep_source has enough information for all cases.\n> E.g., if you hit an error while grepping in commit headers, you'd\n> probably want to mention the oid of the commit. There's an \"identifier\"\n> field in the grep_source, but it's opaque.\n>\n> The caller may also want to do more things than just print an error\n> (like the exit code adjustment I mentioned above). Which implies to me\n> we should be passing the error information up, not trying to bring the\n> context down.\n\nIndeed, I was just thinking of the case where we're grepping a file in\nthe tree, not when the engine is used for --grep et al.\n\n>>  * You can adjust these limts in PCRE in Git, although it doesn't help\n>>    in this case, you just add (*LIMIT_MATCH=NUM) or\n>>    (*LIMIT_RECURSION=NUM) (or both) to the start of the pattern. See\n>>    pcresyntax(3) or pcre2syntax(3) man pages depending on what version\n>>    you have installed.\n>\n> I saw that in the pcre manual, but I had the impression you can't\n> actually raise the limits above the defaults with that, only lower them.\n\nYou can, you just can't set them to anything less conservative than\nlimits already set via the C API, but we don't set any of those.\n\nI.e. PCRE allows you to say expose a regex field in a web form (as we do\nwith gitweb) with really conservative settings that can't be overriden\nvia a (*LIMIT) set in the pattern.\n\nBut since we don't use that C API PCRE runs in a mode where the user\ngets set whatever limit they want in the pattern (or other\npattern-altering switch), which makes sense for interactive git-grep\nuse.\n\n>>  * While regexec() won't return an error its version of dealing with\n>>    this is (at least under glibc) to balloon CPU/memory use until the\n>>    OOMkiller kills git (although not on this particular pattern).\n>\n> So in a sense our current behavior with pcre is the same. We just have\n> to provoke the death ourselves. ;)\n\nIndeed :)\n"}]}