{"thread":{"id":"45222","subject":"[PATCH 1/2] apply: guard against renames of non-existant empty files","startedAt":"2017-02-25T10:13:24Z","lastAt":"2017-06-27T21:39:07Z","messageCount":19,"participants":["Vegard Nossum","Philip Oakley","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"312628","messageId":"20170225101307.24067-1-vegard.nossum@oracle.com","threadId":"45222","inReplyTo":null,"subject":"[PATCH 1/2] apply: guard against renames of non-existant empty files","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-02-25T10:13:06Z","receivedAt":"2017-02-25T10:13:24Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"If we have a patch like the one in the new test-case, then we will\ntry to rename a non-existant empty file, i.e. patch->old_name will\nbe NULL. In this case, a NULL entry will be added to fn_table, which\nis not allowed (a subsequent binary search will die with a NULL\npointer dereference).\n\nThe patch file is completely bogus as it tries to rename something\nthat is known not to exist, so we can throw an error for this.\n\nFound using AFL.\n\nSigned-off-by: Vegard Nossum \n---\n apply.c | 3 ++-\n t/t4154-apply-git-header.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n create mode 100755 t/t4154-apply-git-header.sh\n\ndiff --git a/apply.c b/apply.c\nindex 0e2caeab9..cbf7cc7f2 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,\n \t\t\t\tpatch->old_name = xstrdup(patch->def_name);\n \t\t\t\tpatch->new_name = xstrdup(patch->def_name);\n \t\t\t}\n-\t\t\tif (!patch->is_delete && !patch->new_name) {\n+\t\t\tif ((!patch->is_delete && !patch->new_name) ||\n+\t\t\t (patch->is_rename && !patch->old_name)) {\n \t\t\t\terror(_(\"git diff header lacks filename information \"\n \t\t\t\t\t \"(line %d)\"), state->linenr);\n \t\t\t\treturn -128;\ndiff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh\nnew file mode 100755\nindex 000000000..d651af4a2\n--- /dev/null\n+++ b/t/t4154-apply-git-header.sh\n@@ -0,0 +1,15 @@\n+#!/bin/sh\n+\n+test_description='apply with git/--git headers'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'apply old mode / rename new' '\n+\ttest_must_fail git apply << EOF\n+diff --git a/1 b/1\n+old mode 0\n+rename new 0\n+EOF\n+'\n+\n+test_done\n-- \n2.12.0.rc0\n\n"},{"id":"312630","messageId":"20170225101307.24067-2-vegard.nossum@oracle.com","threadId":"45222","inReplyTo":"20170225101307.24067-1-vegard.nossum@oracle.com","subject":"[PATCH 2/2] apply: handle assertion failure gracefully","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-02-25T10:13:07Z","receivedAt":"2017-02-25T10:13:40Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"For the patches in the added testcases, we were crashing with:\n\n git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' failed.\n\nAs it turns out, check_preimage() is prepared to handle these conditions,\nso we can remove the assertion.\n\nFound using AFL.\n\nSigned-off-by: Vegard Nossum \n\n---\n\n(I'm fully aware of how it looks to just delete an assertion to \"fix\" a\nbug without any other changes to accomodate the condition that was\nbeing tested for. I am definitely not an expert on this code, but as far\nas I can tell -- both by reviewing and testing the code -- the function\nreally is prepared to handle the case where patch->is_new == 1, as it\nwill always hit another error condition if that is true. I've tried to\nadd more test cases to show what errors you can expect to see instead of\nthe assertion failure when trying to apply these nonsensical patches. If\nyou don't want to remove the assertion for whatever reason, please feel\nfree to take the testcases and add \"# TODO: known breakage\" or whatever.)\n---\n apply.c | 1 -\n t/t4154-apply-git-header.sh | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex cbf7cc7f2..9219d2737 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,\n \tif (!old_name)\n \t\treturn 0;\n \n-\tassert(patch->is_new <= 0);\n \tprevious = previous_patch(state, patch, &status);\n \n \tif (status)\ndiff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh\nindex d651af4a2..c440c48ad 100755\n--- a/t/t4154-apply-git-header.sh\n+++ b/t/t4154-apply-git-header.sh\n@@ -12,4 +12,40 @@ rename new 0\n EOF\n '\n \n+test_expect_success 'apply deleted file mode / new file mode / wrong mode' '\n+\ttest_must_fail git apply << EOF\n+diff --git a/. b/.\n+deleted file mode \n+new file mode \n+EOF\n+'\n+\n+test_expect_success 'apply deleted file mode / new file mode / wrong type' '\n+\tmkdir x &&\n+\tchmod 755 x &&\n+\ttest_must_fail git apply << EOF\n+diff --git a/x b/x\n+deleted file mode 160755\n+new file mode \n+EOF\n+'\n+\n+test_expect_success 'apply deleted file mode / new file mode / already exists' '\n+\ttouch 1 &&\n+\tchmod 644 1 &&\n+\ttest_must_fail git apply << EOF\n+diff --git a/1 b/1\n+deleted file mode 100644\n+new file mode \n+EOF\n+'\n+\n+test_expect_success 'apply new file mode / copy from / nonexistant file' '\n+\ttest_must_fail git apply << EOF\n+diff --git a/. b/.\n+new file mode \n+copy from \n+EOF\n+'\n+\n test_done\n-- \n2.12.0.rc0\n\n"},{"id":"312634","messageId":"E14B054C79E1450F8B41E6477D1D7CD1@PhilipOakley","threadId":"45222","inReplyTo":"20170225101307.24067-1-vegard.nossum@oracle.com","subject":"Re: [PATCH 1/2] apply: guard against renames of non-existant empty files","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-02-25T11:59:33Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Vegard Nossum\" \n> If we have a patch like the one in the new test-case, then we will\n\n\"the one in the new test-case\" needs a clearer reference to the particular \ncase so that future readers will know what it refers to. Noticed while \nbrowsing the commit message..\n\n..reads further; Maybe it's \"AFL (American fuzzy lop) found a failure. Add a \nnew test case and fix the fault\"?\n\n[same for patch 2]\n\n> try to rename a non-existant empty file, i.e. patch->old_name will\n> be NULL. In this case, a NULL entry will be added to fn_table, which\n> is not allowed (a subsequent binary search will die with a NULL\n> pointer dereference).\n>\n> The patch file is completely bogus as it tries to rename something\n> that is known not to exist, so we can throw an error for this.\n>\n> Found using AFL.\n>\n> Signed-off-by: Vegard Nossum \n> ---\n> apply.c | 3 ++-\n> t/t4154-apply-git-header.sh | 15 +++++++++++++++\n> 2 files changed, 17 insertions(+), 1 deletion(-)\n> create mode 100755 t/t4154-apply-git-header.sh\n>\n> diff --git a/apply.c b/apply.c\n> index 0e2caeab9..cbf7cc7f2 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,\n> patch->old_name = xstrdup(patch->def_name);\n> patch->new_name = xstrdup(patch->def_name);\n> }\n> - if (!patch->is_delete && !patch->new_name) {\n> + if ((!patch->is_delete && !patch->new_name) ||\n> + (patch->is_rename && !patch->old_name)) {\n> error(_(\"git diff header lacks filename information \"\n> \"(line %d)\"), state->linenr);\n> return -128;\n> diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh\n> new file mode 100755\n> index 000000000..d651af4a2\n> --- /dev/null\n> +++ b/t/t4154-apply-git-header.sh\n> @@ -0,0 +1,15 @@\n> +#!/bin/sh\n> +\n> +test_description='apply with git/--git headers'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'apply old mode / rename new' '\n> + test_must_fail git apply << EOF\n> +diff --git a/1 b/1\n> +old mode 0\n> +rename new 0\n> +EOF\n> +'\n> +\n> +test_done\n> -- \n> 2.12.0.rc0\n--\nPhilip \n\n"},{"id":"312635","messageId":"0cdd4304-7b71-c38d-21ab-b4e997242bd4@oracle.com","threadId":"45222","inReplyTo":"E14B054C79E1450F8B41E6477D1D7CD1@PhilipOakley","subject":"Re: [PATCH 1/2] apply: guard against renames of non-existant empty files","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-02-25T12:06:47Z","receivedAt":"2017-02-25T12:21:11Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 25/02/2017 12:59, Philip Oakley wrote:\n> From: \"Vegard Nossum\" \n>> If we have a patch like the one in the new test-case, then we will\n>\n> \"the one in the new test-case\" needs a clearer reference to the\n> particular case so that future readers will know what it refers to.\n> Noticed while browsing the commit message..\n\nThere is only one testcase added by this patch, so how is it possibly\nunclear? In what situation would you read a commit message and not even\nthink to glance at the patch for more details?\n\n\nVegard\n"},{"id":"312637","messageId":"36746FDD909546E29F39F1040810DA17@PhilipOakley","threadId":"45222","inReplyTo":"0cdd4304-7b71-c38d-21ab-b4e997242bd4@oracle.com","subject":"Re: [PATCH 1/2] apply: guard against renames of non-existant empty files","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-02-25T12:53:36Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Vegard Nossum\" \n> On 25/02/2017 12:59, Philip Oakley wrote:\n>> From: \"Vegard Nossum\" \n>>> If we have a patch like the one in the new test-case, then we will\n>>\n>> \"the one in the new test-case\" needs a clearer reference to the\n>> particular case so that future readers will know what it refers to.\n>> Noticed while browsing the commit message..\n>\n> There is only one testcase added by this patch, so how is it possibly\n> unclear? In what situation would you read a commit message and not even\n> think to glance at the patch for more details?\n>\nOn initial reading of a commit message, the expectation is that the commit \nwill be about a change from some previous state, so I immediately asked \nmyself, where is that new (recent) test case from.\n\nYou could say \"This patch presents a new test case\" which would straight \naway set the expectation that one should read on to see what its about. It \nwas just that as a reader of the log message I didn't pick up the sense you \nwanted to convey. It's easy to see with hindsight or fore-knowledge.\n\nI, personally, think that bringing the AFL discovery to the fore would help \nin explaining why/how the patch appeared in the first place.\n\nHope that helps explain why I responded.\n\nregards\n\nPhilip \n\n"},{"id":"312664","messageId":"baf195cc-ef81-bbad-4e01-4149498efedb@web.de","threadId":"45222","inReplyTo":"20170225101307.24067-1-vegard.nossum@oracle.com","subject":"Re: [PATCH 1/2] apply: guard against renames of non-existant empty files","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-25T20:51:47Z","receivedAt":"2017-02-25T20:52:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.02.2017 um 11:13 schrieb Vegard Nossum:\n> If we have a patch like the one in the new test-case, then we will\n> try to rename a non-existant empty file, i.e. patch->old_name will\n> be NULL. In this case, a NULL entry will be added to fn_table, which\n> is not allowed (a subsequent binary search will die with a NULL\n> pointer dereference).\n>\n> The patch file is completely bogus as it tries to rename something\n> that is known not to exist, so we can throw an error for this.\n>\n> Found using AFL.\n>\n> Signed-off-by: Vegard Nossum \n> ---\n> apply.c | 3 ++-\n> t/t4154-apply-git-header.sh | 15 +++++++++++++++\n> 2 files changed, 17 insertions(+), 1 deletion(-)\n> create mode 100755 t/t4154-apply-git-header.sh\n>\n> diff --git a/apply.c b/apply.c\n> index 0e2caeab9..cbf7cc7f2 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,\n> \t\t\t\tpatch->old_name = xstrdup(patch->def_name);\n> \t\t\t\tpatch->new_name = xstrdup(patch->def_name);\n> \t\t\t}\n> -\t\t\tif (!patch->is_delete && !patch->new_name) {\n> +\t\t\tif ((!patch->is_delete && !patch->new_name) ||\n> +\t\t\t (patch->is_rename && !patch->old_name)) {\n\nWould it make sense to mirror the previously existing condition and \ncheck for is_new instead? I.e.:\n\n\t\t\tif ((!patch->is_delete && !patch->new_name) ||\n\t\t\t (!patch->is_new && !patch->old_name)) {\n\nor\n\n\t\t\tif (!(patch->is_delete || patch->new_name) ||\n\t\t\t !(patch->is_new || patch->old_name)) {\n\nRené\n"},{"id":"312665","messageId":"a5626d97-e644-65b5-2fd3-41ce870f85a6@web.de","threadId":"45222","inReplyTo":"20170225101307.24067-2-vegard.nossum@oracle.com","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-25T21:21:45Z","receivedAt":"2017-02-25T21:22:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.02.2017 um 11:13 schrieb Vegard Nossum:\n> For the patches in the added testcases, we were crashing with:\n>\n> git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' failed.\n>\n> As it turns out, check_preimage() is prepared to handle these conditions,\n> so we can remove the assertion.\n>\n> Found using AFL.\n>\n> Signed-off-by: Vegard Nossum \n>\n> ---\n>\n> (I'm fully aware of how it looks to just delete an assertion to \"fix\" a\n> bug without any other changes to accomodate the condition that was\n> being tested for. I am definitely not an expert on this code, but as far\n> as I can tell -- both by reviewing and testing the code -- the function\n> really is prepared to handle the case where patch->is_new == 1, as it\n> will always hit another error condition if that is true. I've tried to\n> add more test cases to show what errors you can expect to see instead of\n> the assertion failure when trying to apply these nonsensical patches. If\n> you don't want to remove the assertion for whatever reason, please feel\n> free to take the testcases and add \"# TODO: known breakage\" or whatever.)\n> ---\n> apply.c | 1 -\n> t/t4154-apply-git-header.sh | 36 ++++++++++++++++++++++++++++++++++++\n> 2 files changed, 36 insertions(+), 1 deletion(-)\n>\n> diff --git a/apply.c b/apply.c\n> index cbf7cc7f2..9219d2737 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,\n> \tif (!old_name)\n> \t\treturn 0;\n>\n> -\tassert(patch->is_new <= 0);\n\n5c47f4c6 (builtin-apply: accept patch to an empty file) added that line. \n Its intent was to handle diffs that contain an old name even for a \nfile that's created. Citing from its commit message: \"When we cannot be \nsure by parsing the patch that it is not a creation patch, we shouldn't \ncomplain when if there is no such a file.\" Why not stop complaining \nalso in case we happen to know for sure that it's a creation patch? \nI.e., why not replace the assert() with:\n\n\tif (patch->is_new == 1)\n\t\tgoto is_new;\n\n> \tprevious = previous_patch(state, patch, &status);\n>\n> \tif (status)\n> diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh\n> index d651af4a2..c440c48ad 100755\n> --- a/t/t4154-apply-git-header.sh\n> +++ b/t/t4154-apply-git-header.sh\n> @@ -12,4 +12,40 @@ rename new 0\n> EOF\n> '\n>\n> +test_expect_success 'apply deleted file mode / new file mode / wrong mode' '\n> +\ttest_must_fail git apply << EOF\n> +diff --git a/. b/.\n> +deleted file mode\n> +new file mode\n> +EOF\n> +'\n> +\n> +test_expect_success 'apply deleted file mode / new file mode / wrong type' '\n> +\tmkdir x &&\n> +\tchmod 755 x &&\n> +\ttest_must_fail git apply << EOF\n> +diff --git a/x b/x\n> +deleted file mode 160755\n> +new file mode\n> +EOF\n> +'\n> +\n> +test_expect_success 'apply deleted file mode / new file mode / already exists' '\n> +\ttouch 1 &&\n> +\tchmod 644 1 &&\n> +\ttest_must_fail git apply << EOF\n> +diff --git a/1 b/1\n> +deleted file mode 100644\n> +new file mode\n> +EOF\n> +'\n> +\n> +test_expect_success 'apply new file mode / copy from / nonexistant file' '\n> +\ttest_must_fail git apply << EOF\n> +diff --git a/. b/.\n> +new file mode\n> +copy from\n> +EOF\n> +'\n> +\n> test_done\n>\n"},{"id":"312768","messageId":"xmqqmvd7wgc7.fsf@gitster.mtv.corp.google.com","threadId":"45222","inReplyTo":"a5626d97-e644-65b5-2fd3-41ce870f85a6@web.de","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T20:04:24Z","receivedAt":"2017-02-27T20:11:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> diff --git a/apply.c b/apply.c\n>> index cbf7cc7f2..9219d2737 100644\n>> --- a/apply.c\n>> +++ b/apply.c\n>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,\n>> \tif (!old_name)\n>> \t\treturn 0;\n>>\n>> -\tassert(patch->is_new <= 0);\n>\n> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that\n> line. Its intent was to handle diffs that contain an old name even for\n> a file that's created. Citing from its commit message: \"When we\n> cannot be sure by parsing the patch that it is not a creation patch,\n> we shouldn't complain when if there is no such a file.\" Why not stop\n> complaining also in case we happen to know for sure that it's a\n> creation patch? I.e., why not replace the assert() with:\n>\n> \tif (patch->is_new == 1)\n> \t\tgoto is_new;\n>\n>> \tprevious = previous_patch(state, patch, &status);\n\nWhen the caller does know is_new is true, old_name must be made/left\nNULL. That is the invariant this assert is checking to catch an\nerror in the calling code.\n\nErrors in the patches fed as its input are caught by \"if we do not\nknow if the patch is to add a new path yet, then declare it is, but\nif we do know the patch is _NOT_ adding a new path, barf if that\npath is not there\" and other checks in this function, and changing\nthe assert to \"if already new, then make it a no-op\" defeats the\nwhole point of having an assert (and just removing it is even worse).\n\nThanks.\n"},{"id":"312769","messageId":"xmqqinnvwg2d.fsf@gitster.mtv.corp.google.com","threadId":"45222","inReplyTo":"baf195cc-ef81-bbad-4e01-4149498efedb@web.de","subject":"Re: [PATCH 1/2] apply: guard against renames of non-existant empty files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T20:10:18Z","receivedAt":"2017-02-27T20:11:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Would it make sense to mirror the previously existing condition and\n> check for is_new instead? I.e.:\n>\n> \t\t\tif ((!patch->is_delete && !patch->new_name) ||\n> \t\t\t (!patch->is_new && !patch->old_name)) {\n>\n\nYes, probably.\n\n> or\n>\n> \t\t\tif (!(patch->is_delete || patch->new_name) ||\n> \t\t\t !(patch->is_new || patch->old_name)) {\n\nThis happens after calling parse_git_header() so we should know the\nactual value of is_delete and is_new by now (instead of mistaking\n-1 aka \"unknown\" as true), so this rewrite would also be OK.\n\n"},{"id":"312787","messageId":"f191e3a8-a55b-7030-ebbb-3f46c74fdc94@web.de","threadId":"45222","inReplyTo":"xmqqmvd7wgc7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-27T22:18:03Z","receivedAt":"2017-02-27T22:20:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.2017 um 21:04 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>>> diff --git a/apply.c b/apply.c\n>>> index cbf7cc7f2..9219d2737 100644\n>>> --- a/apply.c\n>>> +++ b/apply.c\n>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,\n>>> \tif (!old_name)\n>>> \t\treturn 0;\n>>>\n>>> -\tassert(patch->is_new <= 0);\n>>\n>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that\n>> line. Its intent was to handle diffs that contain an old name even for\n>> a file that's created. Citing from its commit message: \"When we\n>> cannot be sure by parsing the patch that it is not a creation patch,\n>> we shouldn't complain when if there is no such a file.\" Why not stop\n>> complaining also in case we happen to know for sure that it's a\n>> creation patch? I.e., why not replace the assert() with:\n>>\n>> \tif (patch->is_new == 1)\n>> \t\tgoto is_new;\n>>\n>>> \tprevious = previous_patch(state, patch, &status);\n>\n> When the caller does know is_new is true, old_name must be made/left\n> NULL. That is the invariant this assert is checking to catch an\n> error in the calling code.\n\nThere are some places in apply.c that set ->is_new to 1, but none of \nthem set ->old_name to NULL at the same time.\n\nHaving to keep these two members in sync sounds iffy anyway. Perhaps \naccessors can help, e.g. a setter which frees old_name when is_new is \nset to 1, or a getter which returns NULL for old_name if is_new is 1.\n\nRené\n"},{"id":"312788","messageId":"ed46f675-559a-88a3-cf97-d0ba7cf3112f@web.de","threadId":"45222","inReplyTo":"xmqqinnvwg2d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] apply: guard against renames of non-existant empty files","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-27T22:18:16Z","receivedAt":"2017-02-27T22:20:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.2017 um 21:10 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Would it make sense to mirror the previously existing condition and\n>> check for is_new instead? I.e.:\n>>\n>> \t\t\tif ((!patch->is_delete && !patch->new_name) ||\n>> \t\t\t (!patch->is_new && !patch->old_name)) {\n>>\n>\n> Yes, probably.\n>\n>> or\n>>\n>> \t\t\tif (!(patch->is_delete || patch->new_name) ||\n>> \t\t\t !(patch->is_new || patch->old_name)) {\n>\n> This happens after calling parse_git_header() so we should know the\n> actual value of is_delete and is_new by now (instead of mistaking\n> -1 aka \"unknown\" as true), so this rewrite would also be OK.\n\nThe two variants are logically equivalent -- (!a && !b) == !(a || b). I \nwonder if the second one may be harder to read, though.\n\nRené\n"},{"id":"312793","messageId":"xmqq1sujnu1g.fsf@gitster.mtv.corp.google.com","threadId":"45222","inReplyTo":"f191e3a8-a55b-7030-ebbb-3f46c74fdc94@web.de","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T22:33:15Z","receivedAt":"2017-02-27T22:35:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>>> diff --git a/apply.c b/apply.c\n>>>> index cbf7cc7f2..9219d2737 100644\n>>>> --- a/apply.c\n>>>> +++ b/apply.c\n>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,\n>>>> \tif (!old_name)\n>>>> \t\treturn 0;\n>>>>\n>>>> -\tassert(patch->is_new <= 0);\n>>>\n>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that\n>>> line. Its intent was to handle diffs that contain an old name even for\n>>> a file that's created. Citing from its commit message: \"When we\n>>> cannot be sure by parsing the patch that it is not a creation patch,\n>>> we shouldn't complain when if there is no such a file.\" Why not stop\n>>> complaining also in case we happen to know for sure that it's a\n>>> creation patch? I.e., why not replace the assert() with:\n>>>\n>>> \tif (patch->is_new == 1)\n>>> \t\tgoto is_new;\n>>>\n>>>> \tprevious = previous_patch(state, patch, &status);\n>>\n>> When the caller does know is_new is true, old_name must be made/left\n>> NULL. That is the invariant this assert is checking to catch an\n>> error in the calling code.\n>\n> There are some places in apply.c that set ->is_new to 1, but none of\n> them set ->old_name to NULL at the same time.\n\nI thought all of these are flipping ->is_new that used to be -1\n(unknown) to (now we know it is new), and sets only new_name without\ndoing anything to old_name, because they know originally both names\nare set to NULL.\n\n> Having to keep these two members in sync sounds iffy anyway. Perhaps\n> accessors can help, e.g. a setter which frees old_name when is_new is\n> set to 1, or a getter which returns NULL for old_name if is_new is 1.\n\nDefinitely, the setter would make it harder to make the mistake.\n"},{"id":"312828","messageId":"05fe5800-ebc0-76d7-579d-77f64a851fc1@web.de","threadId":"45222","inReplyTo":"xmqq1sujnu1g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-28T10:50:51Z","receivedAt":"2017-02-28T10:57:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.2017 um 23:33 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:\n>>> René Scharfe writes:\n>>>\n>>>>> diff --git a/apply.c b/apply.c\n>>>>> index cbf7cc7f2..9219d2737 100644\n>>>>> --- a/apply.c\n>>>>> +++ b/apply.c\n>>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,\n>>>>> \tif (!old_name)\n>>>>> \t\treturn 0;\n>>>>>\n>>>>> -\tassert(patch->is_new <= 0);\n>>>>\n>>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that\n>>>> line. Its intent was to handle diffs that contain an old name even for\n>>>> a file that's created. Citing from its commit message: \"When we\n>>>> cannot be sure by parsing the patch that it is not a creation patch,\n>>>> we shouldn't complain when if there is no such a file.\" Why not stop\n>>>> complaining also in case we happen to know for sure that it's a\n>>>> creation patch? I.e., why not replace the assert() with:\n>>>>\n>>>> \tif (patch->is_new == 1)\n>>>> \t\tgoto is_new;\n>>>>\n>>>>> \tprevious = previous_patch(state, patch, &status);\n>>>\n>>> When the caller does know is_new is true, old_name must be made/left\n>>> NULL. That is the invariant this assert is checking to catch an\n>>> error in the calling code.\n>>\n>> There are some places in apply.c that set ->is_new to 1, but none of\n>> them set ->old_name to NULL at the same time.\n> \n> I thought all of these are flipping ->is_new that used to be -1\n> (unknown) to (now we know it is new), and sets only new_name without\n> doing anything to old_name, because they know originally both names\n> are set to NULL.\n> \n>> Having to keep these two members in sync sounds iffy anyway. Perhaps\n>> accessors can help, e.g. a setter which frees old_name when is_new is\n>> set to 1, or a getter which returns NULL for old_name if is_new is 1.\n> \n> Definitely, the setter would make it harder to make the mistake.\n\nWhen I added setters, apply started to passed NULL to unlink(2) and\nrmdir(2) in some of the new tests, which still failed.\n\nThat's because three of the diffs trigger both gitdiff_delete(), which\nsets is_delete and old_name, and gitdiff_newfile(), which sets is_new\nand new_name. Create and delete equals move, right? Or should we\nerror out at this point already?\n\nThe last new diff adds a new file that is copied. Sounds impossible.\nHow about something like this, which forbids combinations that make no\nsense. Hope it's not too strict; at least all tests succeed.\n\n---\n apply.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 61 insertions(+), 18 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 21b0bebec5..6cb6860511 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -197,6 +197,14 @@ struct fragment {\n #define BINARY_DELTA_DEFLATED\t1\n #define BINARY_LITERAL_DEFLATED 2\n \n+enum patch_type {\n+\tCHANGE,\n+\tCREATE,\n+\tDELETE,\n+\tRENAME,\n+\tCOPY\n+};\n+\n /*\n * This represents a \"patch\" to a file, both metainfo changes\n * such as creation/deletion, filemode and content changes represented\n@@ -205,6 +213,7 @@ struct fragment {\n struct patch {\n \tchar *new_name, *old_name, *def_name;\n \tunsigned int old_mode, new_mode;\n+\tenum patch_type type;\n \tint is_new, is_delete;\t/* -1 = unknown, 0 = false, 1 = true */\n \tint rejected;\n \tunsigned ws_rule;\n@@ -229,6 +238,36 @@ struct patch {\n \tstruct object_id threeway_stage[3];\n };\n \n+static int set_patch_type(struct patch *patch, enum patch_type type)\n+{\n+\tif (patch->type != CHANGE && patch->type != type)\n+\t\treturn error(_(\"conflicting patch types\"));\n+\tpatch->type = type;\n+\tswitch (type) {\n+\tcase CHANGE:\n+\t\tbreak;\n+\tcase CREATE:\n+\t\tpatch->is_new = 1;\n+\t\tpatch->is_delete = 0;\n+\t\tfree(patch->old_name);\n+\t\tpatch->old_name = NULL;\n+\t\tbreak;\n+\tcase DELETE:\n+\t\tpatch->is_new = 0;\n+\t\tpatch->is_delete = 1;\n+\t\tfree(patch->new_name);\n+\t\tpatch->new_name = NULL;\n+\t\tbreak;\n+\tcase RENAME:\n+\t\tpatch->is_rename = 1;\n+\t\tbreak;\n+\tcase COPY:\n+\t\tpatch->is_copy = 1;\n+\t\tbreak;\n+\t}\n+\treturn 0;\n+}\n+\n static void free_fragment_list(struct fragment *list)\n {\n \twhile (list) {\n@@ -907,13 +946,13 @@ static int parse_traditional_patch(struct apply_state *state,\n \t\t}\n \t}\n \tif (is_dev_null(first)) {\n-\t\tpatch->is_new = 1;\n-\t\tpatch->is_delete = 0;\n+\t\tif (set_patch_type(patch, CREATE))\n+\t\t\treturn -1;\n \t\tname = find_name_traditional(state, second, NULL, state->p_value);\n \t\tpatch->new_name = name;\n \t} else if (is_dev_null(second)) {\n-\t\tpatch->is_new = 0;\n-\t\tpatch->is_delete = 1;\n+\t\tif (set_patch_type(patch, DELETE))\n+\t\t\treturn -1;\n \t\tname = find_name_traditional(state, first, NULL, state->p_value);\n \t\tpatch->old_name = name;\n \t} else {\n@@ -922,12 +961,12 @@ static int parse_traditional_patch(struct apply_state *state,\n \t\tname = find_name_traditional(state, second, first_name, state->p_value);\n \t\tfree(first_name);\n \t\tif (has_epoch_timestamp(first)) {\n-\t\t\tpatch->is_new = 1;\n-\t\t\tpatch->is_delete = 0;\n+\t\t\tif (set_patch_type(patch, CREATE))\n+\t\t\t\treturn -1;\n \t\t\tpatch->new_name = name;\n \t\t} else if (has_epoch_timestamp(second)) {\n-\t\t\tpatch->is_new = 0;\n-\t\t\tpatch->is_delete = 1;\n+\t\t\tif (set_patch_type(patch, DELETE))\n+\t\t\t\treturn -1;\n \t\t\tpatch->old_name = name;\n \t\t} else {\n \t\t\tpatch->old_name = name;\n@@ -1031,7 +1070,8 @@ static int gitdiff_delete(struct apply_state *state,\n \t\t\t const char *line,\n \t\t\t struct patch *patch)\n {\n-\tpatch->is_delete = 1;\n+\tif (set_patch_type(patch, DELETE))\n+\t\treturn -1;\n \tfree(patch->old_name);\n \tpatch->old_name = xstrdup_or_null(patch->def_name);\n \treturn gitdiff_oldmode(state, line, patch);\n@@ -1041,7 +1081,8 @@ static int gitdiff_newfile(struct apply_state *state,\n \t\t\t const char *line,\n \t\t\t struct patch *patch)\n {\n-\tpatch->is_new = 1;\n+\tif (set_patch_type(patch, CREATE))\n+\t\treturn -1;\n \tfree(patch->new_name);\n \tpatch->new_name = xstrdup_or_null(patch->def_name);\n \treturn gitdiff_newmode(state, line, patch);\n@@ -1051,7 +1092,8 @@ static int gitdiff_copysrc(struct apply_state *state,\n \t\t\t const char *line,\n \t\t\t struct patch *patch)\n {\n-\tpatch->is_copy = 1;\n+\tif (set_patch_type(patch, COPY))\n+\t\treturn -1;\n \tfree(patch->old_name);\n \tpatch->old_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);\n \treturn 0;\n@@ -1061,7 +1103,8 @@ static int gitdiff_copydst(struct apply_state *state,\n \t\t\t const char *line,\n \t\t\t struct patch *patch)\n {\n-\tpatch->is_copy = 1;\n+\tif (set_patch_type(patch, COPY))\n+\t\treturn -1;\n \tfree(patch->new_name);\n \tpatch->new_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);\n \treturn 0;\n@@ -1071,7 +1114,8 @@ static int gitdiff_renamesrc(struct apply_state *state,\n \t\t\t const char *line,\n \t\t\t struct patch *patch)\n {\n-\tpatch->is_rename = 1;\n+\tif (set_patch_type(patch, RENAME))\n+\t\treturn -1;\n \tfree(patch->old_name);\n \tpatch->old_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);\n \treturn 0;\n@@ -1081,7 +1125,8 @@ static int gitdiff_renamedst(struct apply_state *state,\n \t\t\t const char *line,\n \t\t\t struct patch *patch)\n {\n-\tpatch->is_rename = 1;\n+\tif (set_patch_type(patch, RENAME))\n+\t\treturn -1;\n \tfree(patch->new_name);\n \tpatch->new_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);\n \treturn 0;\n@@ -3704,10 +3749,8 @@ static int check_preimage(struct apply_state *state,\n \treturn 0;\n \n is_new:\n-\tpatch->is_new = 1;\n-\tpatch->is_delete = 0;\n-\tfree(patch->old_name);\n-\tpatch->old_name = NULL;\n+\tif (set_patch_type(patch, CREATE))\n+\t\treturn -1;\n \treturn 0;\n }\n \n-- \n2.12.0\n"},{"id":"323348","messageId":"e9c9c1f9-a8b3-95ac-e74e-c82e52de5b73@web.de","threadId":"45222","inReplyTo":"ed46f675-559a-88a3-cf97-d0ba7cf3112f@web.de","subject":"Re: [PATCH 1/2] apply: guard against renames of non-existant empty files","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-06-27T17:03:30Z","receivedAt":"2017-06-27T17:03:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.2017 um 23:18 schrieb René Scharfe:\n> Am 27.02.2017 um 21:10 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>> Would it make sense to mirror the previously existing condition and\n>>> check for is_new instead? I.e.:\n>>>\n>>> if ((!patch->is_delete && !patch->new_name) ||\n>>> (!patch->is_new && !patch->old_name)) {\n>>>\n>>\n>> Yes, probably.\n\nSo let's actually do it!\n\n-- >8 --\nSubject: [PATCH] apply: check git diffs for missing old filenames\n\n2c93286a (fix \"git apply --index ...\" not to deref NULL) added a check\nfor git patches missing a +++ line, preventing a segfault. Check for\nmissing --- lines as well, and add a test for each case.\n\nFound by Vegard Nossum using AFL.\n\nOriginal-patch-by: Vegard Nossum \nSigned-off-by: Rene Scharfe \n---\n apply.c | 3 ++-\n t/t4133-apply-filenames.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex b963d7d8fb..8cd6435c74 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1575,7 +1575,8 @@ static int find_header(struct apply_state *state,\n \t\t\t\tpatch->old_name = xstrdup(patch->def_name);\n \t\t\t\tpatch->new_name = xstrdup(patch->def_name);\n \t\t\t}\n-\t\t\tif (!patch->is_delete && !patch->new_name) {\n+\t\t\tif ((!patch->new_name && !patch->is_delete) ||\n+\t\t\t (!patch->old_name && !patch->is_new)) {\n \t\t\t\terror(_(\"git diff header lacks filename information \"\n \t\t\t\t\t \"(line %d)\"), state->linenr);\n \t\t\t\treturn -128;\ndiff --git a/t/t4133-apply-filenames.sh b/t/t4133-apply-filenames.sh\nindex 2ecb4216b7..c5ed3b17c4 100755\n--- a/t/t4133-apply-filenames.sh\n+++ b/t/t4133-apply-filenames.sh\n@@ -35,4 +35,28 @@ test_expect_success 'apply diff with inconsistent filenames in headers' '\n \ttest_i18ngrep \"inconsistent old filename\" err\n '\n \n+test_expect_success 'apply diff with new filename missing from headers' '\n+\tcat >missing_new_filename.diff <<-\\EOF &&\n+\tdiff --git a/f b/f\n+\tindex 0000000..d00491f\n+\t--- a/f\n+\t@@ -0,0 +1 @@\n+\t+1\n+\tEOF\n+\ttest_must_fail git apply missing_new_filename.diff 2>err &&\n+\ttest_i18ngrep \"lacks filename information\" err\n+'\n+\n+test_expect_success 'apply diff with old filename missing from headers' '\n+\tcat >missing_old_filename.diff <<-\\EOF &&\n+\tdiff --git a/f b/f\n+\tindex d00491f..0000000\n+\t+++ b/f\n+\t@@ -1 +0,0 @@\n+\t-1\n+\tEOF\n+\ttest_must_fail git apply missing_old_filename.diff 2>err &&\n+\ttest_i18ngrep \"lacks filename information\" err\n+'\n+\n test_done\n-- \n2.13.2\n"},{"id":"323349","messageId":"5128cdf1-39fc-59ca-5640-801777bac2fa@web.de","threadId":"45222","inReplyTo":"05fe5800-ebc0-76d7-579d-77f64a851fc1@web.de","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-06-27T17:03:39Z","receivedAt":"2017-06-27T17:03:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.02.2017 um 11:50 schrieb René Scharfe:\n> Am 27.02.2017 um 23:33 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:\n>>>> René Scharfe writes:\n>>>>\n>>>>>> diff --git a/apply.c b/apply.c\n>>>>>> index cbf7cc7f2..9219d2737 100644\n>>>>>> --- a/apply.c\n>>>>>> +++ b/apply.c\n>>>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,\n>>>>>> \tif (!old_name)\n>>>>>> \t\treturn 0;\n>>>>>>\n>>>>>> -\tassert(patch->is_new <= 0);\n>>>>>\n>>>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that\n>>>>> line. Its intent was to handle diffs that contain an old name even for\n>>>>> a file that's created. Citing from its commit message: \"When we\n>>>>> cannot be sure by parsing the patch that it is not a creation patch,\n>>>>> we shouldn't complain when if there is no such a file.\" Why not stop\n>>>>> complaining also in case we happen to know for sure that it's a\n>>>>> creation patch? I.e., why not replace the assert() with:\n>>>>>\n>>>>> \tif (patch->is_new == 1)\n>>>>> \t\tgoto is_new;\n>>>>>\n>>>>>> \tprevious = previous_patch(state, patch, &status);\n>>>>\n>>>> When the caller does know is_new is true, old_name must be made/left\n>>>> NULL. That is the invariant this assert is checking to catch an\n>>>> error in the calling code.\n>>>\n>>> There are some places in apply.c that set ->is_new to 1, but none of\n>>> them set ->old_name to NULL at the same time.\n>>\n>> I thought all of these are flipping ->is_new that used to be -1\n>> (unknown) to (now we know it is new), and sets only new_name without\n>> doing anything to old_name, because they know originally both names\n>> are set to NULL.\n>>\n>>> Having to keep these two members in sync sounds iffy anyway. Perhaps\n>>> accessors can help, e.g. a setter which frees old_name when is_new is\n>>> set to 1, or a getter which returns NULL for old_name if is_new is 1.\n>>\n>> Definitely, the setter would make it harder to make the mistake.\n> \n> When I added setters, apply started to passed NULL to unlink(2) and\n> rmdir(2) in some of the new tests, which still failed.\n> \n> That's because three of the diffs trigger both gitdiff_delete(), which\n> sets is_delete and old_name, and gitdiff_newfile(), which sets is_new\n> and new_name. Create and delete equals move, right? Or should we\n> error out at this point already?\n> \n> The last new diff adds a new file that is copied. Sounds impossible.\n> How about something like this, which forbids combinations that make no\n> sense. Hope it's not too strict; at least all tests succeed.\n> \n> ---\n> apply.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------\n> 1 file changed, 61 insertions(+), 18 deletions(-)\n\nThought a bit more about it, and as a result here's a simpler approach:\n\n-- >8 --\nSubject: [PATCH] apply: check git diffs for mutually exclusive header lines\n\nA file can either be added, removed, copied, or renamed, but no two of\nthese actions can be done by the same patch. Some of these combinations\nprovoke error messages due to missing file names, and some are only\ncaught by an assertion. Check git patches already as they are parsed\nand report conflicting lines on sight.\n\nFound by Vegard Nossum using AFL.\n\nReported-by: Vegard Nossum \nSigned-off-by: Rene Scharfe \n---\n apply.c | 14 ++++++++++++++\n apply.h | 1 +\n t/t4136-apply-check.sh | 18 ++++++++++++++++++\n 3 files changed, 33 insertions(+)\n\ndiff --git a/apply.c b/apply.c\nindex 8cd6435c74..8a5e44c474 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1312,6 +1312,18 @@ static char *git_header_name(struct apply_state *state,\n \t}\n }\n \n+static int check_header_line(struct apply_state *state, struct patch *patch)\n+{\n+\tint extensions = (patch->is_delete == 1) + (patch->is_new == 1) +\n+\t\t\t (patch->is_rename == 1) + (patch->is_copy == 1);\n+\tif (extensions > 1)\n+\t\treturn error(_(\"inconsistent header lines %d and %d\"),\n+\t\t\t state->extension_linenr, state->linenr);\n+\tif (extensions && !state->extension_linenr)\n+\t\tstate->extension_linenr = state->linenr;\n+\treturn 0;\n+}\n+\n /* Verify that we recognize the lines following a git header */\n static int parse_git_header(struct apply_state *state,\n \t\t\t const char *line,\n@@ -1378,6 +1390,8 @@ static int parse_git_header(struct apply_state *state,\n \t\t\tres = p->fn(state, line + oplen, patch);\n \t\t\tif (res < 0)\n \t\t\t\treturn -1;\n+\t\t\tif (check_header_line(state, patch))\n+\t\t\t\treturn -1;\n \t\t\tif (res > 0)\n \t\t\t\treturn offset;\n \t\t\tbreak;\ndiff --git a/apply.h b/apply.h\nindex b3d6783d55..b52078b486 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -79,6 +79,7 @@ struct apply_state {\n \n \t/* Various \"current state\" */\n \tint linenr; /* current line number */\n+\tint extension_linenr; /* first line specifying delete/new/rename/copy */\n \tstruct string_list symlink_changes; /* we have to track symlinks */\n \n \t/*\ndiff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh\nindex 4b0a374b63..6d92872318 100755\n--- a/t/t4136-apply-check.sh\n+++ b/t/t4136-apply-check.sh\n@@ -29,4 +29,22 @@ test_expect_success 'apply exits non-zero with no-op patch' '\n \ttest_must_fail git apply --check input\n '\n \n+test_expect_success 'invalid combination: create and copy' '\n+\ttest_must_fail git apply --check - <<-\\EOF\n+\tdiff --git a/1 b/2\n+\tnew file mode 100644\n+\tcopy from 1\n+\tcopy to 2\n+\tEOF\n+'\n+\n+test_expect_success 'invalid combination: create and rename' '\n+\ttest_must_fail git apply --check - <<-\\EOF\n+\tdiff --git a/1 b/2\n+\tnew file mode 100644\n+\trename from 1\n+\trename to 2\n+\tEOF\n+'\n+\n test_done\n-- \n2.13.2\n"},{"id":"323350","messageId":"ef6886c0-d567-ced1-3caf-98bd5557c580@web.de","threadId":"45222","inReplyTo":"20170225101307.24067-2-vegard.nossum@oracle.com","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-06-27T17:03:47Z","receivedAt":"2017-06-27T17:04:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.02.2017 um 11:13 schrieb Vegard Nossum:\n> For the patches in the added testcases, we were crashing with:\n> \n> git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' failed.\n\n\n> diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh\n> index d651af4a2..c440c48ad 100755\n> --- a/t/t4154-apply-git-header.sh\n> +++ b/t/t4154-apply-git-header.sh\n> @@ -12,4 +12,40 @@ rename new 0\n> EOF\n> '\n> \n> +test_expect_success 'apply deleted file mode / new file mode / wrong mode' '\n> +\ttest_must_fail git apply << EOF\n> +diff --git a/. b/.\n> +deleted file mode\n> +new file mode\n> +EOF\n> +'\n\n-- >8 --\nSubject: [PATCH] apply: check git diffs for invalid file modes\n\nAn empty string as mode specification is accepted silently by git apply,\nas Vegard Nossum found out using AFL. It's interpreted as zero. Reject\nsuch bogus file modes, and only accept ones consisting exclusively of\noctal digits.\n\nReported-by: Vegard Nossum \nSigned-off-by: Rene Scharfe \n---\n apply.c | 17 ++++++++++++-----\n t/t4129-apply-samemode.sh | 16 +++++++++++++++-\n 2 files changed, 27 insertions(+), 6 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 8a5e44c474..db38bc3cdd 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1001,20 +1001,27 @@ static int gitdiff_newname(struct apply_state *state,\n \t\t\t\t DIFF_NEW_NAME);\n }\n \n+static int parse_mode_line(const char *line, int linenr, unsigned int *mode)\n+{\n+\tchar *end;\n+\t*mode = strtoul(line, &end, 8);\n+\tif (end == line || !isspace(*end))\n+\t\treturn error(_(\"invalid mode on line %d: %s\"), linenr, line);\n+\treturn 0;\n+}\n+\n static int gitdiff_oldmode(struct apply_state *state,\n \t\t\t const char *line,\n \t\t\t struct patch *patch)\n {\n-\tpatch->old_mode = strtoul(line, NULL, 8);\n-\treturn 0;\n+\treturn parse_mode_line(line, state->linenr, &patch->old_mode);\n }\n \n static int gitdiff_newmode(struct apply_state *state,\n \t\t\t const char *line,\n \t\t\t struct patch *patch)\n {\n-\tpatch->new_mode = strtoul(line, NULL, 8);\n-\treturn 0;\n+\treturn parse_mode_line(line, state->linenr, &patch->new_mode);\n }\n \n static int gitdiff_delete(struct apply_state *state,\n@@ -1128,7 +1135,7 @@ static int gitdiff_index(struct apply_state *state,\n \tmemcpy(patch->new_sha1_prefix, line, len);\n \tpatch->new_sha1_prefix[len] = 0;\n \tif (*ptr == ' ')\n-\t\tpatch->old_mode = strtoul(ptr+1, NULL, 8);\n+\t\treturn gitdiff_oldmode(state, ptr + 1, patch);\n \treturn 0;\n }\n \ndiff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh\nindex c268298eaf..5cdd76dfa7 100755\n--- a/t/t4129-apply-samemode.sh\n+++ b/t/t4129-apply-samemode.sh\n@@ -13,7 +13,9 @@ test_expect_success setup '\n \techo modified >file &&\n \tgit diff --stat -p >patch-0.txt &&\n \tchmod +x file &&\n-\tgit diff --stat -p >patch-1.txt\n+\tgit diff --stat -p >patch-1.txt &&\n+\tsed \"s/^\\(new mode \\).*/\\1/\" patch-empty-mode.txt &&\n+\tsed \"s/^\\(new mode \\).*/\\1garbage/\" patch-bogus-mode.txt\n '\n \n test_expect_success FILEMODE 'same mode (no index)' '\n@@ -59,4 +61,16 @@ test_expect_success FILEMODE 'mode update (index only)' '\n \tgit ls-files -s file | grep \"^100755\"\n '\n \n+test_expect_success FILEMODE 'empty mode is rejected' '\n+\tgit reset --hard &&\n+\ttest_must_fail git apply patch-empty-mode.txt 2>err &&\n+\ttest_i18ngrep \"invalid mode\" err\n+'\n+\n+test_expect_success FILEMODE 'bogus mode is rejected' '\n+\tgit reset --hard &&\n+\ttest_must_fail git apply patch-bogus-mode.txt 2>err &&\n+\ttest_i18ngrep \"invalid mode\" err\n+'\n+\n test_done\n-- \n2.13.2\n"},{"id":"323361","messageId":"xmqqshil1ex1.fsf@gitster.mtv.corp.google.com","threadId":"45222","inReplyTo":"5128cdf1-39fc-59ca-5640-801777bac2fa@web.de","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-27T18:08:58Z","receivedAt":"2017-06-27T18:09:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Thought a bit more about it, and as a result here's a simpler approach:\n>\n> -- >8 --\n> Subject: [PATCH] apply: check git diffs for mutually exclusive header lines\n>\n> A file can either be added, removed, copied, or renamed, but no two of\n> these actions can be done by the same patch. Some of these combinations\n> provoke error messages due to missing file names, and some are only\n> caught by an assertion. Check git patches already as they are parsed\n> and report conflicting lines on sight.\n>\n> Found by Vegard Nossum using AFL.\n>\n> Reported-by: Vegard Nossum \n> Signed-off-by: Rene Scharfe \n> ---\n> apply.c | 14 ++++++++++++++\n> apply.h | 1 +\n> t/t4136-apply-check.sh | 18 ++++++++++++++++++\n> 3 files changed, 33 insertions(+)\n>\n> diff --git a/apply.c b/apply.c\n> index 8cd6435c74..8a5e44c474 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1312,6 +1312,18 @@ static char *git_header_name(struct apply_state *state,\n> \t}\n> }\n> \n> +static int check_header_line(struct apply_state *state, struct patch *patch)\n> +{\n> +\tint extensions = (patch->is_delete == 1) + (patch->is_new == 1) +\n> +\t\t\t (patch->is_rename == 1) + (patch->is_copy == 1);\n> +\tif (extensions > 1)\n> +\t\treturn error(_(\"inconsistent header lines %d and %d\"),\n> +\t\t\t state->extension_linenr, state->linenr);\n> +\tif (extensions && !state->extension_linenr)\n> +\t\tstate->extension_linenr = state->linenr;\n\nOK. I wondered briefly what happens if the first git_header that\nsets one of the extensions can be at line 0 (calusng\nstate->extension_linenr to be set to 0), but even in that case, the\nsecond problematic one will correctly report the 0th and its own\nline as culprit, so this is OK. It makes me question if there is\nany point checking !state->extension_linenr in the if() statement,\nthough.\n\n"},{"id":"323382","messageId":"1374711c-2cf5-ae8e-16e0-7c10be253a08@web.de","threadId":"45222","inReplyTo":"xmqqshil1ex1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-06-27T20:20:37Z","receivedAt":"2017-06-27T20:21:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.06.2017 um 20:08 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> Thought a bit more about it, and as a result here's a simpler approach:\n>>\n>> -- >8 --\n>> Subject: [PATCH] apply: check git diffs for mutually exclusive header lines\n>>\n>> A file can either be added, removed, copied, or renamed, but no two of\n>> these actions can be done by the same patch. Some of these combinations\n>> provoke error messages due to missing file names, and some are only\n>> caught by an assertion. Check git patches already as they are parsed\n>> and report conflicting lines on sight.\n>>\n>> Found by Vegard Nossum using AFL.\n>>\n>> Reported-by: Vegard Nossum \n>> Signed-off-by: Rene Scharfe \n>> ---\n>> apply.c | 14 ++++++++++++++\n>> apply.h | 1 +\n>> t/t4136-apply-check.sh | 18 ++++++++++++++++++\n>> 3 files changed, 33 insertions(+)\n>>\n>> diff --git a/apply.c b/apply.c\n>> index 8cd6435c74..8a5e44c474 100644\n>> --- a/apply.c\n>> +++ b/apply.c\n>> @@ -1312,6 +1312,18 @@ static char *git_header_name(struct apply_state *state,\n>> \t}\n>> }\n>> \n>> +static int check_header_line(struct apply_state *state, struct patch *patch)\n>> +{\n>> +\tint extensions = (patch->is_delete == 1) + (patch->is_new == 1) +\n>> +\t\t\t (patch->is_rename == 1) + (patch->is_copy == 1);\n>> +\tif (extensions > 1)\n>> +\t\treturn error(_(\"inconsistent header lines %d and %d\"),\n>> +\t\t\t state->extension_linenr, state->linenr);\n>> +\tif (extensions && !state->extension_linenr)\n>> +\t\tstate->extension_linenr = state->linenr;\n> \n> OK. I wondered briefly what happens if the first git_header that\n> sets one of the extensions can be at line 0 (calusng\n> state->extension_linenr to be set to 0), but even in that case, the\n> second problematic one will correctly report the 0th and its own\n> line as culprit, so this is OK. It makes me question if there is\n> any point checking !state->extension_linenr in the if() statement,\n> though.\n\nIt makes sure that ->extension_linenr is set to the first line number of\nan extension (like, say, \"copy from\") and not advanced again (e.g. when\nwe hit \"copy to\", or more importantly some line like \"similarity index\"\nwhich does not set one of the extension bits and thus can't actually\ncause a conflict).\n\nHmm, pondering that, it seems I forgot to reset its value after each\npatch. Or better just move it into struct patch, next to the extension\nbits:\n\n-- >8 --\nSubject: fixup! apply: check git diffs for mutually exclusive header lines\n---\n apply.c | 7 ++++---\n apply.h | 1 -\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex db38bc3cdd..c442b89328 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -211,6 +211,7 @@ struct patch {\n \tunsigned ws_rule;\n \tint lines_added, lines_deleted;\n \tint score;\n+\tint extension_linenr; /* first line specifying delete/new/rename/copy */\n \tunsigned int is_toplevel_relative:1;\n \tunsigned int inaccurate_eof:1;\n \tunsigned int is_binary:1;\n@@ -1325,9 +1326,9 @@ static int check_header_line(struct apply_state *state, struct patch *patch)\n \t\t\t (patch->is_rename == 1) + (patch->is_copy == 1);\n \tif (extensions > 1)\n \t\treturn error(_(\"inconsistent header lines %d and %d\"),\n-\t\t\t state->extension_linenr, state->linenr);\n-\tif (extensions && !state->extension_linenr)\n-\t\tstate->extension_linenr = state->linenr;\n+\t\t\t patch->extension_linenr, state->linenr);\n+\tif (extensions && !patch->extension_linenr)\n+\t\tpatch->extension_linenr = state->linenr;\n \treturn 0;\n }\n \ndiff --git a/apply.h b/apply.h\nindex b52078b486..b3d6783d55 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -79,7 +79,6 @@ struct apply_state {\n \n \t/* Various \"current state\" */\n \tint linenr; /* current line number */\n-\tint extension_linenr; /* first line specifying delete/new/rename/copy */\n \tstruct string_list symlink_changes; /* we have to track symlinks */\n \n \t/*\n-- \n2.13.2\n"},{"id":"323397","messageId":"xmqqbmp9yutm.fsf@gitster.mtv.corp.google.com","threadId":"45222","inReplyTo":"1374711c-2cf5-ae8e-16e0-7c10be253a08@web.de","subject":"Re: [PATCH 2/2] apply: handle assertion failure gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-27T21:39:01Z","receivedAt":"2017-06-27T21:39:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Hmm, pondering that, it seems I forgot to reset its value after each\n> patch. Or better just move it into struct patch, next to the extension\n> bits:\n\nGood catch.\n\n> -- >8 --\n> Subject: fixup! apply: check git diffs for mutually exclusive header lines\n> ---\n> apply.c | 7 ++++---\n> apply.h | 1 -\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index db38bc3cdd..c442b89328 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -211,6 +211,7 @@ struct patch {\n> \tunsigned ws_rule;\n> \tint lines_added, lines_deleted;\n> \tint score;\n> +\tint extension_linenr; /* first line specifying delete/new/rename/copy */\n> \tunsigned int is_toplevel_relative:1;\n> \tunsigned int inaccurate_eof:1;\n> \tunsigned int is_binary:1;\n> @@ -1325,9 +1326,9 @@ static int check_header_line(struct apply_state *state, struct patch *patch)\n> \t\t\t (patch->is_rename == 1) + (patch->is_copy == 1);\n> \tif (extensions > 1)\n> \t\treturn error(_(\"inconsistent header lines %d and %d\"),\n> -\t\t\t state->extension_linenr, state->linenr);\n> -\tif (extensions && !state->extension_linenr)\n> -\t\tstate->extension_linenr = state->linenr;\n> +\t\t\t patch->extension_linenr, state->linenr);\n> +\tif (extensions && !patch->extension_linenr)\n> +\t\tpatch->extension_linenr = state->linenr;\n> \treturn 0;\n> }\n> \n> diff --git a/apply.h b/apply.h\n> index b52078b486..b3d6783d55 100644\n> --- a/apply.h\n> +++ b/apply.h\n> @@ -79,7 +79,6 @@ struct apply_state {\n> \n> \t/* Various \"current state\" */\n> \tint linenr; /* current line number */\n> -\tint extension_linenr; /* first line specifying delete/new/rename/copy */\n> \tstruct string_list symlink_changes; /* we have to track symlinks */\n> \n> \t/*\n"}]}