{"thread":{"id":"31958","subject":"crash on git diff-tree -Ganything <tree> for new files with textconv filter","startedAt":"2012-10-27T18:37:24Z","lastAt":"2013-06-03T22:17:16Z","messageCount":24,"participants":["Peter Oberndorfer","Jeff King","Junio C Hamano","Ramsay Jones"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"202012","messageId":"508C29E4.5000801@arcor.de","threadId":"31958","inReplyTo":null,"subject":"crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2012-10-27T18:37:24Z","receivedAt":"2012-10-27T18:37:24Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"Hi,\n\nIt seems \"git diff-tree -Ganything <tree>\" crashes[1] with a null\npointer dereference\nwhen run on a commit that adds a file (pdf) with a textconv filter.\n\nIt can be reproduced with vanilla git by having a commit on top that\nadds a file with a textconv filter and executing git diff-tree\n-Ganything HEAD\nBut running git log -Ganything still works without a crash.\nThis problem seems to exist since the feature was first added in f506b8e8b5.\n\nWhile testing I also noticed the -S and -G act on the original file\ninstead of the textconv munged data.\nIs this intentional or caused by accessing the wrong data?\nWild guess: should we really access p->one->data and not mf1.ptr?\n\nIs there some more information i should provide?\n\nGreetings Peter\n\n[1]\nI am running msysgit v1.8.0.msysgit.0 (52d3a7583a)\nand i tried added -G pickaxe support to gitk.\ngitk runs git diff-tree -r -s --stdin -Gpattern\n\nThis is how i detected the crash the first time.\n(but only because of a crash popup on Windows, gitk does not complain...)\n\nFor testing on vanilla git I used .git/config:\n[diff \"upcase2\"]\n    textconv = tr a-z A-Z <\n\n.gitatrributes:\nnewtext diff=upcase2\n\nProgram received signal SIGSEGV, Segmentation fault.\n[Switching to thread 3864.0x83c]\n0x0049d90c in regexec (preg=0x22f900, string=0x0, nmatch=1,\npmatch=0x22f46c, eflags=0) at compat/regex/regexec.c:241\n241           length = strlen (string);\n(gdb) bt\n#0  0x0049d90c in regexec (preg=0x22f900, string=0x0, nmatch=1,\npmatch=0x22f46c, eflags=0) at compat/regex/regexec.c:241\n#1  0x004f5763 in diff_grep (p=0x109a530, o=0x550b48, regexp=0x22f900,\nkws=0x0) at diffcore-pickaxe.c:110\n#2  0x004f59dc in pickaxe (o=<value optimized out>, regexp=0x22f900,\nkws=0xffffffff, fn=0x4f5620 <diff_grep>) at diffcore-pickaxe.c:40\n#3  0x004f5bd4 in diffcore_pickaxe_grep (o=0x550b48) at\ndiffcore-pickaxe.c:154\n#4  0x0048049a in diffcore_std (options=0x550b48) at diff.c:4630\n#5  0x004dc16a in log_tree_diff_flush (opt=0x5508c0) at log-tree.c:697\n#6  0x004dc32e in log_tree_commit (opt=0x5508c0, commit=0xffc620) at\nlog-tree.c:790\n#7  0x004206dd in cmd_diff_tree (argc=<value optimized out>,\nargv=0x3d24bc, prefix=0x0) at builtin/diff-tree.c:43\n#8  0x00401a16 in handle_internal_command (argc=<value optimized out>,\nargv=<value optimized out>) at git.c:306\n#9  0x00401c00 in main (argc=6, argv=0x3d24b8) at git.c:513\n(gdb) up\n#1  0x004f5763 in diff_grep (p=0x109a530, o=0x550b48, regexp=0x22f900,\nkws=0x0) at diffcore-pickaxe.c:110\n110                     hit = !regexec(regexp, p->one->data, 1,\n&regmatch, 0);\n(gdb) info locals\nregmatch = {rm_so = 17408640, rm_eo = 2291968}\ntextconv_one = (struct userdiff_driver *) 0x0\ntextconv_two = (struct userdiff_driver *) 0xe10468\nmf1 = {ptr = 0x0, size = 0}\nmf2 = {\n  ptr = 0x2bbcaf0 ' ' <repeats 52 times>, \"PROJECT\nDESCRIPTION\\r\\n\\r\\n\\r\\nProject Number:\", ' ' <repeats 19 times>,\n\"xxxxx\\r\\nProject Description:\", ' ' <repeats 14 times>,\n\"xxxxxxxx\\r\\nxxxxx:\", ' ' <repeats 11 times>..., size = 64185}\nhit = <value optimized out>\n(gdb) print p\n$1 = (struct diff_filepair *) 0x109a530\n(gdb) print *p\n$2 = {one = 0x109a320, two = 0x109a428, score = 0, status = 0 '\\0',\nbroken_pair = 0, renamed_pair = 0, is_unmerged = 0}\n(gdb) print p->one\n$3 = (struct diff_filespec *) 0x109a320\n(gdb) print *p->one\n$4 = {sha1 = '\\0' <repeats 19 times>, path = 0x109a360\n\"xxxxxxxx/xxx/doc/xxxx xxxxx 1.4.pdf\", data = 0x0, cnt_data = 0x0,\n  funcname_pattern_ident = 0x0, size = 0, count = 1, xfrm_flags = 0,\nrename_used = 0, mode = 0, sha1_valid = 0, should_free = 0,\nshould_munmap = 0,\n  dirty_submodule = 0, is_stdin = 0, has_more_entries = 0, driver = 0x0,\nis_binary = -1}\n(gdb) print *p->two\n$5 = {sha1 = \"0aax\\217\\231)oBaAa(\\021\\234^'Q\\236\\230\", path = 0x109a468\n\"xxxxxxxx/xxx/doc/xxxx xxxxx 1.4.pdf\", data = 0x0,\n  cnt_data = 0x0, funcname_pattern_ident = 0x0, size = 0, count = 1,\nxfrm_flags = 0, rename_used = 0, mode = 33188, sha1_valid = 1,\nshould_free = 0,\n  should_munmap = 0, dirty_submodule = 0, is_stdin = 0, has_more_entries\n= 0, driver = 0xe10468, is_binary = -1}\n(gdb) print textconv_one\n$6 = (struct userdiff_driver *) 0x0\n(gdb) print textconv_two\n$7 = (struct userdiff_driver *) 0xe10468\n(gdb) print *textconv_two\n$10 = {name = 0xfe4fc0 \"astextplain\", external = 0x0, binary = -1,\nfuncname = {pattern = 0x0, cflags = 0}, word_regex = 0x0,\n  textconv = 0xfe4fd8 \"astextplain\", textconv_cache = 0x0,\ntextconv_want_cache = 0}\n(gdb)\n"},{"id":"202039","messageId":"20121028120104.GE11434@sigill.intra.peff.net","threadId":"31958","inReplyTo":"508C29E4.5000801@arcor.de","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-28T12:01:04Z","receivedAt":"2012-10-28T12:01:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 27, 2012 at 08:37:24PM +0200, Peter Oberndorfer wrote:\n\n> It seems \"git diff-tree -Ganything <tree>\" crashes[1] with a null\n> pointer dereference\n> when run on a commit that adds a file (pdf) with a textconv filter.\n> \n> It can be reproduced with vanilla git by having a commit on top that\n> adds a file with a textconv filter and executing git diff-tree\n> -Ganything HEAD\n> But running git log -Ganything still works without a crash.\n> This problem seems to exist since the feature was first added in f506b8e8b5.\n\nThanks for a thorough bug report. I didn't reproduce the crash, but I\ncan see how it happens (it happens with diff-tree because we will reuse\nthe working tree file in that instance; it could also happen if you\nturned on textconv caching).\n\n> While testing I also noticed the -S and -G act on the original file\n> instead of the textconv munged data.\n> Is this intentional or caused by accessing the wrong data?\n\nBoth, perhaps. :)\n\n-G operates on the munged data; you can see it feed the munged data to\nxdiff in diff_grep. But the optimization for handling added and removed\nfiles accidentally fed the wrong pointer. Fixing that is a no-brainer,\nsince the optimization is inconsistent with the regular code path.\n\n-S, however, predates the invention of textconv and has never used it.\nIt is a little less clear that textconv is the right thing here, because\nit is not about grepping the diff, but about counting occurrences of the\nstring inside the file content. I tend to think that doing so on the\ntextconv'd data would be what people generally want, but it is a\nbehavior change.\n\n> Wild guess: should we really access p->one->data and not mf1.ptr?\n\nPrecisely. Thanks for your wild guess; it made finding the bug very\neasy. :)\n\n> Is there some more information i should provide?\n\nThe patch below should fix it. I added tests, but please try your\nreal-world test case on it to double-check.\n\n-- >8 --\nSubject: [PATCH] diff_grep: use textconv buffers for add/deleted files\n\nIf you use \"-G\" to grep a diff, we will apply a configured\ntextconv filter to the data before generating the diff.\nHowever, if the diff is an addition or deletion, we do not\nbother running the diff at all, and just look for the token\nin the added (or removed) content. This works because we\nknow that the diff must contain every line of content.\n\nHowever, while we used the textconv-derived buffers in the\nregular diff, we accidentally passed the original unmodified\nbuffers to regexec when checking the added or removed\ncontent. This could lead to an incorrect answer.\n\nWorse, in some cases we might have a textconv buffer but no\noriginal buffer (e.g., if we pulled the textconv data from\ncache, or if we reused a working tree file when generating\nit). In that case, we could actually feed NULL to regexec\nand segfault.\n\nReported-by: Peter Oberndorfer <kumbayo84@arcor.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diffcore-pickaxe.c       |  4 ++--\n t/t4030-diff-textconv.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex ed23eb4..a209376 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -104,10 +104,10 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o,\n \t\tif (!mf2.ptr)\n \t\t\treturn 0; /* ignore unmerged */\n \t\t/* created \"two\" -- does it have what we are looking for? */\n-\t\thit = !regexec(regexp, p->two->data, 1, &regmatch, 0);\n+\t\thit = !regexec(regexp, mf2.ptr, 1, &regmatch, 0);\n \t} else if (!mf2.ptr) {\n \t\t/* removed \"one\" -- did it have what we are looking for? */\n-\t\thit = !regexec(regexp, p->one->data, 1, &regmatch, 0);\n+\t\thit = !regexec(regexp, mf1.ptr, 1, &regmatch, 0);\n \t} else {\n \t\t/*\n \t\t * We have both sides; need to run textual diff and see if\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex eebb1ee..461d27a 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -84,6 +84,18 @@ test_expect_success 'status -v produces text' '\n \tgit reset --soft HEAD@{1}\n '\n \n+test_expect_success 'grep-diff (-G) operates on textconv data (add)' '\n+\techo one >expect &&\n+\tgit log --root --format=%s -G0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep-diff (-G) operates on textconv data (modification)' '\n+\techo two >expect &&\n+\tgit log --root --format=%s -G1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >expect.stat <<'EOF'\n  file | Bin 2 -> 4 bytes\n  1 file changed, 0 insertions(+), 0 deletions(-)\n-- \n1.8.0.3.g3456896\n"},{"id":"202040","messageId":"20121028124540.GF11434@sigill.intra.peff.net","threadId":"31958","inReplyTo":"20121028120104.GE11434@sigill.intra.peff.net","subject":"[PATCH 0/2] textconv support for \"log -S\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-28T12:45:40Z","receivedAt":"2012-10-28T12:45:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 28, 2012 at 08:01:04AM -0400, Jeff King wrote:\n\n> -G operates on the munged data; you can see it feed the munged data to\n> xdiff in diff_grep. But the optimization for handling added and removed\n> files accidentally fed the wrong pointer. Fixing that is a no-brainer,\n> since the optimization is inconsistent with the regular code path.\n> \n> -S, however, predates the invention of textconv and has never used it.\n> It is a little less clear that textconv is the right thing here, because\n> it is not about grepping the diff, but about counting occurrences of the\n> string inside the file content. I tend to think that doing so on the\n> textconv'd data would be what people generally want, but it is a\n> behavior change.\n\nI prepared the earlier bugfix for \"-G\" for maint. Modifying \"-S\" would\nbe a separate feature topic, and would look like this (I built it on top\nof the bugfix patch, since the tests are a follow-on).\n\n  [1/2]: pickaxe: hoist empty needle check\n  [2/2]: pickaxe: use textconv for -S counting\n\n-Peff\n"},{"id":"202041","messageId":"20121028124628.GA24548@sigill.intra.peff.net","threadId":"31958","inReplyTo":"20121028124540.GF11434@sigill.intra.peff.net","subject":"[PATCH 1/2] pickaxe: hoist empty needle check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-28T12:46:28Z","receivedAt":"2012-10-28T12:46:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we are given an empty pickaxe needle like \"git log -S ''\",\nit is impossible for us to find anything (because no matter\nwhat the content, the count will always be 0). We currently\ncheck this at the lowest level of contains(). Let's hoist\nthe logic much earlier to has_changes(), so that it is\nsimpler to return our answer before loading any blob data.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diffcore-pickaxe.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex a209376..61f628c 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -163,8 +163,6 @@ static unsigned int contains(struct diff_filespec *one, struct diff_options *o,\n \tunsigned int cnt;\n \tunsigned long sz;\n \tconst char *data;\n-\tif (!o->pickaxe[0])\n-\t\treturn 0;\n \tif (diff_populate_filespec(one, 0))\n \t\treturn 0;\n \n@@ -206,6 +204,9 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \t\t       regex_t *regexp, kwset_t kws)\n {\n+\tif (!o->pickaxe[0])\n+\t\treturn 0;\n+\n \tif (!DIFF_FILE_VALID(p->one)) {\n \t\tif (!DIFF_FILE_VALID(p->two))\n \t\t\treturn 0; /* ignore unmerged */\n-- \n1.8.0.3.g3456896\n"},{"id":"202042","messageId":"20121028124701.GB24548@sigill.intra.peff.net","threadId":"31958","inReplyTo":"20121028124540.GF11434@sigill.intra.peff.net","subject":"[PATCH 2/2] pickaxe: use textconv for -S counting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-28T12:47:01Z","receivedAt":"2012-10-28T12:47:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently just look at raw blob data when using \"-S\" to\npickaxe. This is mostly historical, as pickaxe predates the\ntextconv feature. If the user has bothered to define a\ntextconv filter, it is more likely that their search string will be\non the textconv output, as that is what they will see in the\ndiff (and we do not even provide a mechanism for them to\nsearch for binary needles that contain NUL characters).\n\nThis patch teaches \"-S\" to use textconv, just as we\nalready do for \"-G\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diffcore-pickaxe.c       | 56 +++++++++++++++++++++++++++++++++---------------\n t/t4030-diff-textconv.sh | 12 +++++++++++\n 2 files changed, 51 insertions(+), 17 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 61f628c..b097fa7 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -157,17 +157,15 @@ static unsigned int contains(struct diff_filespec *one, struct diff_options *o,\n \treturn;\n }\n \n-static unsigned int contains(struct diff_filespec *one, struct diff_options *o,\n+static unsigned int contains(mmfile_t *mf, struct diff_options *o,\n \t\t\t     regex_t *regexp, kwset_t kws)\n {\n \tunsigned int cnt;\n \tunsigned long sz;\n \tconst char *data;\n-\tif (diff_populate_filespec(one, 0))\n-\t\treturn 0;\n \n-\tsz = one->size;\n-\tdata = one->data;\n+\tsz = mf->size;\n+\tdata = mf->ptr;\n \tcnt = 0;\n \n \tif (regexp) {\n@@ -197,29 +195,53 @@ static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \t\t\tcnt++;\n \t\t}\n \t}\n-\tdiff_free_filespec_data(one);\n \treturn cnt;\n }\n \n static int has_changes(struct diff_filepair *p, struct diff_options *o,\n \t\t       regex_t *regexp, kwset_t kws)\n {\n+\tstruct userdiff_driver *textconv_one = get_textconv(p->one);\n+\tstruct userdiff_driver *textconv_two = get_textconv(p->two);\n+\tmmfile_t mf1, mf2;\n+\tint ret;\n+\n \tif (!o->pickaxe[0])\n \t\treturn 0;\n \n-\tif (!DIFF_FILE_VALID(p->one)) {\n-\t\tif (!DIFF_FILE_VALID(p->two))\n-\t\t\treturn 0; /* ignore unmerged */\n+\t/*\n+\t * If we have an unmodified pair, we know that the count will be the\n+\t * same and don't even have to load the blobs. Unless textconv is in\n+\t * play, _and_ we are using two different textconv filters (e.g.,\n+\t * because a pair is an exact rename with different textconv attributes\n+\t * for each side, which might generate different content).\n+\t */\n+\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n+\t\treturn 0;\n+\n+\tfill_one(p->one, &mf1, &textconv_one);\n+\tfill_one(p->two, &mf2, &textconv_two);\n+\n+\tif (!mf1.ptr) {\n+\t\tif (!mf2.ptr)\n+\t\t\tret = 0; /* ignore unmerged */\n \t\t/* created */\n-\t\treturn contains(p->two, o, regexp, kws) != 0;\n-\t}\n-\tif (!DIFF_FILE_VALID(p->two))\n-\t\treturn contains(p->one, o, regexp, kws) != 0;\n-\tif (!diff_unmodified_pair(p)) {\n-\t\treturn contains(p->one, o, regexp, kws) !=\n-\t\t       contains(p->two, o, regexp, kws);\n+\t\tret = contains(&mf2, o, regexp, kws) != 0;\n \t}\n-\treturn 0;\n+\telse if (!mf2.ptr) /* removed */\n+\t\tret = contains(&mf1, o, regexp, kws) != 0;\n+\telse\n+\t\tret = contains(&mf1, o, regexp, kws) !=\n+\t\t      contains(&mf2, o, regexp, kws);\n+\n+\tif (textconv_one)\n+\t\tfree(mf1.ptr);\n+\tif (textconv_two)\n+\t\tfree(mf2.ptr);\n+\tdiff_free_filespec_data(p->one);\n+\tdiff_free_filespec_data(p->two);\n+\n+\treturn ret;\n }\n \n static void diffcore_pickaxe_count(struct diff_options *o)\ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 461d27a..53ec330 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -96,6 +96,18 @@ test_expect_success 'grep-diff (-G) operates on textconv data (modification)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pickaxe (-S) operates on textconv data (add)' '\n+\techo one >expect &&\n+\tgit log --root --format=%s -S0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pickaxe (-S) operates on textconv data (modification)' '\n+\techo two >expect &&\n+\tgit log --root --format=%s -S1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >expect.stat <<'EOF'\n  file | Bin 2 -> 4 bytes\n  1 file changed, 0 insertions(+), 0 deletions(-)\n-- \n1.8.0.3.g3456896\n"},{"id":"202060","messageId":"508D8DF7.7040007@arcor.de","threadId":"31958","inReplyTo":"20121028120104.GE11434@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2012-10-28T19:56:39Z","receivedAt":"2012-10-28T19:56:39Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"On 2012-10-28 13:01, Jeff King wrote:\n> On Sat, Oct 27, 2012 at 08:37:24PM +0200, Peter Oberndorfer wrote:\n>\n>> It seems \"git diff-tree -Ganything <tree>\" crashes[1] with a null\n>> pointer dereference\n>> when run on a commit that adds a file (pdf) with a textconv filter.\n>>\n>> It can be reproduced with vanilla git by having a commit on top that\n>> adds a file with a textconv filter and executing git diff-tree\n>> -Ganything HEAD\n>> But running git log -Ganything still works without a crash.\n>> This problem seems to exist since the feature was first added in f506b8e8b5.\n> Thanks for a thorough bug report. I didn't reproduce the crash, but I\n> can see how it happens (it happens with diff-tree because we will reuse\n> the working tree file in that instance; it could also happen if you\n> turned on textconv caching).\n>\n>> While testing I also noticed the -S and -G act on the original file\n>> instead of the textconv munged data.\n>> Is this intentional or caused by accessing the wrong data?\n> Both, perhaps. :)\nHi,\nthanks for your patch for this!\n\n>\n> -G operates on the munged data; you can see it feed the munged data to\n> xdiff in diff_grep. But the optimization for handling added and removed\n> files accidentally fed the wrong pointer. Fixing that is a no-brainer,\n> since the optimization is inconsistent with the regular code path.\n>\n> -S, however, predates the invention of textconv and has never used it.\n> It is a little less clear that textconv is the right thing here, because\n> it is not about grepping the diff, but about counting occurrences of the\n> string inside the file content. I tend to think that doing so on the\n> textconv'd data would be what people generally want, but it is a\n> behavior change.\n>\n>> Wild guess: should we really access p->one->data and not mf1.ptr?\n> Precisely. Thanks for your wild guess; it made finding the bug very\n> easy. :)\n>\n>> Is there some more information i should provide?\n> The patch below should fix it. I added tests, but please try your\n> real-world test case on it to double-check.\n\nI tested your patch, but now it crashes for another reason :-)\ni have a file with exactly 12288(0x3000) bytes in the repository.\nWhen the file is loaded, the data is placed luckily so the data end\nfalls at a page boundary.\nLater diff_grep() calls regexec() which calls strlen() on the loaded buffer\nand ends up reading beyond the actual data into the next page\nwhich is not allocated and causes a pagefault.\nOr it could possibly (randomly) match the regex on data that is not\nactually part of a file...\nDifferent memory allocation rules on Windows probably also have some\ninfluence here.\n\nMy guess is that diff_filespec->data is not supposed to be zero terminated\nand we should not invoke strlen() on a not zero terminated data.\nBut this should be decided by somebody who knows the rules.\n\nGreetings Peter\n"},{"id":"202088","messageId":"20121029060524.GB4457@sigill.intra.peff.net","threadId":"31958","inReplyTo":"508D8DF7.7040007@arcor.de","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-29T06:05:24Z","receivedAt":"2012-10-29T06:05:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 28, 2012 at 08:56:39PM +0100, Peter Oberndorfer wrote:\n\n> > The patch below should fix it. I added tests, but please try your\n> > real-world test case on it to double-check.\n> \n> I tested your patch, but now it crashes for another reason :-)\n\nWell, that's progress, right? :)\n\n> i have a file with exactly 12288(0x3000) bytes in the repository.\n> When the file is loaded, the data is placed luckily so the data end\n> falls at a page boundary.\n> Later diff_grep() calls regexec() which calls strlen() on the loaded buffer\n> and ends up reading beyond the actual data into the next page\n> which is not allocated and causes a pagefault.\n> Or it could possibly (randomly) match the regex on data that is not\n> actually part of a file...\n\nYuck. For the most part, we treat blob content (and generally most\nobject content) as a sized buffer. However, there are some spots which,\neither through laziness or because a code interface expects a string, we\npass the value as a string. This works because the object-reading code\nputs an extra NUL at the end of our buffer to handle just such an\ninstance. So we might prematurely end if the object contains embedded\nNULs, but we would never read past the end.\n\nThe code to read the output of a textconv filter does not do this\nexplicitly. I would think it would get it for free by virtue of reading\ninto a strbuf, though. I'll try to investigate.\n\n-Peff\n"},{"id":"202089","messageId":"20121029061854.GA5102@sigill.intra.peff.net","threadId":"31958","inReplyTo":"20121029060524.GB4457@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-29T06:18:54Z","receivedAt":"2012-10-29T06:18:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 29, 2012 at 02:05:24AM -0400, Jeff King wrote:\n\n> > i have a file with exactly 12288(0x3000) bytes in the repository.\n> > When the file is loaded, the data is placed luckily so the data end\n> > falls at a page boundary.\n> > Later diff_grep() calls regexec() which calls strlen() on the loaded buffer\n> > and ends up reading beyond the actual data into the next page\n> > which is not allocated and causes a pagefault.\n> > Or it could possibly (randomly) match the regex on data that is not\n> > actually part of a file...\n> \n> Yuck. For the most part, we treat blob content (and generally most\n> object content) as a sized buffer. However, there are some spots which,\n> either through laziness or because a code interface expects a string, we\n> pass the value as a string. This works because the object-reading code\n> puts an extra NUL at the end of our buffer to handle just such an\n> instance. So we might prematurely end if the object contains embedded\n> NULs, but we would never read past the end.\n> \n> The code to read the output of a textconv filter does not do this\n> explicitly. I would think it would get it for free by virtue of reading\n> into a strbuf, though. I'll try to investigate.\n\nI can't seem to replicate the problem here, even under valgrind. Do you\nhave a minimal test case?\n\n-Peff\n"},{"id":"202136","messageId":"508EE4E4.1080407@arcor.de","threadId":"31958","inReplyTo":"20121029060524.GB4457@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2012-10-29T20:19:48Z","receivedAt":"2012-10-29T20:19:48Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"On 2012-10-29 07:05, Jeff King wrote:\n> On Sun, Oct 28, 2012 at 08:56:39PM +0100, Peter Oberndorfer wrote:\n>\n>>> The patch below should fix it. I added tests, but please try your\n>>> real-world test case on it to double-check.\n>> I tested your patch, but now it crashes for another reason :-)\n> Well, that's progress, right? :)\nSure :-)\n>\n>> i have a file with exactly 12288(0x3000) bytes in the repository.\n>> When the file is loaded, the data is placed luckily so the data end\n>> falls at a page boundary.\n>> Later diff_grep() calls regexec() which calls strlen() on the loaded buffer\n>> and ends up reading beyond the actual data into the next page\n>> which is not allocated and causes a pagefault.\n>> Or it could possibly (randomly) match the regex on data that is not\n>> actually part of a file...\n> Yuck. For the most part, we treat blob content (and generally most\n> object content) as a sized buffer. However, there are some spots which,\n> either through laziness or because a code interface expects a string, we\n> pass the value as a string. This works because the object-reading code\n> puts an extra NUL at the end of our buffer to handle just such an\n> instance. So we might prematurely end if the object contains embedded\n> NULs, but we would never read past the end.\n>\n> The code to read the output of a textconv filter does not do this\n> explicitly. I would think it would get it for free by virtue of reading\n> into a strbuf, though. I'll try to investigate.\nI could reproduce with my 0x3000 bytes file on linux. The buffer is not\nread with a trailing null byte it is mapped by mmap in\ndiff_populate_filespec...\nSo i think we will not get away with expecting a trailing null :-/\n\nFor me the key to reproduce the problem was to have 2 commits.\nAdding the file in the root commit it did not work. [1]\n\nGreetings Peter\n> -Peff\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n\n[1]\nkumbayo@home:~/src$ mkdir git_mmap_crash2\nkumbayo@home:~/src$ cd git_mmap_crash2\nkumbayo@home:~/src/git_mmap_crash2$ git init\nkumbayo@home:~/src/git_mmap_crash2$ echo blah>blah\nkumbayo@home:~/src/git_mmap_crash2$ git add blah\nkumbayo@home:~/src/git_mmap_crash2$ git commit -m blah\n[master (Basis-Version) 3458422] blah\ndiff_populate_filespec -> xmmap for blah size:0x5 returned: 0xb7206000\n 1 file changed, 1 insertion(+)\n create mode 100644 blah\nkumbayo@home:~/src/git_mmap_crash2$ perl -e 'print \"-\" x 0x3000 '> asdf.txt\nkumbayo@home:~/src/git_mmap_crash2$ git add asdf.txt\nkumbayo@home:~/src/git_mmap_crash2$ git commit -m crashy\n[master 5cf2c5f] crashy\ndiff_populate_filespec -> xmmap for asdf.txt size:0x3000 returned:\n0xb771e000\n 1 file changed, 1 insertion(+)\n create mode 100644 asdf.txt\n\nkumbayo@soybean:~/src/git_mmap_crash2$ valgrind git diff-tree -Ganything HEAD\n==8388== Memcheck, a memory error detector\n==8388== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.\n==8388== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info\n==8388== Command: git diff-tree -Ganything HEAD\n==8388==\n==8388== Conditional jump or move depends on uninitialised value(s)\n==8388==    at 0x405ADD8: inflateReset2 (in /lib/i386-linux-gnu/libz.so.1.2.3.4)\n==8388==    by 0xA0: ???\n==8388==\n==8388== Conditional jump or move depends on uninitialised value(s)\n==8388==    at 0x405ADD8: inflateReset2 (in /lib/i386-linux-gnu/libz.so.1.2.3.4)\n==8388==    by 0x7F: ???\n==8388==\n\n\n==8388== Conditional jump or move depends on uninitialised value(s)\n\n\n==8388==    at 0x405ADD8: inflateReset2 (in /lib/i386-linux-gnu/libz.so.1.2.3.4)\n\n\n==8388==    by 0x30: ???\n\n\n==8388==\n\n\n==8388== Conditional jump or move depends on uninitialised value(s)\n\n\n==8388==    at 0x405ADD8: inflateReset2 (in /lib/i386-linux-gnu/libz.so.1.2.3.4)\n\n\n==8388==    by 0x50: ???\n\n\n==8388==\n\n\ndiffcore_pickaxe_grep\n\n\ndiff_populate_filespec -> xmmap for asdf.txt size:0x3000 returned: 0x4035000\n\n\n==8388== Invalid read of size 1\n\n\n==8388==    at 0x402C683: __GI_strlen (in\n/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)\n\n\n==8388==    by 0x430581F: regexec@@GLIBC_2.3.4 (regexec.c:245)\n\n\n==8388==    by 0x814489D: diff_grep (diffcore-pickaxe.c:110)\n==8388==    by 0x8144B89: pickaxe.constprop.6 (diffcore-pickaxe.c:40)\n==8388==    by 0x8144DCD: diffcore_pickaxe_grep (diffcore-pickaxe.c:155)\n==8388==    by 0x80DCE64: diffcore_std (diff.c:4638)\n==8388==    by 0x80F0B20: log_tree_diff_flush (log-tree.c:696)\n==8388==  Address 0x4038000 is not stack'd, malloc'd or (recently) free'd\n==8388==\n==8388==\n==8388== Process terminating with default action of signal 11 (SIGSEGV)\n==8388==  Access not within mapped region at address 0x4038000\n==8388==    at 0x402C683: __GI_strlen (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)\n==8388==    by 0x430581F: regexec@@GLIBC_2.3.4 (regexec.c:245)\n==8388==    by 0x814489D: diff_grep (diffcore-pickaxe.c:110)\n==8388==    by 0x8144B89: pickaxe.constprop.6 (diffcore-pickaxe.c:40)\n==8388==    by 0x8144DCD: diffcore_pickaxe_grep (diffcore-pickaxe.c:155)\n==8388==    by 0x80DCE64: diffcore_std (diff.c:4638)\n==8388==    by 0x80F0B20: log_tree_diff_flush (log-tree.c:696)\n==8388==  If you believe this happened as a result of a stack\n==8388==  overflow in your program's main thread (unlikely but\n==8388==  possible), you can try to increase the size of the\n==8388==  main thread stack using the --main-stacksize= flag.\n==8388==  The main thread stack size used in this run was 8388608.\n==8388==\n==8388== HEAP SUMMARY:\n==8388==     in use at exit: 86,229 bytes in 69 blocks\n==8388==   total heap usage: 193 allocs, 124 frees, 259,991 bytes allocated\n==8388==\n==8388== LEAK SUMMARY:\n==8388==    definitely lost: 65 bytes in 1 blocks\n==8388==    indirectly lost: 0 bytes in 0 blocks\n==8388==      possibly lost: 0 bytes in 0 blocks\n==8388==    still reachable: 86,164 bytes in 68 blocks\n==8388==         suppressed: 0 bytes in 0 blocks\n==8388== Rerun with --leak-check=full to see details of leaked memory\n==8388==\n==8388== For counts of detected and suppressed errors, rerun with: -v\n==8388== Use --track-origins=yes to see where uninitialised values come from\n==8388== ERROR SUMMARY: 7 errors from 5 contexts (suppressed: 0 from 0)\n"},{"id":"202152","messageId":"20121029223521.GJ20513@sigill.intra.peff.net","threadId":"31958","inReplyTo":"508EE4E4.1080407@arcor.de","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-29T22:35:21Z","receivedAt":"2012-10-29T22:35:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 29, 2012 at 09:19:48PM +0100, Peter Oberndorfer wrote:\n\n> I could reproduce with my 0x3000 bytes file on linux. The buffer is not\n> read with a trailing null byte it is mapped by mmap in\n> diff_populate_filespec...\n> So i think we will not get away with expecting a trailing null :-/\n\nThanks for the reproduction recipe. I was testing with \"git log\", which\ndoes not use the mmap optimization.\n\n> For me the key to reproduce the problem was to have 2 commits.\n> Adding the file in the root commit it did not work. [1]\n\nYou probably would need to pass \"--root\" for it to do the diff of the\ninitial commit.\n\nThe patch below fixes it, but it's terribly inefficient (it just detects\nthe situation and reallocates). It would be much better to disable the\nreuse_worktree_file mmap when we populate the filespec, but it is too\nlate to pass an option; we may have already populated from an earlier\ndiffcore stage.\n\nI guess if we teach the whole diff code that \"-G\" (and --pickaxe-regex)\nis brittle, we can disable the optimization from the beginning based on\nthe diff options. I'll take a look.\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex b097fa7..88d1a8f 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -80,6 +80,29 @@ static void fill_one(struct diff_filespec *one,\n \tif (DIFF_FILE_VALID(one)) {\n \t\t*textconv = get_textconv(one);\n \t\tmf->size = fill_textconv(*textconv, one, &mf->ptr);\n+\n+\t\t/*\n+\t\t * Horrible, horrible hack. If we are going to feed the result\n+\t\t * to regexec, we must make sure it is NUL-terminated, but we\n+\t\t * will not be if we have mmap'd a file and never munged it.\n+\t\t *\n+\t\t * We would do much better to turn off the reuse_worktree_file\n+\t\t * optimization in the first place, which is the sole source of\n+\t\t * these mmaps.\n+\t\t */\n+\t\tif (one->should_munmap && !*textconv) { mf->ptr =\n+\t\t\txmallocz(one->size); memcpy(mf->ptr, one->data,\n+\t\t\t\t\t\t    one->size);\n+\n+\t\t\t/*\n+\t\t\t * Attach the result to the filespec, which will\n+\t\t\t * properly free it eventually.\n+\t\t\t */\n+\t\t\tmunmap(one->data, one->size);\n+\t\t\tone->should_munmap = 0;\n+\t\t\tone->data = mf->ptr;\n+\t\t\tone->should_free = 1;\n+\t\t}\n \t} else {\n \t\tmemset(mf, 0, sizeof(*mf));\n \t}\n"},{"id":"202154","messageId":"20121029224705.GA32148@sigill.intra.peff.net","threadId":"31958","inReplyTo":"20121029223521.GJ20513@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-29T22:47:05Z","receivedAt":"2012-10-29T22:47:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 29, 2012 at 06:35:21PM -0400, Jeff King wrote:\n\n> The patch below fixes it, but it's terribly inefficient (it just detects\n> the situation and reallocates). It would be much better to disable the\n> reuse_worktree_file mmap when we populate the filespec, but it is too\n> late to pass an option; we may have already populated from an earlier\n> diffcore stage.\n> \n> I guess if we teach the whole diff code that \"-G\" (and --pickaxe-regex)\n> is brittle, we can disable the optimization from the beginning based on\n> the diff options. I'll take a look.\n\nHmm. That is problematic for two reasons.\n\n  1. The whole diff call chain will have to be modified to pass the\n     options around, so they can make it down to the\n     diff_populate_filespec level. Alternatively, we could do some kind\n     of global hack, which is ugly but would work OK in practice.\n\n  2. Reusing a working tree file is only half of the reason a filespec\n     might be mmap'd. It might also be because we are literally diffing\n     the working tree. \"-G\" was meant to be used to limit log traversal,\n     but it also works to reduce the diff output for something like \"git\n     diff HEAD^\".\n\nI really wish there were an alternate regexec interface we could use\nthat took a pointer/size pair. Bleh.\n\n-Peff\n"},{"id":"202198","messageId":"20121030121747.GA4231@sigill.intra.peff.net","threadId":"31958","inReplyTo":"20121029224705.GA32148@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-30T12:17:48Z","receivedAt":"2012-10-30T12:17:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 29, 2012 at 06:47:05PM -0400, Jeff King wrote:\n\n> On Mon, Oct 29, 2012 at 06:35:21PM -0400, Jeff King wrote:\n> \n> > The patch below fixes it, but it's terribly inefficient (it just detects\n> > the situation and reallocates). It would be much better to disable the\n> > reuse_worktree_file mmap when we populate the filespec, but it is too\n> > late to pass an option; we may have already populated from an earlier\n> > diffcore stage.\n> > \n> > I guess if we teach the whole diff code that \"-G\" (and --pickaxe-regex)\n> > is brittle, we can disable the optimization from the beginning based on\n> > the diff options. I'll take a look.\n> \n> Hmm. That is problematic for two reasons.\n> \n>   1. The whole diff call chain will have to be modified to pass the\n>      options around, so they can make it down to the\n>      diff_populate_filespec level. Alternatively, we could do some kind\n>      of global hack, which is ugly but would work OK in practice.\n> \n>   2. Reusing a working tree file is only half of the reason a filespec\n>      might be mmap'd. It might also be because we are literally diffing\n>      the working tree. \"-G\" was meant to be used to limit log traversal,\n>      but it also works to reduce the diff output for something like \"git\n>      diff HEAD^\".\n> \n> I really wish there were an alternate regexec interface we could use\n> that took a pointer/size pair. Bleh.\n\nThinking on it more, my patch, hacky thought it seems, may not be the\nworst solution. Here are the options that I see:\n\n  1. Use a regex library that does not require NUL termination. If we\n     are bound by the regular regexec interface, this is not feasible.\n     But the GNU implementation works on arbitrary-length buffers (you\n     just have to use a slightly different interface), and we already\n     carry it in compat. It would mean platforms which provide a working\n     but non-GNU regexec would have to start defining NO_REGEX.\n\n  2. Figure out a way to get one extra zero byte via mmap. If the\n     requested size does not fall on a page boundary, you get extra\n     zero-ed bytes. Unfortunately, requesting an extra byte does not\n     do what we want; you get SIGBUS accessing it.\n\n  3. Copy mmap'd data at point-of-use into a NUL-terminated buffer. That\n     way we only incur the cost when we need it.\n\n  4. Avoid mmap-ing in the first place when we are using -G or\n     --pickaxe-regex (e.g., by doing a big read()). At first glance,\n     this sounds more efficient than loading the data one way and then\n     making another copy. But mmap+memcpy, aside from the momentary\n     doubled memory requirement, is probably just as fast or faster than\n     calling read() repeatedly.\n\nI am really tempted by (1).\n\nGiven that (2) does not work, unless somebody comes up with something\nclever there, that would make (3) the next best choice.\n\n-Peff\n"},{"id":"202199","messageId":"da24b6ea-ac9b-46dd-b591-25fd4e8e6504@email.android.com","threadId":"31958","inReplyTo":"20121030121747.GA4231@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-30T12:46:01Z","receivedAt":"2012-10-30T12:46:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"(1) sounds attractive for more than one reason. In addition to avoidance of this issue, it would bring bug-to-bug compatibility across platforms.\n\n(4), if we can run grep on streaming data (tweak interface we have for checking out a large blob to the working tree), would let us work on dataset larger than fit in core. Even though it would be much more work, it might turn out to be a better option in the longer run.\n\nJeff King <peff@peff.net> wrote:\n\n>On Mon, Oct 29, 2012 at 06:47:05PM -0400, Jeff King wrote:\n>\n>> On Mon, Oct 29, 2012 at 06:35:21PM -0400, Jeff King wrote:\n>> \n>> > The patch below fixes it, but it's terribly inefficient (it just\n>detects\n>> > the situation and reallocates). It would be much better to disable\n>the\n>> > reuse_worktree_file mmap when we populate the filespec, but it is\n>too\n>> > late to pass an option; we may have already populated from an\n>earlier\n>> > diffcore stage.\n>> > \n>> > I guess if we teach the whole diff code that \"-G\" (and\n>--pickaxe-regex)\n>> > is brittle, we can disable the optimization from the beginning\n>based on\n>> > the diff options. I'll take a look.\n>> \n>> Hmm. That is problematic for two reasons.\n>> \n>>   1. The whole diff call chain will have to be modified to pass the\n>>      options around, so they can make it down to the\n>>      diff_populate_filespec level. Alternatively, we could do some\n>kind\n>>      of global hack, which is ugly but would work OK in practice.\n>> \n>>   2. Reusing a working tree file is only half of the reason a\n>filespec\n>>      might be mmap'd. It might also be because we are literally\n>diffing\n>>      the working tree. \"-G\" was meant to be used to limit log\n>traversal,\n>>      but it also works to reduce the diff output for something like\n>\"git\n>>      diff HEAD^\".\n>> \n>> I really wish there were an alternate regexec interface we could use\n>> that took a pointer/size pair. Bleh.\n>\n>Thinking on it more, my patch, hacky thought it seems, may not be the\n>worst solution. Here are the options that I see:\n>\n>  1. Use a regex library that does not require NUL termination. If we\n>     are bound by the regular regexec interface, this is not feasible.\n>     But the GNU implementation works on arbitrary-length buffers (you\n>     just have to use a slightly different interface), and we already\n>    carry it in compat. It would mean platforms which provide a working\n>     but non-GNU regexec would have to start defining NO_REGEX.\n>\n>  2. Figure out a way to get one extra zero byte via mmap. If the\n>     requested size does not fall on a page boundary, you get extra\n>     zero-ed bytes. Unfortunately, requesting an extra byte does not\n>     do what we want; you get SIGBUS accessing it.\n>\n> 3. Copy mmap'd data at point-of-use into a NUL-terminated buffer. That\n>     way we only incur the cost when we need it.\n>\n>  4. Avoid mmap-ing in the first place when we are using -G or\n>     --pickaxe-regex (e.g., by doing a big read()). At first glance,\n>     this sounds more efficient than loading the data one way and then\n>     making another copy. But mmap+memcpy, aside from the momentary\n>    doubled memory requirement, is probably just as fast or faster than\n>     calling read() repeatedly.\n>\n>I am really tempted by (1).\n>\n>Given that (2) does not work, unless somebody comes up with something\n>clever there, that would make (3) the next best choice.\n>\n>-Peff\n"},{"id":"202200","messageId":"20121030131221.GA19571@sigill.intra.peff.net","threadId":"31958","inReplyTo":"da24b6ea-ac9b-46dd-b591-25fd4e8e6504@email.android.com","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-30T13:12:22Z","receivedAt":"2012-10-30T13:12:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 30, 2012 at 09:46:01PM +0900, Junio C Hamano wrote:\n\n> (1) sounds attractive for more than one reason. In addition to\n> avoidance of this issue, it would bring bug-to-bug compatibility\n> across platforms.\n\nYeah. I mentioned breaking the build for people who would now need to\nturn on NO_REGEX. But the only reason to do that is to let people on\nglibc systems use the system version of the tools. A much saner approach\nwould be to just always build with our compat regex, and turn NO_REGEX\ninto a no-op. We already do the same thing for kwset.\n\n> (4), if we can run grep on streaming data (tweak interface we have for\n> checking out a large blob to the working tree), would let us work on\n> dataset larger than fit in core. Even though it would be much more\n> work, it might turn out to be a better option in the longer run.\n\nAgreed, that would be nice. It's potentially a lot of work, but we could\nprobably get by with a special streaming version of diff_populate_filespec.\n\nThe tricky thing is that we have to run the regex matcher progressively\nas we stream data in (since your match might fall in the middle of a\nread boundary). Which is certainly going to require switching off of\nregular regexec. I don't think glibc regex will handle it either,\nthough. It looks like pcre can report a partial match at the end of the\nstring, and you can either continue with the next chunk (if using\npcre_dfa) or append and re-start the pattern match (for regular\npcre_exec).\n\nWhich means we'd probably have to make streaming matches an optional\nfeature, and still do (1) first to fix the correctness issue.\n\n-Peff\n"},{"id":"202375","messageId":"5092CB40.3090707@ramsay1.demon.co.uk","threadId":"31958","inReplyTo":"20121030121747.GA4231@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-11-01T19:19:28Z","receivedAt":"2012-11-01T19:19:28Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> Thinking on it more, my patch, hacky thought it seems, may not be the\n> worst solution. Here are the options that I see:\n> \n>   1. Use a regex library that does not require NUL termination. If we\n>      are bound by the regular regexec interface, this is not feasible.\n>      But the GNU implementation works on arbitrary-length buffers (you\n>      just have to use a slightly different interface), and we already\n>      carry it in compat. It would mean platforms which provide a working\n>      but non-GNU regexec would have to start defining NO_REGEX.\n\nI have thought about the possibility of doing this for unrelated reasons\nin the past.\n\nOn cygwin, there have been two unexpected test passes since about v1.6.0\n(I reported it to the list in passing), like so:\n\n    [ ... ]\n    All tests successful.\n\n    Test Summary Report\n    -------------------\n    t0050-filesystem.sh                              (Wstat: 0 Tests: 9 Failed: 0)\n      TODO passed:   5\n    t7008-grep-binary.sh                             (Wstat: 0 Tests: 20 Failed: 0)\n      TODO passed:   12\n    Files=604, Tests=8439, 11190 wallclock secs ( 2.59 usr  1.59 sys + 7294.86 cusr\n    3416.65 csys = 10715.70 CPU)\n    Result: PASS\n\nIn particular, t7008.12 passes on cygwin because the regex library apparently\nmatches '.' to NUL. Indeed if you add a test_pause to the script and execute\n\"grep .fi a\" (note grep *not* git-grep) then \"Binary file a matches\" on Linux,\ncygwin and MinGW. (So I assume the test was added to document a difference in\nbehaviour to GNU grep).\n\nSo, if we use the GNU interface to the regex routines in compat, then we may\nspecify the \"grep syntax\" for use in git-grep. (Well that's the theory, I've\nnot actually tried to code it up, so take this with a pinch of salt! :-P ).\n\nATB,\nRamsay Jones\n"},{"id":"202621","messageId":"509ACE63.9070007@arcor.de","threadId":"31958","inReplyTo":"20121029223521.GJ20513@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2012-11-07T21:10:59Z","receivedAt":"2012-11-07T21:10:59Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"On 2012-10-29 23:35, Jeff King wrote:\n> On Mon, Oct 29, 2012 at 09:19:48PM +0100, Peter Oberndorfer wrote:\n>\n>> I could reproduce with my 0x3000 bytes file on linux. The buffer is not\n>> read with a trailing null byte it is mapped by mmap in\n>> diff_populate_filespec...\n>> So i think we will not get away with expecting a trailing null :-/\n> Thanks for the reproduction recipe. I was testing with \"git log\", which\n> does not use the mmap optimization.\n>\n>> For me the key to reproduce the problem was to have 2 commits.\n>> Adding the file in the root commit it did not work. [1]\n> You probably would need to pass \"--root\" for it to do the diff of the\n> initial commit.\n>\n> The patch below fixes it, but it's terribly inefficient (it just detects\n> the situation and reallocates). It would be much better to disable the\n> reuse_worktree_file mmap when we populate the filespec, but it is too\n> late to pass an option; we may have already populated from an earlier\n> diffcore stage.\nHi,\nI tested your patch, and i can confirm it fixes the problem for me.\n(also on my real world test in msysgit)\n\nAgain, thanks a lot!\nGreetings Peter\n\n> I guess if we teach the whole diff code that \"-G\" (and --pickaxe-regex)\n> is brittle, we can disable the optimization from the beginning based on\n> the diff options. I'll take a look.\n>\n> <snip patch>\n"},{"id":"202622","messageId":"20121107211339.GA29184@sigill.intra.peff.net","threadId":"31958","inReplyTo":"509ACE63.9070007@arcor.de","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-07T21:13:39Z","receivedAt":"2012-11-07T21:13:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 07, 2012 at 10:10:59PM +0100, Peter Oberndorfer wrote:\n\n> >> For me the key to reproduce the problem was to have 2 commits.\n> >> Adding the file in the root commit it did not work. [1]\n> > You probably would need to pass \"--root\" for it to do the diff of the\n> > initial commit.\n> >\n> > The patch below fixes it, but it's terribly inefficient (it just detects\n> > the situation and reallocates). It would be much better to disable the\n> > reuse_worktree_file mmap when we populate the filespec, but it is too\n> > late to pass an option; we may have already populated from an earlier\n> > diffcore stage.\n> Hi,\n> I tested your patch, and i can confirm it fixes the problem for me.\n> (also on my real world test in msysgit)\n\nThanks for the report. I'd still like to pursue using a regex library\nthat does not require NUL-termination, but I've been distracted by other\nthings. I'm going to hold back my copy-to-a-NUL-buffer patch for now and\nsee if I can get to the regex thing this week.\n\n-Peff\n"},{"id":"203157","messageId":"7vk3tpcd0w.fsf@alter.siamese.dyndns.org","threadId":"31958","inReplyTo":"20121028124701.GB24548@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pickaxe: use textconv for -S counting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T23:13:19Z","receivedAt":"2012-11-13T23:13:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We currently just look at raw blob data when using \"-S\" to\n> pickaxe. This is mostly historical, as pickaxe predates the\n> textconv feature. If the user has bothered to define a\n> textconv filter, it is more likely that their search string will be\n> on the textconv output, as that is what they will see in the\n> diff (and we do not even provide a mechanism for them to\n> search for binary needles that contain NUL characters).\n\nOookay, I suppose...\n\n>  static int has_changes(struct diff_filepair *p, struct diff_options *o,\n>  \t\t       regex_t *regexp, kwset_t kws)\n>  {\n> +\tstruct userdiff_driver *textconv_one = get_textconv(p->one);\n> +\tstruct userdiff_driver *textconv_two = get_textconv(p->two);\n> +\tmmfile_t mf1, mf2;\n> +\tint ret;\n> +\n>  \tif (!o->pickaxe[0])\n>  \t\treturn 0;\n>  \n> -\tif (!DIFF_FILE_VALID(p->one)) {\n> -\t\tif (!DIFF_FILE_VALID(p->two))\n> -\t\t\treturn 0; /* ignore unmerged */\n\nWhat happened to this part that avoids showing nonsense for unmerged\npaths?\n\n> +\t/*\n> +\t * If we have an unmodified pair, we know that the count will be the\n> +\t * same and don't even have to load the blobs. Unless textconv is in\n> +\t * play, _and_ we are using two different textconv filters (e.g.,\n> +\t * because a pair is an exact rename with different textconv attributes\n> +\t * for each side, which might generate different content).\n> +\t */\n> +\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n> +\t\treturn 0;\n\nI am not sure about this part that cares about the textconv.\n\nWouldn't the normal \"git diff A B\" skip the filepair that are\nunmodified in the first place at the object name level without even\nlooking at the contents (see e.g. diff_flush_patch())?\n\nShouldn't this part of the code emulating that behaviour no matter\nwhat textconv filter(s) are configured for these paths?\n"},{"id":"203248","messageId":"20121115012131.GA17894@sigill.intra.peff.net","threadId":"31958","inReplyTo":"7vk3tpcd0w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] pickaxe: use textconv for -S counting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T01:21:31Z","receivedAt":"2012-11-15T01:21:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 03:13:19PM -0800, Junio C Hamano wrote:\n\n> >  static int has_changes(struct diff_filepair *p, struct diff_options *o,\n> >  \t\t       regex_t *regexp, kwset_t kws)\n> >  {\n> > +\tstruct userdiff_driver *textconv_one = get_textconv(p->one);\n> > +\tstruct userdiff_driver *textconv_two = get_textconv(p->two);\n> > +\tmmfile_t mf1, mf2;\n> > +\tint ret;\n> > +\n> >  \tif (!o->pickaxe[0])\n> >  \t\treturn 0;\n> >  \n> > -\tif (!DIFF_FILE_VALID(p->one)) {\n> > -\t\tif (!DIFF_FILE_VALID(p->two))\n> > -\t\t\treturn 0; /* ignore unmerged */\n> \n> What happened to this part that avoids showing nonsense for unmerged\n> paths?\n\nIt's moved down. fill_one will return an empty mmfile if\n!DIFF_FILE_VALID, so we end up here:\n\n        fill_one(p->one, &mf1, &textconv_one);\n        fill_one(p->two, &mf2, &textconv_two);\n\n        if (!mf1.ptr) {\n                if (!mf2.ptr)\n                        ret = 0; /* ignore unmerged */\n\nPrior to this change, we didn't use fill_one, so we had to check manually.\n\n> > +\t/*\n> > +\t * If we have an unmodified pair, we know that the count will be the\n> > +\t * same and don't even have to load the blobs. Unless textconv is in\n> > +\t * play, _and_ we are using two different textconv filters (e.g.,\n> > +\t * because a pair is an exact rename with different textconv attributes\n> > +\t * for each side, which might generate different content).\n> > +\t */\n> > +\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n> > +\t\treturn 0;\n> \n> I am not sure about this part that cares about the textconv.\n> \n> Wouldn't the normal \"git diff A B\" skip the filepair that are\n> unmodified in the first place at the object name level without even\n> looking at the contents (see e.g. diff_flush_patch())?\n\nHmph. The point was to find the case when the paths are different (e.g.,\nin a rename), and therefore the textconvs might be different. But I\nthink I missed the fact that diff_unmodified_pair will note the\ndifference in paths. So just calling diff_unmodified_pair would be\nsufficient, as the code prior to my patch does.\n\nI thought the point was an optimization to avoid comparing contains() on\nthe same data (which we can know will match without looking at it).\nExact renames are the obvious one, but they are not handled here. So I\nam not sure of the point (to catch \"git diff $blob1 $blob2\" when the two\nare identical? I am not sure at what layer we cull that from the diff\nqueue).\n\nSo there is room for optimization here on exact renames, but\ndiff_unmodified_pair is too forgiving of what is interesting (a rename\nis interesting to diff_flush_patch, because it wants to mention the\nrename, but it is not interesting to pickaxe, because we did not change\nthe content, and it could be culled here).\n\nI don't know that it is that big a deal in general. Pure renames are\ngoing to be the minority of blobs we look at, so it is probably not even\nmeasurable. You could construct a pathological case (e.g., an otherwise\nsmall repo with a 2G file, rename the 2G file without modification, then\nrunning \"git log -Sfoo\" will unnecessarily load the giant blob while\nexamining the rename commit).\n\n> Shouldn't this part of the code emulating that behaviour no matter\n> what textconv filter(s) are configured for these paths?\n\nYeah, I just missed that it is checking the path already. It may still\nmake sense to tighten the optimization, but that is a separate issue. It\nshould just check diff_unmodified_pair as before; textconv only matters\nif you are trying to optimize out exact renames.\n\n-Peff\n"},{"id":"203530","messageId":"7v3905uncf.fsf@alter.siamese.dyndns.org","threadId":"31958","inReplyTo":"20121115012131.GA17894@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pickaxe: use textconv for -S counting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-20T00:31:12Z","receivedAt":"2012-11-20T00:31:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Nov 13, 2012 at 03:13:19PM -0800, Junio C Hamano wrote:\n>\n>> >  static int has_changes(struct diff_filepair *p, struct diff_options *o,\n>> >  \t\t       regex_t *regexp, kwset_t kws)\n>> >  {\n>> > +\tstruct userdiff_driver *textconv_one = get_textconv(p->one);\n>> > +\tstruct userdiff_driver *textconv_two = get_textconv(p->two);\n>> > +\tmmfile_t mf1, mf2;\n>> > +\tint ret;\n>> > +\n>> >  \tif (!o->pickaxe[0])\n>> >  \t\treturn 0;\n>> >  \n>> > -\tif (!DIFF_FILE_VALID(p->one)) {\n>> > -\t\tif (!DIFF_FILE_VALID(p->two))\n>> > -\t\t\treturn 0; /* ignore unmerged */\n>> \n>> What happened to this part that avoids showing nonsense for unmerged\n>> paths?\n>\n> It's moved down. fill_one will return an empty mmfile if\n> !DIFF_FILE_VALID, so we end up here:\n>\n>         fill_one(p->one, &mf1, &textconv_one);\n>         fill_one(p->two, &mf2, &textconv_two);\n>\n>         if (!mf1.ptr) {\n>                 if (!mf2.ptr)\n>                         ret = 0; /* ignore unmerged */\n>\n> Prior to this change, we didn't use fill_one, so we had to check manually.\n>\n>> > +\t/*\n>> > +\t * If we have an unmodified pair, we know that the count will be the\n>> > +\t * same and don't even have to load the blobs. Unless textconv is in\n>> > +\t * play, _and_ we are using two different textconv filters (e.g.,\n>> > +\t * because a pair is an exact rename with different textconv attributes\n>> > +\t * for each side, which might generate different content).\n>> > +\t */\n>> > +\tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n>> > +\t\treturn 0;\n>> \n>> I am not sure about this part that cares about the textconv.\n>> \n>> Wouldn't the normal \"git diff A B\" skip the filepair that are\n>> unmodified in the first place at the object name level without even\n>> looking at the contents (see e.g. diff_flush_patch())?\n>\n> Hmph. The point was to find the case when the paths are different (e.g.,\n> in a rename), and therefore the textconvs might be different. But I\n> think I missed the fact that diff_unmodified_pair will note the\n> difference in paths. So just calling diff_unmodified_pair would be\n> sufficient, as the code prior to my patch does.\n>\n> I thought the point was an optimization to avoid comparing contains() on\n> the same data (which we can know will match without looking at it).\n\nYes.\n\n> Exact renames are the obvious one, but they are not handled here.\n\nThat is half true.  Before this change, we will find the same number\nof needles and this function would have said \"no differences\" in a\nvery inefficient way.  After this change, we may apply different\ntextconv filters and this function will say \"there is a difference\",\neven though we wouldn't see such a difference at the content level\nif there wasn't any rename.\n"},{"id":"203531","messageId":"7vr4npt7zd.fsf@alter.siamese.dyndns.org","threadId":"31958","inReplyTo":"7v3905uncf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] pickaxe: use textconv for -S counting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-20T00:48:22Z","receivedAt":"2012-11-20T00:48:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Exact renames are the obvious one, but they are not handled here.\n>\n> That is half true.  Before this change, we will find the same number\n> of needles and this function would have said \"no differences\" in a\n> very inefficient way.  After this change, we may apply different\n> textconv filters and this function will say \"there is a difference\",\n> even though we wouldn't see such a difference at the content level\n> if there wasn't any rename.\n\n... but I think that is a good thing anyway.\n\nIf you renamed foo.c to foo.cc with different conversions from C\ncode to the text that explain what the code does, if we special case\nonly the exact rename case but let pickaxe examine the converted\nresult in a case where blobs are modified only by one byte, we would\nget drastically different results between the two cases.\n\nCorollary to this is what should happen when you update the attributes\nbetween two trees so that textconv for a path that did not change\nbetween preimage and postimage are different.  Ideally, we should\nnotice that the two converted result are different, perhaps, but I\ndo not like the performance implications very much.\n"},{"id":"203627","messageId":"20121121202704.GH16280@sigill.intra.peff.net","threadId":"31958","inReplyTo":"7vr4npt7zd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] pickaxe: use textconv for -S counting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-21T20:27:05Z","receivedAt":"2012-11-21T20:27:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 19, 2012 at 04:48:22PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >> Exact renames are the obvious one, but they are not handled here.\n> >\n> > That is half true.  Before this change, we will find the same number\n> > of needles and this function would have said \"no differences\" in a\n> > very inefficient way.  After this change, we may apply different\n> > textconv filters and this function will say \"there is a difference\",\n> > even though we wouldn't see such a difference at the content level\n> > if there wasn't any rename.\n> \n> ... but I think that is a good thing anyway.\n> \n> If you renamed foo.c to foo.cc with different conversions from C\n> code to the text that explain what the code does, if we special case\n> only the exact rename case but let pickaxe examine the converted\n> result in a case where blobs are modified only by one byte, we would\n> get drastically different results between the two cases.\n\nRight, exactly. I think the only sane thing is to always textconv or\nalways not textconv (whether they are identical renames or not), and any\n\"these are the same\" optimization for identical content needs to take\ninto account whether we _would have_ done a different textconv (which\nmost of the time is going to be \"no\", as textconv is either not in use,\nor both paths use the same diff driver; but it is not too expensive to\nlook up).\n\nThe diff_unmodified_pair at the top off diff_flush_patch is correct,\nbecause it treats renames as interesting (because we have to show the\ndiff header, anyway). I do not know offhand if we avoid feeding\nidentical content to xdiff at all, but if so, we should be doing so only\nafter checking that the textconv filters are identical.\n\n> Corollary to this is what should happen when you update the attributes\n> between two trees so that textconv for a path that did not change\n> between preimage and postimage are different.  Ideally, we should\n> notice that the two converted result are different, perhaps, but I\n> do not like the performance implications very much.\n\nThe content to compare cannot be different unless either the input\ncontent changed or the path changed, and we treat either as\n\"interesting\" in most code paths. So I do not think there are any\nperformance implications, except that we may need to make sure to look\nup textconvs a few lines sooner in some cases.\n\nI'll re-roll the series next week and break out the rename-optimization\nbits separately so it is more obvious that it is doing the right thing.\n\nAs an aside, I also need to revisit the regex half of that code, which\nis still buggy (before and after my patch, due to the expecting-a-NUL\nbehavior we talked about a week or two ago).  That is a separate topic,\nbut the same area of code.\n\n-Peff\n"},{"id":"219252","messageId":"51ACD172.4070608@arcor.de","threadId":"31958","inReplyTo":"20121107211339.GA29184@sigill.intra.peff.net","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2013-06-03T17:25:06Z","receivedAt":"2013-06-03T17:25:06Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"On 2012-11-07 22:13, Jeff King wrote:\n> On Wed, Nov 07, 2012 at 10:10:59PM +0100, Peter Oberndorfer wrote:\n>\n>>>> For me the key to reproduce the problem was to have 2 commits.\n>>>> Adding the file in the root commit it did not work. [1]\n>>> You probably would need to pass \"--root\" for it to do the diff of the\n>>> initial commit.\n>>>\n>>> The patch below fixes it, but it's terribly inefficient (it just detects\n>>> the situation and reallocates). It would be much better to disable the\n>>> reuse_worktree_file mmap when we populate the filespec, but it is too\n>>> late to pass an option; we may have already populated from an earlier\n>>> diffcore stage.\n>> Hi,\n>> I tested your patch, and i can confirm it fixes the problem for me.\n>> (also on my real world test in msysgit)\n> Thanks for the report. I'd still like to pursue using a regex library\n> that does not require NUL-termination, but I've been distracted by other\n> things. I'm going to hold back my copy-to-a-NUL-buffer patch for now and\n> see if I can get to the regex thing this week.\n>\nHi,\n\nare there any news regarding this problem?\nThe crash seems to still exist in the current version 1.8.3 and master.\n\nThanks,\nGreetings Peter\n"},{"id":"219308","messageId":"20130603221716.GC23224@sigill.intra.peff.net","threadId":"31958","inReplyTo":"51ACD172.4070608@arcor.de","subject":"Re: crash on git diff-tree -Ganything <tree> for new files with textconv filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-03T22:17:16Z","receivedAt":"2013-06-03T22:17:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 03, 2013 at 07:25:06PM +0200, Peter Oberndorfer wrote:\n\n> > Thanks for the report. I'd still like to pursue using a regex library\n> > that does not require NUL-termination, but I've been distracted by other\n> > things. I'm going to hold back my copy-to-a-NUL-buffer patch for now and\n> > see if I can get to the regex thing this week.\n> >\n> are there any news regarding this problem?\n> The crash seems to still exist in the current version 1.8.3 and master.\n\nSorry, no, this got dropped due to lack of time. I _think_ it is as\nsimple as just tweaking the Makefile to unconditionally build against\nthe compat/ regex library, and then tweaking callsites as appropriate to\nuse the GNU-specific interface that takes buf/len instead of a\nNUL-terminated string.\n\nBut there may be some hidden complexities to it.\n\n-Peff\n"}]}