{"thread":{"id":"28656","subject":"[PATCH] fix \"git apply --index ...\" not to deref NULL","startedAt":"2011-10-12T08:18:01Z","lastAt":"2011-10-12T14:33:54Z","messageCount":3,"participants":["Jim Meyering","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"177431","messageId":"87lisq8vye.fsf@rho.meyering.net","threadId":"28656","inReplyTo":null,"subject":"[PATCH] fix \"git apply --index ...\" not to deref NULL","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-10-12T08:18:01Z","receivedAt":"2011-10-12T08:18:01Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\nI noticed this when \"git am CORRUPTED\" unexpectedly failed with an\nodd diagnostic, and even removed one of the files it was supposed\nto have patched.\n\nReproduce with any valid old/new patch from which you have removed\nthe \"+++ b/FILE\" line. You'll see a diagnostic like this\n\n fatal: unable to write file '(null)' mode 100644: Bad address\n\nand you'll find that FILE has been removed.\n\nThe above is on glibc-based systems. On other systems, rather than\ngetting \"null\" in parentheses, you'll probably provoke a segfault,\nas git tries to dereference the NULL file name.\n\nSigned-off-by: Jim Meyering \n---\n builtin/apply.c | 3 +++\n t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 0 deletions(-)\n create mode 100644 t/t4254-am-corrupt.sh\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex f2edc52..aaa39fe 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1407,6 +1407,9 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc\n \t\t\t\t\t \"%d leading pathname components (line %d)\" , p_value, linenr);\n \t\t\t\tpatch->old_name = patch->new_name = patch->def_name;\n \t\t\t}\n+\t\t\tif (!patch->is_delete && !patch->new_name)\n+\t\t\t\tdie(\"git diff header lacks filename information \"\n+\t\t\t\t \"(line %d)\", linenr);\n \t\t\tpatch->is_toplevel_relative = 1;\n \t\t\t*hdrsize = git_hdr_len;\n \t\t\treturn offset;\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nnew file mode 100644\nindex 0000000..b7da95f\n--- /dev/null\n+++ b/t/t4254-am-corrupt.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+test_description='git am with corrupt input'\n+. ./test-lib.sh\n+\n+# Note the missing \"+++\" line:\n+cat > bad-patch.diff <<'EOF'\n+From: A U Thor \n+diff --git a/f b/f\n+index 7898192..6178079 100644\n+--- a/f\n+@@ -1 +1 @@\n+-a\n++b\n+EOF\n+\n+test_expect_success setup '\n+\ttest $? = 0 &&\n+\techo a > f &&\n+\tgit add f &&\n+\ttest_tick &&\n+\tgit commit -m initial\n+'\n+\n+# This used to fail before, too, but with a different diagnostic.\n+# fatal: unable to write file '(null)' mode 100644: Bad address\n+# Also, it had the unwanted side-effect of deleting f.\n+test_expect_success 'try to apply corrupted patch' '\n+\tgit am bad-patch.diff 2> actual\n+\ttest $? = 1\n+'\n+\n+cat > expected < I noticed this when \"git am CORRUPTED\" unexpectedly failed with an\n> odd diagnostic, and even removed one of the files it was supposed\n> to have patched.\n> \n> Reproduce with any valid old/new patch from which you have removed\n> the \"+++ b/FILE\" line. You'll see a diagnostic like this\n> \n> fatal: unable to write file '(null)' mode 100644: Bad address\n> \n> and you'll find that FILE has been removed.\n\nYikes. Your fix looks right to me.\n\n> builtin/apply.c | 3 +++\n> t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 46 insertions(+), 0 deletions(-)\n> create mode 100644 t/t4254-am-corrupt.sh\n\nMissing executable bit on the new test.\n\n-Peff\n"},{"id":"177442","messageId":"87sjmy5lf1.fsf@rho.meyering.net","threadId":"28656","inReplyTo":"20111012142750.GB25085@sigill.intra.peff.net","subject":"Re: [PATCH] fix \"git apply --index ...\" not to deref NULL","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-10-12T14:33:54Z","receivedAt":"2011-10-12T14:33:54Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Jeff King wrote:\n> On Wed, Oct 12, 2011 at 10:18:01AM +0200, Jim Meyering wrote:\n>\n>> I noticed this when \"git am CORRUPTED\" unexpectedly failed with an\n>> odd diagnostic, and even removed one of the files it was supposed\n>> to have patched.\n>>\n>> Reproduce with any valid old/new patch from which you have removed\n>> the \"+++ b/FILE\" line. You'll see a diagnostic like this\n>>\n>> fatal: unable to write file '(null)' mode 100644: Bad address\n>>\n>> and you'll find that FILE has been removed.\n>\n> Yikes. Your fix looks right to me.\n>\n>> builtin/apply.c | 3 +++\n>> t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 46 insertions(+), 0 deletions(-)\n>> create mode 100644 t/t4254-am-corrupt.sh\n>\n> Missing executable bit on the new test.\n\nThanks.\nFixed with this:\n\n-- >8 --\nSubject: [PATCH] fix \"git apply --index ...\" not to deref NULL\n\nI noticed this when \"git am CORRUPTED\" unexpectedly failed with an\nodd diagnostic, and even removed one of the files it was supposed\nto have patched.\n\nReproduce with any valid old/new patch from which you have removed\nthe \"+++ b/FILE\" line. You'll see a diagnostic like this\n\n fatal: unable to write file '(null)' mode 100644: Bad address\n\nand you'll find that FILE has been removed.\n\nThe above is on glibc-based systems. On other systems, rather than\ngetting \"null\", you may provoke a segfault as git tries to\ndereference the NULL file name.\n\nSigned-off-by: Jim Meyering \n---\n builtin/apply.c | 3 +++\n t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 0 deletions(-)\n create mode 100755 t/t4254-am-corrupt.sh\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex f2edc52..aaa39fe 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1407,6 +1407,9 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc\n \t\t\t\t\t \"%d leading pathname components (line %d)\" , p_value, linenr);\n \t\t\t\tpatch->old_name = patch->new_name = patch->def_name;\n \t\t\t}\n+\t\t\tif (!patch->is_delete && !patch->new_name)\n+\t\t\t\tdie(\"git diff header lacks filename information \"\n+\t\t\t\t \"(line %d)\", linenr);\n \t\t\tpatch->is_toplevel_relative = 1;\n \t\t\t*hdrsize = git_hdr_len;\n \t\t\treturn offset;\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nnew file mode 100755\nindex 0000000..b7da95f\n--- /dev/null\n+++ b/t/t4254-am-corrupt.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+test_description='git am with corrupt input'\n+. ./test-lib.sh\n+\n+# Note the missing \"+++\" line:\n+cat > bad-patch.diff <<'EOF'\n+From: A U Thor \n+diff --git a/f b/f\n+index 7898192..6178079 100644\n+--- a/f\n+@@ -1 +1 @@\n+-a\n++b\n+EOF\n+\n+test_expect_success setup '\n+\ttest $? = 0 &&\n+\techo a > f &&\n+\tgit add f &&\n+\ttest_tick &&\n+\tgit commit -m initial\n+'\n+\n+# This used to fail before, too, but with a different diagnostic.\n+# fatal: unable to write file '(null)' mode 100644: Bad address\n+# Also, it had the unwanted side-effect of deleting f.\n+test_expect_success 'try to apply corrupted patch' '\n+\tgit am bad-patch.diff 2> actual\n+\ttest $? = 1\n+'\n+\n+cat > expected <