crash on git diff-tree -Ganything <tree> for new files with textconv filter

Hi,

It seems "git diff-tree -Ganything <tree>" crashes[1] with a null pointer dereference when run on a commit that adds a file (pdf) with a textconv filter.

It can be reproduced with vanilla git by having a commit on top that adds a file with a textconv filter and executing git diff-tree -Ganything HEAD But running git log -Ganything still works without a crash. This problem seems to exist since the feature was first added in f506b8e8b5.

While testing I also noticed the -S and -G act on the original file instead of the textconv munged data. Is this intentional or caused by accessing the wrong data? Wild guess: should we really access p->one->data and not mf1.ptr?

Is there some more information i should provide?

Greetings Peter

[1] I am running msysgit v1.8.0.msysgit.0 (52d3a7583a) and i tried added -G pickaxe support to gitk. gitk runs git diff-tree -r -s --stdin -Gpattern

This is how i detected the crash the first time. (but only because of a crash popup on Windows, gitk does not complain...)

For testing on vanilla git I used .git/config:
[diff "upcase2"]
    textconv = tr a-z A-Z <

.gitatrributes: newtext diff=upcase2

Program received signal SIGSEGV, Segmentation fault.
[Switching to thread 3864.0x83c]
0x0049d90c in regexec (preg=0x22f900, string=0x0, nmatch=1,
pmatch=0x22f46c, eflags=0) at compat/regex/regexec.c:241
241           length = strlen (string);
(gdb) bt
#0  0x0049d90c in regexec (preg=0x22f900, string=0x0, nmatch=1,
pmatch=0x22f46c, eflags=0) at compat/regex/regexec.c:241
#1  0x004f5763 in diff_grep (p=0x109a530, o=0x550b48, regexp=0x22f900,
kws=0x0) at diffcore-pickaxe.c:110
#2  0x004f59dc in pickaxe (o=<value optimized out>, regexp=0x22f900,
kws=0xffffffff, fn=0x4f5620 <diff_grep>) at diffcore-pickaxe.c:40
#3  0x004f5bd4 in diffcore_pickaxe_grep (o=0x550b48) at
diffcore-pickaxe.c:154
#4  0x0048049a in diffcore_std (options=0x550b48) at diff.c:4630
#5  0x004dc16a in log_tree_diff_flush (opt=0x5508c0) at log-tree.c:697
#6  0x004dc32e in log_tree_commit (opt=0x5508c0, commit=0xffc620) at
log-tree.c:790
#7  0x004206dd in cmd_diff_tree (argc=<value optimized out>,
argv=0x3d24bc, prefix=0x0) at builtin/diff-tree.c:43
#8  0x00401a16 in handle_internal_command (argc=<value optimized out>,
argv=<value optimized out>) at git.c:306
#9  0x00401c00 in main (argc=6, argv=0x3d24b8) at git.c:513
(gdb) up
#1  0x004f5763 in diff_grep (p=0x109a530, o=0x550b48, regexp=0x22f900,
kws=0x0) at diffcore-pickaxe.c:110
110                     hit = !regexec(regexp, p->one->data, 1,
&regmatch, 0);
(gdb) info locals
regmatch = {rm_so = 17408640, rm_eo = 2291968}
textconv_one = (struct userdiff_driver *) 0x0
textconv_two = (struct userdiff_driver *) 0xe10468
mf1 = {ptr = 0x0, size = 0}
mf2 = {
  ptr = 0x2bbcaf0 ' ' <repeats 52 times>, "PROJECT
DESCRIPTION\r\n\r\n\r\nProject Number:", ' ' <repeats 19 times>,
"xxxxx\r\nProject Description:", ' ' <repeats 14 times>,
"xxxxxxxx\r\nxxxxx:", ' ' <repeats 11 times>..., size = 64185}
hit = <value optimized out>
(gdb) print p
$1 = (struct diff_filepair *) 0x109a530
(gdb) print *p
$2 = {one = 0x109a320, two = 0x109a428, score = 0, status = 0 '\0',
broken_pair = 0, renamed_pair = 0, is_unmerged = 0}
(gdb) print p->one
$3 = (struct diff_filespec *) 0x109a320
(gdb) print *p->one
$4 = {sha1 = '\0' <repeats 19 times>, path = 0x109a360
"xxxxxxxx/xxx/doc/xxxx xxxxx 1.4.pdf", data = 0x0, cnt_data = 0x0,
  funcname_pattern_ident = 0x0, size = 0, count = 1, xfrm_flags = 0,
rename_used = 0, mode = 0, sha1_valid = 0, should_free = 0,
should_munmap = 0,
  dirty_submodule = 0, is_stdin = 0, has_more_entries = 0, driver = 0x0,
is_binary = -1}
(gdb) print *p->two
$5 = {sha1 = "0aax\217\231)oBaAa(\021\234^'Q\236\230", path = 0x109a468
"xxxxxxxx/xxx/doc/xxxx xxxxx 1.4.pdf", data = 0x0,
  cnt_data = 0x0, funcname_pattern_ident = 0x0, size = 0, count = 1,
xfrm_flags = 0, rename_used = 0, mode = 33188, sha1_valid = 1,
should_free = 0,
  should_munmap = 0, dirty_submodule = 0, is_stdin = 0, has_more_entries
= 0, driver = 0xe10468, is_binary = -1}
(gdb) print textconv_one
$6 = (struct userdiff_driver *) 0x0
(gdb) print textconv_two
$7 = (struct userdiff_driver *) 0xe10468
(gdb) print *textconv_two
$10 = {name = 0xfe4fc0 "astextplain", external = 0x0, binary = -1,
funcname = {pattern = 0x0, cflags = 0}, word_regex = 0x0,
  textconv = 0xfe4fd8 "astextplain", textconv_cache = 0x0,
textconv_want_cache = 0}
(gdb)

On Sat, Oct 27, 2012 at 08:37:24PM +0200, Peter Oberndorfer wrote:

Thanks for a thorough bug report. I didn't reproduce the crash, but I can see how it happens (it happens with diff-tree because we will reuse the working tree file in that instance; it could also happen if you turned on textconv caching).

Both, perhaps. :)

-G operates on the munged data; you can see it feed the munged data to xdiff in diff_grep. But the optimization for handling added and removed files accidentally fed the wrong pointer. Fixing that is a no-brainer, since the optimization is inconsistent with the regular code path.

-S, however, predates the invention of textconv and has never used it. It is a little less clear that textconv is the right thing here, because it is not about grepping the diff, but about counting occurrences of the string inside the file content. I tend to think that doing so on the textconv'd data would be what people generally want, but it is a behavior change.

Precisely. Thanks for your wild guess; it made finding the bug very easy. :)

The patch below should fix it. I added tests, but please try your real-world test case on it to double-check.

-- >8 --
Subject: [PATCH] diff_grep: use textconv buffers for add/deleted files

If you use "-G" to grep a diff, we will apply a configured textconv filter to the data before generating the diff. However, if the diff is an addition or deletion, we do not bother running the diff at all, and just look for the token in the added (or removed) content. This works because we know that the diff must contain every line of content.

However, while we used the textconv-derived buffers in the regular diff, we accidentally passed the original unmodified buffers to regexec when checking the added or removed content. This could lead to an incorrect answer.

Worse, in some cases we might have a textconv buffer but no original buffer (e.g., if we pulled the textconv data from cache, or if we reused a working tree file when generating it). In that case, we could actually feed NULL to regexec and segfault.

Reported-by: Peter Oberndorfer <kumbayo84@arcor.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 diffcore-pickaxe.c       |  4 ++--
 t/t4030-diff-textconv.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

On Sun, Oct 28, 2012 at 08:01:04AM -0400, Jeff King wrote:

I prepared the earlier bugfix for "-G" for maint. Modifying "-S" would be a separate feature topic, and would look like this (I built it on top of the bugfix patch, since the tests are a follow-on).

  [1/2]: pickaxe: hoist empty needle check
  [2/2]: pickaxe: use textconv for -S counting

-Peff

If we are given an empty pickaxe needle like "git log -S ''", it is impossible for us to find anything (because no matter what the content, the count will always be 0). We currently check this at the lowest level of contains(). Let's hoist the logic much earlier to has_changes(), so that it is simpler to return our answer before loading any blob data.

Signed-off-by: Jeff King <peff@peff.net>
---
 diffcore-pickaxe.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

We currently just look at raw blob data when using "-S" to pickaxe. This is mostly historical, as pickaxe predates the textconv feature. If the user has bothered to define a textconv filter, it is more likely that their search string will be on the textconv output, as that is what they will see in the diff (and we do not even provide a mechanism for them to search for binary needles that contain NUL characters).

This patch teaches "-S" to use textconv, just as we already do for "-G".

Signed-off-by: Jeff King <peff@peff.net>
---
 diffcore-pickaxe.c       | 56 +++++++++++++++++++++++++++++++++---------------
 t/t4030-diff-textconv.sh | 12 +++++++++++
 2 files changed, 51 insertions(+), 17 deletions(-)

Jeff King <peff@peff.net> writes:

Oookay, I suppose...

What happened to this part that avoids showing nonsense for unmerged paths?

I am not sure about this part that cares about the textconv.

Wouldn't the normal "git diff A B" skip the filepair that are unmodified in the first place at the object name level without even looking at the contents (see e.g. diff_flush_patch())?

Shouldn't this part of the code emulating that behaviour no matter what textconv filter(s) are configured for these paths?

On Tue, Nov 13, 2012 at 03:13:19PM -0800, Junio C Hamano wrote:

It's moved down. fill_one will return an empty mmfile if !DIFF_FILE_VALID, so we end up here:

        fill_one(p->one, &mf1, &textconv_one);
        fill_one(p->two, &mf2, &textconv_two);

        if (!mf1.ptr) {
                if (!mf2.ptr)
                        ret = 0; /* ignore unmerged */

Prior to this change, we didn't use fill_one, so we had to check manually.

Hmph. The point was to find the case when the paths are different (e.g., in a rename), and therefore the textconvs might be different. But I think I missed the fact that diff_unmodified_pair will note the difference in paths. So just calling diff_unmodified_pair would be sufficient, as the code prior to my patch does.

I thought the point was an optimization to avoid comparing contains() on the same data (which we can know will match without looking at it). Exact renames are the obvious one, but they are not handled here. So I am not sure of the point (to catch "git diff $blob1 $blob2" when the two are identical? I am not sure at what layer we cull that from the diff queue).

So there is room for optimization here on exact renames, but diff_unmodified_pair is too forgiving of what is interesting (a rename is interesting to diff_flush_patch, because it wants to mention the rename, but it is not interesting to pickaxe, because we did not change the content, and it could be culled here).

I don't know that it is that big a deal in general. Pure renames are going to be the minority of blobs we look at, so it is probably not even measurable. You could construct a pathological case (e.g., an otherwise small repo with a 2G file, rename the 2G file without modification, then running "git log -Sfoo" will unnecessarily load the giant blob while examining the rename commit).

Yeah, I just missed that it is checking the path already. It may still make sense to tighten the optimization, but that is a separate issue. It should just check diff_unmodified_pair as before; textconv only matters if you are trying to optimize out exact renames.

-Peff

Jeff King <peff@peff.net> writes:

Yes.

That is half true. Before this change, we will find the same number of needles and this function would have said "no differences" in a very inefficient way. After this change, we may apply different textconv filters and this function will say "there is a difference", even though we wouldn't see such a difference at the content level if there wasn't any rename.

Junio C Hamano <gitster@pobox.com> writes:

... but I think that is a good thing anyway.

If you renamed foo.c to foo.cc with different conversions from C code to the text that explain what the code does, if we special case only the exact rename case but let pickaxe examine the converted result in a case where blobs are modified only by one byte, we would get drastically different results between the two cases.

Corollary to this is what should happen when you update the attributes between two trees so that textconv for a path that did not change between preimage and postimage are different. Ideally, we should notice that the two converted result are different, perhaps, but I do not like the performance implications very much.

On Mon, Nov 19, 2012 at 04:48:22PM -0800, Junio C Hamano wrote:

Right, exactly. I think the only sane thing is to always textconv or always not textconv (whether they are identical renames or not), and any "these are the same" optimization for identical content needs to take into account whether we _would have_ done a different textconv (which most of the time is going to be "no", as textconv is either not in use, or both paths use the same diff driver; but it is not too expensive to look up).

The diff_unmodified_pair at the top off diff_flush_patch is correct, because it treats renames as interesting (because we have to show the diff header, anyway). I do not know offhand if we avoid feeding identical content to xdiff at all, but if so, we should be doing so only after checking that the textconv filters are identical.

The content to compare cannot be different unless either the input content changed or the path changed, and we treat either as "interesting" in most code paths. So I do not think there are any performance implications, except that we may need to make sure to look up textconvs a few lines sooner in some cases.

I'll re-roll the series next week and break out the rename-optimization bits separately so it is more obvious that it is doing the right thing.

As an aside, I also need to revisit the regex half of that code, which is still buggy (before and after my patch, due to the expecting-a-NUL behavior we talked about a week or two ago). That is a separate topic, but the same area of code.

-Peff

On 2012-10-28 13:01, Jeff King wrote:

Hi, thanks for your patch for this!

I tested your patch, but now it crashes for another reason :-) i have a file with exactly 12288(0x3000) bytes in the repository. When the file is loaded, the data is placed luckily so the data end falls at a page boundary. Later diff_grep() calls regexec() which calls strlen() on the loaded buffer and ends up reading beyond the actual data into the next page which is not allocated and causes a pagefault. Or it could possibly (randomly) match the regex on data that is not actually part of a file... Different memory allocation rules on Windows probably also have some influence here.

My guess is that diff_filespec->data is not supposed to be zero terminated and we should not invoke strlen() on a not zero terminated data. But this should be decided by somebody who knows the rules.

Greetings Peter

On Sun, Oct 28, 2012 at 08:56:39PM +0100, Peter Oberndorfer wrote:

Well, that's progress, right? :)

Yuck. For the most part, we treat blob content (and generally most object content) as a sized buffer. However, there are some spots which, either through laziness or because a code interface expects a string, we pass the value as a string. This works because the object-reading code puts an extra NUL at the end of our buffer to handle just such an instance. So we might prematurely end if the object contains embedded NULs, but we would never read past the end.

The code to read the output of a textconv filter does not do this explicitly. I would think it would get it for free by virtue of reading into a strbuf, though. I'll try to investigate.

-Peff

On Mon, Oct 29, 2012 at 02:05:24AM -0400, Jeff King wrote:

I can't seem to replicate the problem here, even under valgrind. Do you have a minimal test case?

-Peff

On 2012-10-29 07:05, Jeff King wrote:

Sure :-)

I could reproduce with my 0x3000 bytes file on linux. The buffer is not read with a trailing null byte it is mapped by mmap in diff_populate_filespec... So i think we will not get away with expecting a trailing null :-/

For me the key to reproduce the problem was to have 2 commits. Adding the file in the root commit it did not work. [1]

Greetings Peter

[1]
kumbayo@home:~/src$ mkdir git_mmap_crash2
kumbayo@home:~/src$ cd git_mmap_crash2
kumbayo@home:~/src/git_mmap_crash2$ git init
kumbayo@home:~/src/git_mmap_crash2$ echo blah>blah
kumbayo@home:~/src/git_mmap_crash2$ git add blah
kumbayo@home:~/src/git_mmap_crash2$ git commit -m blah
[master (Basis-Version) 3458422] blah
diff_populate_filespec -> xmmap for blah size:0x5 returned: 0xb7206000
 1 file changed, 1 insertion(+)
 create mode 100644 blah
kumbayo@home:~/src/git_mmap_crash2$ perl -e 'print "-" x 0x3000 '> asdf.txt
kumbayo@home:~/src/git_mmap_crash2$ git add asdf.txt
kumbayo@home:~/src/git_mmap_crash2$ git commit -m crashy
[master 5cf2c5f] crashy
diff_populate_filespec -> xmmap for asdf.txt size:0x3000 returned:
0xb771e000
 1 file changed, 1 insertion(+)
 create mode 100644 asdf.txt

kumbayo@soybean:~/src/git_mmap_crash2$ valgrind git diff-tree -Ganything HEAD ==8388== Memcheck, a memory error detector ==8388== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al. ==8388== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info ==8388== Command: git diff-tree -Ganything HEAD ==8388== ==8388== Conditional jump or move depends on uninitialised value(s) ==8388== at 0x405ADD8: inflateReset2 (in /lib/i386-linux-gnu/libz.so.1.2.3.4) ==8388== by 0xA0: ??? ==8388== ==8388== Conditional jump or move depends on uninitialised value(s) ==8388== at 0x405ADD8: inflateReset2 (in /lib/i386-linux-gnu/libz.so.1.2.3.4) ==8388== by 0x7F: ??? ==8388==

==8388== Conditional jump or move depends on uninitialised value(s)

==8388==    at 0x405ADD8: inflateReset2 (in /lib/i386-linux-gnu/libz.so.1.2.3.4)

==8388==    by 0x30: ???

==8388==

==8388== Conditional jump or move depends on uninitialised value(s)

==8388==    at 0x405ADD8: inflateReset2 (in /lib/i386-linux-gnu/libz.so.1.2.3.4)

==8388==    by 0x50: ???

==8388==

diffcore_pickaxe_grep

diff_populate_filespec -> xmmap for asdf.txt size:0x3000 returned: 0x4035000

==8388== Invalid read of size 1

==8388== at 0x402C683: __GI_strlen (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)

==8388==    by 0x430581F: regexec@@GLIBC_2.3.4 (regexec.c:245)

==8388== by 0x814489D: diff_grep (diffcore-pickaxe.c:110) ==8388== by 0x8144B89: pickaxe.constprop.6 (diffcore-pickaxe.c:40) ==8388== by 0x8144DCD: diffcore_pickaxe_grep (diffcore-pickaxe.c:155) ==8388== by 0x80DCE64: diffcore_std (diff.c:4638) ==8388== by 0x80F0B20: log_tree_diff_flush (log-tree.c:696) ==8388== Address 0x4038000 is not stack'd, malloc'd or (recently) free'd ==8388== ==8388== ==8388== Process terminating with default action of signal 11 (SIGSEGV) ==8388== Access not within mapped region at address 0x4038000 ==8388== at 0x402C683: __GI_strlen (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==8388== by 0x430581F: regexec@@GLIBC_2.3.4 (regexec.c:245) ==8388== by 0x814489D: diff_grep (diffcore-pickaxe.c:110) ==8388== by 0x8144B89: pickaxe.constprop.6 (diffcore-pickaxe.c:40) ==8388== by 0x8144DCD: diffcore_pickaxe_grep (diffcore-pickaxe.c:155) ==8388== by 0x80DCE64: diffcore_std (diff.c:4638) ==8388== by 0x80F0B20: log_tree_diff_flush (log-tree.c:696) ==8388== If you believe this happened as a result of a stack ==8388== overflow in your program's main thread (unlikely but ==8388== possible), you can try to increase the size of the ==8388== main thread stack using the --main-stacksize= flag. ==8388== The main thread stack size used in this run was 8388608. ==8388== ==8388== HEAP SUMMARY: ==8388== in use at exit: 86,229 bytes in 69 blocks ==8388== total heap usage: 193 allocs, 124 frees, 259,991 bytes allocated ==8388== ==8388== LEAK SUMMARY: ==8388== definitely lost: 65 bytes in 1 blocks ==8388== indirectly lost: 0 bytes in 0 blocks ==8388== possibly lost: 0 bytes in 0 blocks ==8388== still reachable: 86,164 bytes in 68 blocks ==8388== suppressed: 0 bytes in 0 blocks ==8388== Rerun with --leak-check=full to see details of leaked memory ==8388== ==8388== For counts of detected and suppressed errors, rerun with: -v ==8388== Use --track-origins=yes to see where uninitialised values come from ==8388== ERROR SUMMARY: 7 errors from 5 contexts (suppressed: 0 from 0)

On Mon, Oct 29, 2012 at 09:19:48PM +0100, Peter Oberndorfer wrote:

Thanks for the reproduction recipe. I was testing with "git log", which does not use the mmap optimization.

You probably would need to pass "--root" for it to do the diff of the initial commit.

The patch below fixes it, but it's terribly inefficient (it just detects the situation and reallocates). It would be much better to disable the reuse_worktree_file mmap when we populate the filespec, but it is too late to pass an option; we may have already populated from an earlier diffcore stage.

I guess if we teach the whole diff code that "-G" (and --pickaxe-regex) is brittle, we can disable the optimization from the beginning based on the diff options. I'll take a look.

On Mon, Oct 29, 2012 at 06:35:21PM -0400, Jeff King wrote:

Hmm. That is problematic for two reasons.

  1. The whole diff call chain will have to be modified to pass the
     options around, so they can make it down to the
     diff_populate_filespec level. Alternatively, we could do some kind
     of global hack, which is ugly but would work OK in practice.

  2. Reusing a working tree file is only half of the reason a filespec
     might be mmap'd. It might also be because we are literally diffing
     the working tree. "-G" was meant to be used to limit log traversal,
     but it also works to reduce the diff output for something like "git
     diff HEAD^".

I really wish there were an alternate regexec interface we could use that took a pointer/size pair. Bleh.

-Peff

On Mon, Oct 29, 2012 at 06:47:05PM -0400, Jeff King wrote:

Thinking on it more, my patch, hacky thought it seems, may not be the worst solution. Here are the options that I see:

  1. Use a regex library that does not require NUL termination. If we
     are bound by the regular regexec interface, this is not feasible.
     But the GNU implementation works on arbitrary-length buffers (you
     just have to use a slightly different interface), and we already
     carry it in compat. It would mean platforms which provide a working
     but non-GNU regexec would have to start defining NO_REGEX.

  2. Figure out a way to get one extra zero byte via mmap. If the
     requested size does not fall on a page boundary, you get extra
     zero-ed bytes. Unfortunately, requesting an extra byte does not
     do what we want; you get SIGBUS accessing it.

  3. Copy mmap'd data at point-of-use into a NUL-terminated buffer. That
     way we only incur the cost when we need it.

  4. Avoid mmap-ing in the first place when we are using -G or
     --pickaxe-regex (e.g., by doing a big read()). At first glance,
     this sounds more efficient than loading the data one way and then
     making another copy. But mmap+memcpy, aside from the momentary
     doubled memory requirement, is probably just as fast or faster than
     calling read() repeatedly.

I am really tempted by (1).

Given that (2) does not work, unless somebody comes up with something clever there, that would make (3) the next best choice.

-Peff

(1) sounds attractive for more than one reason. In addition to avoidance of this issue, it would bring bug-to-bug compatibility across platforms.

(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.

Jeff King <peff@peff.net> wrote:

On Tue, Oct 30, 2012 at 09:46:01PM +0900, Junio C Hamano wrote:

Yeah. I mentioned breaking the build for people who would now need to turn on NO_REGEX. But the only reason to do that is to let people on glibc systems use the system version of the tools. A much saner approach would be to just always build with our compat regex, and turn NO_REGEX into a no-op. We already do the same thing for kwset.

Agreed, that would be nice. It's potentially a lot of work, but we could probably get by with a special streaming version of diff_populate_filespec.

The tricky thing is that we have to run the regex matcher progressively as we stream data in (since your match might fall in the middle of a read boundary). Which is certainly going to require switching off of regular regexec. I don't think glibc regex will handle it either, though. It looks like pcre can report a partial match at the end of the string, and you can either continue with the next chunk (if using pcre_dfa) or append and re-start the pattern match (for regular pcre_exec).

Which means we'd probably have to make streaming matches an optional feature, and still do (1) first to fix the correctness issue.

-Peff

Jeff King wrote:

I have thought about the possibility of doing this for unrelated reasons in the past.

On cygwin, there have been two unexpected test passes since about v1.6.0 (I reported it to the list in passing), like so:

    [ ... ]
    All tests successful.

    Test Summary Report
    -------------------
    t0050-filesystem.sh                              (Wstat: 0 Tests: 9 Failed: 0)
      TODO passed:   5
    t7008-grep-binary.sh                             (Wstat: 0 Tests: 20 Failed: 0)
      TODO passed:   12
    Files=604, Tests=8439, 11190 wallclock secs ( 2.59 usr  1.59 sys + 7294.86 cusr
    3416.65 csys = 10715.70 CPU)
    Result: PASS

In particular, t7008.12 passes on cygwin because the regex library apparently matches '.' to NUL. Indeed if you add a test_pause to the script and execute "grep .fi a" (note grep *not* git-grep) then "Binary file a matches" on Linux, cygwin and MinGW. (So I assume the test was added to document a difference in behaviour to GNU grep).

So, if we use the GNU interface to the regex routines in compat, then we may specify the "grep syntax" for use in git-grep. (Well that's the theory, I've not actually tried to code it up, so take this with a pinch of salt! :-P ).

ATB, Ramsay Jones

On 2012-10-29 23:35, Jeff King wrote:

Hi, I tested your patch, and i can confirm it fixes the problem for me. (also on my real world test in msysgit)

Again, thanks a lot! Greetings Peter

On Wed, Nov 07, 2012 at 10:10:59PM +0100, Peter Oberndorfer wrote:

Thanks for the report. I'd still like to pursue using a regex library that does not require NUL-termination, but I've been distracted by other things. I'm going to hold back my copy-to-a-NUL-buffer patch for now and see if I can get to the regex thing this week.

-Peff

On 2012-11-07 22:13, Jeff King wrote:

Hi,

are there any news regarding this problem? The crash seems to still exist in the current version 1.8.3 and master.

Thanks, Greetings Peter

On Mon, Jun 03, 2013 at 07:25:06PM +0200, Peter Oberndorfer wrote:

Sorry, no, this got dropped due to lack of time. I _think_ it is as simple as just tweaking the Makefile to unconditionally build against the compat/ regex library, and then tweaking callsites as appropriate to use the GNU-specific interface that takes buf/len instead of a NUL-terminated string.

But there may be some hidden complexities to it.

-Peff