Skip to content

Commit 46d723c

Browse files
peffgitster
authored andcommitted
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 0660626 (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>
1 parent c753e2a commit 46d723c

File tree

2 files changed

+24
-0
lines changed

2 files changed

+24
-0
lines changed

apply.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1937,6 +1937,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
19371937

19381938
state->linenr++;
19391939
buffer += llen;
1940+
size -= llen;
19401941
while (1) {
19411942
int byte_length, max_byte_length, newsize;
19421943
llen = linelen(buffer, size);

t/t4103-apply-binary.sh

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,4 +155,27 @@ test_expect_success 'apply binary -p0 diff' '
155155
test -z "$(git diff --name-status binary -- file3)"
156156
'
157157

158+
test_expect_success 'reject truncated binary diff' '
159+
do_reset &&
160+
161+
# this length is calculated to get us very close to
162+
# the 8192-byte strbuf we will use to read in the patch.
163+
test-tool genrandom foo 6205 >file1 &&
164+
git diff --binary >patch &&
165+
166+
# truncate the patch at the second "literal" line,
167+
# but exclude the trailing newline. We must use perl
168+
# for this, since tools like "sed" cannot reliably
169+
# produce output without the trailing newline.
170+
perl -pe "
171+
if (/^literal/ && \$count++ >= 1) {
172+
chomp;
173+
print;
174+
exit 0;
175+
}
176+
" <patch >patch.trunc &&
177+
178+
do_reset &&
179+
test_must_fail git apply patch.trunc
180+
'
158181
test_done

0 commit comments

Comments
 (0)