apply: keep buffer/size pair in sync when parsing binary hunks

We parse through binary hunks by looping through the buffer with code
like:

    llen = linelen(buffer, size);

    ...do something with the line...

    buffer += llen;
    size -= llen;

However, before we enter the loop, there is one call that increments
"buffer" but forgets to decrement "size". As a result, our "size" is off
by the length of that line, and subsequent calls to linelen() may look
past the end of the buffer for a newline.

The fix is easy: we just need to decrement size as we do elsewhere.

This bug goes all the way back to 0660626caf (binary diff: further
updates., 2006-05-05). Presumably nobody noticed because it only
triggers if the patch is corrupted, and even then we are often "saved"
by luck. We use a strbuf to store the incoming patch, so we overallocate
there, plus we add a 16-byte run of NULs as slop for memory comparisons.
So if this happened accidentally, the common case is that we'd just read
a few uninitialized bytes from the end of the strbuf before producing
the expected "this patch is corrupted" error complaint.

However, it is possible to carefully construct a case which reads off
the end of the buffer. The included test does so. It will pass both
before and after this patch when run normally, but using a tool like
ASan shows that we get an out-of-bounds read before this patch, but not
after.

Reported-by: Xingman Chen <xichixingman@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/apply.c b/apply.c
index 4992eca416c9c2b5d21de42460cafa839df25f4e..ee317725b97a7829ca34f4697177b17c12c4878e 100644 (file)
--- a/apply.c
+++ b/apply.c
@@ -1937,6 +1937,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
 
        state->linenr++;
        buffer += llen;
+       size -= llen;
        while (1) {
                int byte_length, max_byte_length, newsize;
                llen = linelen(buffer, size);

diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
index 1b420e3b5fc2a7130d2b10bcd5660670b9d780c8..290779406f4a169b24940e86f92179163eba02e9 100755 (executable)
--- a/t/t4103-apply-binary.sh
+++ b/t/t4103-apply-binary.sh
@@ -155,4 +155,27 @@ test_expect_success 'apply binary -p0 diff' '
        test -z "$(git diff --name-status binary -- file3)"
 '
 
+test_expect_success 'reject truncated binary diff' '
+       do_reset &&
+
+       # this length is calculated to get us very close to
+       # the 8192-byte strbuf we will use to read in the patch.
+       test-tool genrandom foo 6205 >file1 &&
+       git diff --binary >patch &&
+
+       # truncate the patch at the second "literal" line,
+       # but exclude the trailing newline. We must use perl
+       # for this, since tools like "sed" cannot reliably
+       # produce output without the trailing newline.
+       perl -pe "
+               if (/^literal/ && \$count++ >= 1) {
+                       chomp;
+                       print;
+                       exit 0;
+               }
+       " <patch >patch.trunc &&
+
+       do_reset &&
+       test_must_fail git apply patch.trunc
+'
 test_done