threads / discuss / 25471

[CRASH] git segfaults on invalid binary diff

Subject: [CRASH] git segfaults on invalid binary diff

## tl;dr

3 messages between Oct 18, 2010 and Oct 18, 2010.

replies: 2people: 2as markdown or json

Rafaël Carré· Oct 18, 2010, 16:06 UTC · lore

Hi,

I noticed git crashed on a patch generated with: git show --full-index --pretty=email <commit>

(I had forgotten --binary)

% git --version
git version 1.7.3.1.120.g38a18
% mkdir test
% cd test
% git init
Initialized empty Git repository in /home/fun/test/.git/
% git am ~/bad.diff
Applying: foo bar
Segmentation fault (core dumped)
Patch failed at 0001 foo bar
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
% cat ~/bad.diff
From 2e888fde246ecb0d346ecc02e7df0c82724f02b9 Mon Sep 17 00:00:00 2001
From: John Doe <john.doe@example.org>
Date: Mon, 4 Feb 2008 01:17:33 +0000
Subject: [PATCH] foo
     bar

diff --git a/axvlc.tlb b/axvlc.tlb
new file mode 100644
index 0000000000000000000000000000000000000000..a61182de2f3eb2dc2fc13a08b448b79ae56f1967
Binary files /dev/null and b/axvlc.tlb differ
% gdb git core
GNU gdb (GDB) 7.2-ubuntu
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/fun/.local/git/bin/git...done.
[New Thread 24102]

warning: Can't read pathname for load map: Erreur d'entrée/sortie.
Reading symbols from /lib/libz.so.1...(no debugging symbols found)...done.
Loaded symbols for /lib/libz.so.1
Reading symbols from /lib/libpthread.so.0...Reading symbols from /usr/lib/debug/lib/libpthread-2.12.1.so...done.
done.
Loaded symbols for /lib/libpthread.so.0
Reading symbols from /lib/libc.so.6...Reading symbols from /usr/lib/debug/lib/libc-2.12.1.so...done.
done.
Loaded symbols for /lib/libc.so.6
Reading symbols from /lib64/ld-linux-x86-64.so.2...Reading symbols from /usr/lib/debug/lib/ld-2.12.1.so...done.
done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
Core was generated by `git apply --index /home/fun/test/.git/rebase-apply/patch'.
Program terminated with signal 11, Segmentation fault.
#0  0x000000000040c49e in apply_binary_fragment (img=0x7fff47152cd0, 
    patch=0x16ef2a0) at builtin/apply.c:2657
2657		switch (fragment->binary_patch_method) {
(gdb) bt
#0  0x000000000040c49e in apply_binary_fragment (img=0x7fff47152cd0, 
    patch=0x16ef2a0) at builtin/apply.c:2657
#1  0x000000000040c7ef in apply_binary (img=0x7fff47152cd0, patch=0x16ef2a0)
    at builtin/apply.c:2739
#2  0x000000000040c92f in apply_fragments (img=0x7fff47152cd0, patch=0x16ef2a0)
    at builtin/apply.c:2761
#3  0x000000000040cdc5 in apply_data (patch=0x16ef2a0, st=0x7fff47152d60, 
    ce=0x0) at builtin/apply.c:2908
#4  0x000000000040d64a in check_patch (patch=0x16ef2a0) at builtin/apply.c:3098
#5  0x000000000040d6d7 in check_patch_list (patch=0x16ef2a0)
    at builtin/apply.c:3113
#6  0x000000000040ef6d in apply_patch (fd=3, 
    filename=0x7fff471549e7 "/home/fun/test/.git/rebase-apply/patch", 
    options=0) at builtin/apply.c:3683
#7  0x000000000040faee in cmd_apply (argc=1, argv=0x7fff471538a0, prefix_=0x0)
    at builtin/apply.c:3897
#8  0x00000000004048c5 in run_builtin (p=0x766688, argc=3, argv=0x7fff471538a0)
    at git.c:275
#9  0x0000000000404a60 in handle_internal_command (argc=3, argv=0x7fff471538a0)
    at git.c:431
#10 0x0000000000404b5a in run_argv (argcp=0x7fff4715378c, argv=0x7fff47153780)
    at git.c:475
#11 0x0000000000404ce2 in main (argc=3, argv=0x7fff471538a0) at git.c:548
(gdb) frame 0
#0  0x000000000040c49e in apply_binary_fragment (img=0x7fff47152cd0, 
    patch=0x16ef2a0) at builtin/apply.c:2657
2657		switch (fragment->binary_patch_method) {
(gdb) print *patch
$1 = {new_name = 0x16ec0e0 "axvlc.tlb", old_name = 0x0, 
  def_name = 0x16ec0e0 "axvlc.tlb", old_mode = 0, new_mode = 33188, 
  is_new = 1, is_delete = 0, rejected = 1, ws_rule = 19, deflate_origlen = 0, 
  lines_added = 0, lines_deleted = 0, score = 0, is_toplevel_relative = 1, 
  inaccurate_eof = 0, is_binary = 1, is_copy = 0, is_rename = 0, recount = 0, 
  fragments = 0x0, result = 0x0, resultsize = 0, 
  old_sha1_prefix = '0' <se r\377\377\377\377\377������\000\000\000\377\377\377\377\377������������������������������������������������������������������, 
  new_sha1_prefix = "a61182de2f3eb2dc2fc13a08b448b79ae56f1967", next = 0x0}
(gdb)

-- 
✍ Rafaël Carré ☺

Jeff King· Oct 18, 2010, 17:40 UTC · re: Rafaël Carré · lore

Re: [CRASH] git segfaults on invalid binary diff

On Mon, Oct 18, 2010 at 06:06:32PM +0200, Rafaël Carré wrote:

Show 19 quoted lines

> I noticed git crashed on a patch generated with:
> git show --full-index --pretty=email <commit>
> 
> (I had forgotten --binary)
> 
> % git --version
> git version 1.7.3.1.120.g38a18
> % mkdir test
> % cd test
> % git init
> Initialized empty Git repository in /home/fun/test/.git/
> % git am ~/bad.diff
> Applying: foo bar
> Segmentation fault (core dumped)
> Patch failed at 0001 foo bar
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
> % cat ~/bad.diff

Hmm, the key seems to be applying it in a new repo without the actual objects. Git will fall back to trying to apply based on the index line, and that is presumably where the bug is. I'll try to work up a patch.

-Peff

Jeff King· Oct 18, 2010, 18:39 UTC · re: Jeff King · lore

[PATCH] apply: don't segfault on binary files with missing data

Usually when applying a binary diff generated without --binary, it will be rejected early, as we don't even have the full sha1 of the pre- and post-images.

However, if the diff is generated with --full-index (but not --binary), then we will actually try to apply it. If we have the postimage blob, then we can take a shortcut and never even look at the binary diff at all (e.g., this can happen when rebasing changes within a repository).

If we don't have the postimage blob, though, we try to look at the actual fragments, of which there are none, and get a segfault. This patch checks explicitly for that case and complains to the user instead of segfaulting. We need to keep the check at a low level so that the "shortcut" case above continues to work.

We also add a test that demonstrates the segfault. While we're at it, let's also explicitly test the shortcut case.

Reported-by: Rafaël Carré <rafael.carre@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/apply.c         |    6 ++++++
 t/t4103-apply-binary.sh |   27 ++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 23c18c5..f051e66 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2645,6 +2645,12 @@ static int apply_binary_fragment(struct image *img, struct patch *patch)
 	unsigned long len;
 	void *dst;
 
+	if (!fragment)
+		return error("missing binary patch data for '%s'",
+			     patch->new_name ?
+			     patch->new_name :
+			     patch->old_name);
+
 	/* Binary patch is irreversible without the optional second hunk */
 	if (apply_in_reverse) {
 		if (!fragment->next)
diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
index 9692f16..08ad6d8 100755
--- a/t/t4103-apply-binary.sh
+++ b/t/t4103-apply-binary.sh
@@ -37,7 +37,16 @@ test_expect_success 'setup' "
 	git diff-tree -p -C master binary >C.diff &&
 
 	git diff-tree -p --binary master binary >BF.diff &&
-	git diff-tree -p --binary -C master binary >CF.diff
+	git diff-tree -p --binary -C master binary >CF.diff &&
+
+	git diff-tree -p --full-index master binary >B-index.diff &&
+	git diff-tree -p -C --full-index master binary >C-index.diff &&
+
+	git init other-repo &&
+	(cd other-repo &&
+	 git fetch .. master &&
+	 git reset --hard FETCH_HEAD
+	)
 "
 
 test_expect_success 'stat binary diff -- should not fail.' \
@@ -100,6 +109,22 @@ test_expect_success 'apply binary diff (copy) -- should fail.' \
 	'do_reset &&
 	 test_must_fail git apply --index C.diff'
 
+test_expect_success 'apply binary diff with full-index' '
+	do_reset &&
+	git apply B-index.diff
+'
+
+test_expect_success 'apply binary diff with full-index (copy)' '
+	do_reset &&
+	git apply C-index.diff
+'
+
+test_expect_success 'apply full-index binary diff in new repo' '
+	(cd other-repo &&
+	 do_reset &&
+	 test_must_fail git apply ../B-index.diff)
+'
+
 test_expect_success 'apply binary diff without replacement.' \
 	'do_reset &&
 	 git apply BF.diff'

-- 
1.7.3.1.227.ge6319.dirty