Re: [PATCH 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers

[Johannes Schindelin]

Hi Peff,

On Tue, 6 Sep 2016, Jeff King wrote:

> On Mon, Sep 05, 2016 at 05:45:02PM +0200, Johannes Schindelin wrote:
> 
> > Typically, on Linux the test passes. On Windows, it fails virtually
> > every time due to an access violation (that's a segmentation fault for
> > you Unix-y people out there). And Windows would be correct: the
> > regexec() call wants to operate on a regular, NUL-terminated string,
> > there is no NUL in the mmap()ed memory range, and it is undefined
> > whether the next byte is even legal to access.
> > 
> > When run with --valgrind it demonstrates quite clearly the breakage, of
> > course.
> > 
> > So we simply mark it with `test_expect_success` for now.
> 
> I'd prefer if this were marked as expect_failure. It fails reliably for
> me on Linux, even without --valgrind. But even if that were not so,
> there is no reason to hurt bisectability of somebody running with
> "--valgrind" (not when it costs so little to mark it correctly).

Forgot to say in the cover letter: I did change this from
test_expect_success to test_expect_failure.

But of course, now I remember that I failed to change it back in 3/3. Bah.

Ciao,
Dscho

Re: [PATCH 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers

[Jeff King]

On Mon, Sep 05, 2016 at 05:45:02PM +0200, Johannes Schindelin wrote:

> Typically, on Linux the test passes. On Windows, it fails virtually
> every time due to an access violation (that's a segmentation fault for
> you Unix-y people out there). And Windows would be correct: the
> regexec() call wants to operate on a regular, NUL-terminated string,
> there is no NUL in the mmap()ed memory range, and it is undefined
> whether the next byte is even legal to access.
> 
> When run with --valgrind it demonstrates quite clearly the breakage, of
> course.
> 
> So we simply mark it with `test_expect_success` for now.

I'd prefer if this were marked as expect_failure. It fails reliably for
me on Linux, even without --valgrind. But even if that were not so,
there is no reason to hurt bisectability of somebody running with
"--valgrind" (not when it costs so little to mark it correctly).

-Peff

[PATCH 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers

[Johannes Schindelin]

When our pickaxe code feeds file contents to regexec(), it implicitly
assumes that the file contents are read into implicitly NUL-terminated
buffers (i.e. that we overallocate by 1, appending a single '\0').

This is not so.

In particular when the file contents are simply mmap()ed, we can be
virtually certain that the buffer is preceding uninitialized bytes, or
invalid pages.

Note that the test we add here is known to be flakey: we simply cannot
know whether the byte following the mmap()ed ones is a NUL or not.

Typically, on Linux the test passes. On Windows, it fails virtually
every time due to an access violation (that's a segmentation fault for
you Unix-y people out there). And Windows would be correct: the
regexec() call wants to operate on a regular, NUL-terminated string,
there is no NUL in the mmap()ed memory range, and it is undefined
whether the next byte is even legal to access.

When run with --valgrind it demonstrates quite clearly the breakage, of
course.

So we simply mark it with `test_expect_success` for now.

This test case represents a Minimal, Complete and Verifiable Example of
a breakage reported by Chris Sidi.

Signed-off-by: Johannes Schindelin 
---
 t/t4059-diff-pickaxe.sh | 22 ++
 1 file changed, 22 insertions(+)
 create mode 100755 t/t4059-diff-pickaxe.sh

diff --git a/t/t4059-diff-pickaxe.sh b/t/t4059-diff-pickaxe.sh
new file mode 100755
index 000..f0bf50b
--- /dev/null
+++ b/t/t4059-diff-pickaxe.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Johannes Schindelin
+#
+
+test_description='Pickaxe options'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   test_commit initial &&
+   printf "%04096d" 0 >4096-zeroes.txt &&
+   git add 4096-zeroes.txt &&
+   test_tick &&
+   git commit -m "A 4k file"
+'
+test_expect_success '-G matches' '
+   git diff --name-only -G "^0{4096}$" HEAD^ >out &&
+   test 4096-zeroes.txt = "$(cat out)"
+'
+
+test_done
-- 
2.10.0.windows.1.2.g732a511