diff options
author | Jeff King <peff@peff.net> | 2021-08-09 21:01:52 -0400 |
---|---|---|
committer | Junio C Hamano <gitster@pobox.com> | 2021-08-10 11:38:13 -0700 |
commit | 46d723ce57f2dd3c50504dc6f4ca73b4c392fa6f (patch) | |
tree | 1518e7fc1d64be9eb10cf8e2ca48527b49fc36b9 | |
parent | Git 2.22.5 (diff) | |
download | tgif-46d723ce57f2dd3c50504dc6f4ca73b4c392fa6f.tar.xz |
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>
-rw-r--r-- | apply.c | 1 | ||||
-rwxr-xr-x | t/t4103-apply-binary.sh | 23 |
2 files changed, 24 insertions, 0 deletions
@@ -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 1b420e3b5f..290779406f 100755 --- 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 |