# [PATCH] fix "git apply --index ..." not to deref NULL 3 messages from 2011-10-12 to 2011-10-12. Participants: Jim Meyering, Jeff King. Thread: https://gitlist.dev/t/28656 ## Jim Meyering, 2011-10-12 08:18 Subject: [PATCH] fix "git apply --index ..." not to deref NULL Message-ID: <87lisq8vye.fsf@rho.meyering.net> URL: https://gitlist.dev/e/87lisq8vye.fsf%40rho.meyering.net ``` I noticed this when "git am CORRUPTED" unexpectedly failed with an odd diagnostic, and even removed one of the files it was supposed to have patched. Reproduce with any valid old/new patch from which you have removed the "+++ b/FILE" line. You'll see a diagnostic like this fatal: unable to write file '(null)' mode 100644: Bad address and you'll find that FILE has been removed. The above is on glibc-based systems. On other systems, rather than getting "null" in parentheses, you'll probably provoke a segfault, as git tries to dereference the NULL file name. Signed-off-by: Jim Meyering --- builtin/apply.c | 3 +++ t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 0 deletions(-) create mode 100644 t/t4254-am-corrupt.sh diff --git a/builtin/apply.c b/builtin/apply.c index f2edc52..aaa39fe 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1407,6 +1407,9 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc "%d leading pathname components (line %d)" , p_value, linenr); patch->old_name = patch->new_name = patch->def_name; } + if (!patch->is_delete && !patch->new_name) + die("git diff header lacks filename information " + "(line %d)", linenr); patch->is_toplevel_relative = 1; *hdrsize = git_hdr_len; return offset; diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh new file mode 100644 index 0000000..b7da95f --- /dev/null +++ b/t/t4254-am-corrupt.sh @@ -0,0 +1,43 @@ +#!/bin/sh + +test_description='git am with corrupt input' +. ./test-lib.sh + +# Note the missing "+++" line: +cat > bad-patch.diff <<'EOF' +From: A U Thor +diff --git a/f b/f +index 7898192..6178079 100644 +--- a/f +@@ -1 +1 @@ +-a ++b +EOF + +test_expect_success setup ' + test $? = 0 && + echo a > f && + git add f && + test_tick && + git commit -m initial +' + +# This used to fail before, too, but with a different diagnostic. +# fatal: unable to write file '(null)' mode 100644: Bad address +# Also, it had the unwanted side-effect of deleting f. +test_expect_success 'try to apply corrupted patch' ' + git am bad-patch.diff 2> actual + test $? = 1 +' + +cat > expected < URL: https://gitlist.dev/e/20111012142750.GB25085%40sigill.intra.peff.net In-Reply-To: <87lisq8vye.fsf@rho.meyering.net> ``` On Wed, Oct 12, 2011 at 10:18:01AM +0200, Jim Meyering wrote: > I noticed this when "git am CORRUPTED" unexpectedly failed with an > odd diagnostic, and even removed one of the files it was supposed > to have patched. > > Reproduce with any valid old/new patch from which you have removed > the "+++ b/FILE" line. You'll see a diagnostic like this > > fatal: unable to write file '(null)' mode 100644: Bad address > > and you'll find that FILE has been removed. Yikes. Your fix looks right to me. > builtin/apply.c | 3 +++ > t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+), 0 deletions(-) > create mode 100644 t/t4254-am-corrupt.sh Missing executable bit on the new test. -Peff ``` ## Jim Meyering, 2011-10-12 14:33 Subject: Re: [PATCH] fix "git apply --index ..." not to deref NULL Message-ID: <87sjmy5lf1.fsf@rho.meyering.net> URL: https://gitlist.dev/e/87sjmy5lf1.fsf%40rho.meyering.net In-Reply-To: <20111012142750.GB25085@sigill.intra.peff.net> ``` Jeff King wrote: > On Wed, Oct 12, 2011 at 10:18:01AM +0200, Jim Meyering wrote: > >> I noticed this when "git am CORRUPTED" unexpectedly failed with an >> odd diagnostic, and even removed one of the files it was supposed >> to have patched. >> >> Reproduce with any valid old/new patch from which you have removed >> the "+++ b/FILE" line. You'll see a diagnostic like this >> >> fatal: unable to write file '(null)' mode 100644: Bad address >> >> and you'll find that FILE has been removed. > > Yikes. Your fix looks right to me. > >> builtin/apply.c | 3 +++ >> t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 46 insertions(+), 0 deletions(-) >> create mode 100644 t/t4254-am-corrupt.sh > > Missing executable bit on the new test. Thanks. Fixed with this: -- >8 -- Subject: [PATCH] fix "git apply --index ..." not to deref NULL I noticed this when "git am CORRUPTED" unexpectedly failed with an odd diagnostic, and even removed one of the files it was supposed to have patched. Reproduce with any valid old/new patch from which you have removed the "+++ b/FILE" line. You'll see a diagnostic like this fatal: unable to write file '(null)' mode 100644: Bad address and you'll find that FILE has been removed. The above is on glibc-based systems. On other systems, rather than getting "null", you may provoke a segfault as git tries to dereference the NULL file name. Signed-off-by: Jim Meyering --- builtin/apply.c | 3 +++ t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 0 deletions(-) create mode 100755 t/t4254-am-corrupt.sh diff --git a/builtin/apply.c b/builtin/apply.c index f2edc52..aaa39fe 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1407,6 +1407,9 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc "%d leading pathname components (line %d)" , p_value, linenr); patch->old_name = patch->new_name = patch->def_name; } + if (!patch->is_delete && !patch->new_name) + die("git diff header lacks filename information " + "(line %d)", linenr); patch->is_toplevel_relative = 1; *hdrsize = git_hdr_len; return offset; diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh new file mode 100755 index 0000000..b7da95f --- /dev/null +++ b/t/t4254-am-corrupt.sh @@ -0,0 +1,43 @@ +#!/bin/sh + +test_description='git am with corrupt input' +. ./test-lib.sh + +# Note the missing "+++" line: +cat > bad-patch.diff <<'EOF' +From: A U Thor +diff --git a/f b/f +index 7898192..6178079 100644 +--- a/f +@@ -1 +1 @@ +-a ++b +EOF + +test_expect_success setup ' + test $? = 0 && + echo a > f && + git add f && + test_tick && + git commit -m initial +' + +# This used to fail before, too, but with a different diagnostic. +# fatal: unable to write file '(null)' mode 100644: Bad address +# Also, it had the unwanted side-effect of deleting f. +test_expect_success 'try to apply corrupted patch' ' + git am bad-patch.diff 2> actual + test $? = 1 +' + +cat > expected <