[PATCH 1/2] apply: guard against renames of non-existant empty files

[PATCH 1/2] apply: guard against renames of non-existant empty files

[Vegard Nossum]

If we have a patch like the one in the new test-case, then we will
try to rename a non-existant empty file, i.e. patch->old_name will
be NULL. In this case, a NULL entry will be added to fn_table, which
is not allowed (a subsequent binary search will die with a NULL
pointer dereference).

The patch file is completely bogus as it tries to rename something
that is known not to exist, so we can throw an error for this.

Found using AFL.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 apply.c                     |  3 ++-
 t/t4154-apply-git-header.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100755 t/t4154-apply-git-header.sh

diff --git a/apply.c b/apply.c
index 0e2caeab9..cbf7cc7f2 100644
--- a/apply.c
+++ b/apply.c
@@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
 				patch->old_name = xstrdup(patch->def_name);
 				patch->new_name = xstrdup(patch->def_name);
 			}
-			if (!patch->is_delete && !patch->new_name) {
+			if ((!patch->is_delete && !patch->new_name) ||
+			    (patch->is_rename && !patch->old_name)) {
 				error(_("git diff header lacks filename information "
 					     "(line %d)"), state->linenr);
 				return -128;
diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
new file mode 100755
index 000000000..d651af4a2
--- /dev/null
+++ b/t/t4154-apply-git-header.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='apply with git/--git headers'
+
+. ./test-lib.sh
+
+test_expect_success 'apply old mode / rename new' '
+	test_must_fail git apply << EOF
+diff --git a/1 b/1
+old mode 0
+rename new 0
+EOF
+'
+
+test_done
-- 
2.12.0.rc0

[PATCH 2/2] apply: handle assertion failure gracefully

[Vegard Nossum]

For the patches in the added testcases, we were crashing with:

    git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' failed.

As it turns out, check_preimage() is prepared to handle these conditions,
so we can remove the assertion.

Found using AFL.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

---

(I'm fully aware of how it looks to just delete an assertion to "fix" a
bug without any other changes to accomodate the condition that was
being tested for. I am definitely not an expert on this code, but as far
as I can tell -- both by reviewing and testing the code -- the function
really is prepared to handle the case where patch->is_new == 1, as it
will always hit another error condition if that is true. I've tried to
add more test cases to show what errors you can expect to see instead of
the assertion failure when trying to apply these nonsensical patches. If
you don't want to remove the assertion for whatever reason, please feel
free to take the testcases and add "# TODO: known breakage" or whatever.)
---
 apply.c                     |  1 -
 t/t4154-apply-git-header.sh | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index cbf7cc7f2..9219d2737 100644
--- a/apply.c
+++ b/apply.c
@@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
 	if (!old_name)
 		return 0;
 
-	assert(patch->is_new <= 0);
 	previous = previous_patch(state, patch, &status);
 
 	if (status)
diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
index d651af4a2..c440c48ad 100755
--- a/t/t4154-apply-git-header.sh
+++ b/t/t4154-apply-git-header.sh
@@ -12,4 +12,40 @@ rename new 0
 EOF
 '
 
+test_expect_success 'apply deleted file mode / new file mode / wrong mode' '
+	test_must_fail git apply << EOF
+diff --git a/. b/.
+deleted file mode 
+new file mode 
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / wrong type' '
+	mkdir x &&
+	chmod 755 x &&
+	test_must_fail git apply << EOF
+diff --git a/x b/x
+deleted file mode 160755
+new file mode 
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / already exists' '
+	touch 1 &&
+	chmod 644 1 &&
+	test_must_fail git apply << EOF
+diff --git a/1 b/1
+deleted file mode 100644
+new file mode 
+EOF
+'
+
+test_expect_success 'apply new file mode / copy from / nonexistant file' '
+	test_must_fail git apply << EOF
+diff --git a/. b/.
+new file mode 
+copy from  
+EOF
+'
+
 test_done
-- 
2.12.0.rc0

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 25.02.2017 um 11:13 schrieb Vegard Nossum:
> For the patches in the added testcases, we were crashing with:
>
>     git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' failed.
>
> As it turns out, check_preimage() is prepared to handle these conditions,
> so we can remove the assertion.
>
> Found using AFL.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>
> ---
>
> (I'm fully aware of how it looks to just delete an assertion to "fix" a
> bug without any other changes to accomodate the condition that was
> being tested for. I am definitely not an expert on this code, but as far
> as I can tell -- both by reviewing and testing the code -- the function
> really is prepared to handle the case where patch->is_new == 1, as it
> will always hit another error condition if that is true. I've tried to
> add more test cases to show what errors you can expect to see instead of
> the assertion failure when trying to apply these nonsensical patches. If
> you don't want to remove the assertion for whatever reason, please feel
> free to take the testcases and add "# TODO: known breakage" or whatever.)
> ---
>  apply.c                     |  1 -
>  t/t4154-apply-git-header.sh | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index cbf7cc7f2..9219d2737 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>  	if (!old_name)
>  		return 0;
>
> -	assert(patch->is_new <= 0);

5c47f4c6 (builtin-apply: accept patch to an empty file) added that line. 
  Its intent was to handle diffs that contain an old name even for a 
file that's created.  Citing from its commit message: "When we cannot be 
sure by parsing the patch that it is not a creation patch, we shouldn't 
complain when if there is no such a file."  Why not stop complaining 
also in case we happen to know for sure that it's a creation patch? 
I.e., why not replace the assert() with:

	if (patch->is_new == 1)
		goto is_new;

>  	previous = previous_patch(state, patch, &status);
>
>  	if (status)
> diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
> index d651af4a2..c440c48ad 100755
> --- a/t/t4154-apply-git-header.sh
> +++ b/t/t4154-apply-git-header.sh
> @@ -12,4 +12,40 @@ rename new 0
>  EOF
>  '
>
> +test_expect_success 'apply deleted file mode / new file mode / wrong mode' '
> +	test_must_fail git apply << EOF
> +diff --git a/. b/.
> +deleted file mode
> +new file mode
> +EOF
> +'
> +
> +test_expect_success 'apply deleted file mode / new file mode / wrong type' '
> +	mkdir x &&
> +	chmod 755 x &&
> +	test_must_fail git apply << EOF
> +diff --git a/x b/x
> +deleted file mode 160755
> +new file mode
> +EOF
> +'
> +
> +test_expect_success 'apply deleted file mode / new file mode / already exists' '
> +	touch 1 &&
> +	chmod 644 1 &&
> +	test_must_fail git apply << EOF
> +diff --git a/1 b/1
> +deleted file mode 100644
> +new file mode
> +EOF
> +'
> +
> +test_expect_success 'apply new file mode / copy from / nonexistant file' '
> +	test_must_fail git apply << EOF
> +diff --git a/. b/.
> +new file mode
> +copy from
> +EOF
> +'
> +
>  test_done
>

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> diff --git a/apply.c b/apply.c
>> index cbf7cc7f2..9219d2737 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>  	if (!old_name)
>>  		return 0;
>>
>> -	assert(patch->is_new <= 0);
>
> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
> line. Its intent was to handle diffs that contain an old name even for
> a file that's created.  Citing from its commit message: "When we
> cannot be sure by parsing the patch that it is not a creation patch,
> we shouldn't complain when if there is no such a file."  Why not stop
> complaining also in case we happen to know for sure that it's a
> creation patch? I.e., why not replace the assert() with:
>
> 	if (patch->is_new == 1)
> 		goto is_new;
>
>>  	previous = previous_patch(state, patch, &status);

When the caller does know is_new is true, old_name must be made/left
NULL.  That is the invariant this assert is checking to catch an
error in the calling code.

Errors in the patches fed as its input are caught by "if we do not
know if the patch is to add a new path yet, then declare it is, but
if we do know the patch is _NOT_ adding a new path, barf if that
path is not there" and other checks in this function, and changing
the assert to "if already new, then make it a no-op" defeats the
whole point of having an assert (and just removing it is even worse).

Thanks.

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>>> diff --git a/apply.c b/apply.c
>>> index cbf7cc7f2..9219d2737 100644
>>> --- a/apply.c
>>> +++ b/apply.c
>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>>  	if (!old_name)
>>>  		return 0;
>>>
>>> -	assert(patch->is_new <= 0);
>>
>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
>> line. Its intent was to handle diffs that contain an old name even for
>> a file that's created.  Citing from its commit message: "When we
>> cannot be sure by parsing the patch that it is not a creation patch,
>> we shouldn't complain when if there is no such a file."  Why not stop
>> complaining also in case we happen to know for sure that it's a
>> creation patch? I.e., why not replace the assert() with:
>>
>> 	if (patch->is_new == 1)
>> 		goto is_new;
>>
>>>  	previous = previous_patch(state, patch, &status);
>
> When the caller does know is_new is true, old_name must be made/left
> NULL.  That is the invariant this assert is checking to catch an
> error in the calling code.

There are some places in apply.c that set ->is_new to 1, but none of 
them set ->old_name to NULL at the same time.

Having to keep these two members in sync sounds iffy anyway.  Perhaps 
accessors can help, e.g. a setter which frees old_name when is_new is 
set to 1, or a getter which returns NULL for old_name if is_new is 1.

René

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>>> diff --git a/apply.c b/apply.c
>>>> index cbf7cc7f2..9219d2737 100644
>>>> --- a/apply.c
>>>> +++ b/apply.c
>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>>>  	if (!old_name)
>>>>  		return 0;
>>>>
>>>> -	assert(patch->is_new <= 0);
>>>
>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
>>> line. Its intent was to handle diffs that contain an old name even for
>>> a file that's created.  Citing from its commit message: "When we
>>> cannot be sure by parsing the patch that it is not a creation patch,
>>> we shouldn't complain when if there is no such a file."  Why not stop
>>> complaining also in case we happen to know for sure that it's a
>>> creation patch? I.e., why not replace the assert() with:
>>>
>>> 	if (patch->is_new == 1)
>>> 		goto is_new;
>>>
>>>>  	previous = previous_patch(state, patch, &status);
>>
>> When the caller does know is_new is true, old_name must be made/left
>> NULL.  That is the invariant this assert is checking to catch an
>> error in the calling code.
>
> There are some places in apply.c that set ->is_new to 1, but none of
> them set ->old_name to NULL at the same time.

I thought all of these are flipping ->is_new that used to be -1
(unknown) to (now we know it is new), and sets only new_name without
doing anything to old_name, because they know originally both names
are set to NULL.

> Having to keep these two members in sync sounds iffy anyway.  Perhaps
> accessors can help, e.g. a setter which frees old_name when is_new is
> set to 1, or a getter which returns NULL for old_name if is_new is 1.

Definitely, the setter would make it harder to make the mistake.

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 27.02.2017 um 23:33 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>>> diff --git a/apply.c b/apply.c
>>>>> index cbf7cc7f2..9219d2737 100644
>>>>> --- a/apply.c
>>>>> +++ b/apply.c
>>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>>>>  	if (!old_name)
>>>>>  		return 0;
>>>>>
>>>>> -	assert(patch->is_new <= 0);
>>>>
>>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
>>>> line. Its intent was to handle diffs that contain an old name even for
>>>> a file that's created.  Citing from its commit message: "When we
>>>> cannot be sure by parsing the patch that it is not a creation patch,
>>>> we shouldn't complain when if there is no such a file."  Why not stop
>>>> complaining also in case we happen to know for sure that it's a
>>>> creation patch? I.e., why not replace the assert() with:
>>>>
>>>> 	if (patch->is_new == 1)
>>>> 		goto is_new;
>>>>
>>>>>  	previous = previous_patch(state, patch, &status);
>>>
>>> When the caller does know is_new is true, old_name must be made/left
>>> NULL.  That is the invariant this assert is checking to catch an
>>> error in the calling code.
>>
>> There are some places in apply.c that set ->is_new to 1, but none of
>> them set ->old_name to NULL at the same time.
> 
> I thought all of these are flipping ->is_new that used to be -1
> (unknown) to (now we know it is new), and sets only new_name without
> doing anything to old_name, because they know originally both names
> are set to NULL.
> 
>> Having to keep these two members in sync sounds iffy anyway.  Perhaps
>> accessors can help, e.g. a setter which frees old_name when is_new is
>> set to 1, or a getter which returns NULL for old_name if is_new is 1.
> 
> Definitely, the setter would make it harder to make the mistake.

When I added setters, apply started to passed NULL to unlink(2) and
rmdir(2) in some of the new tests, which still failed.

That's because three of the diffs trigger both gitdiff_delete(), which
sets is_delete and old_name, and gitdiff_newfile(), which sets is_new
and new_name.  Create and delete equals move, right?  Or should we
error out at this point already?

The last new diff adds a new file that is copied.  Sounds impossible.
How about something like this, which forbids combinations that make no
sense.  Hope it's not too strict; at least all tests succeed.

---
 apply.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 61 insertions(+), 18 deletions(-)

diff --git a/apply.c b/apply.c
index 21b0bebec5..6cb6860511 100644
--- a/apply.c
+++ b/apply.c
@@ -197,6 +197,14 @@ struct fragment {
 #define BINARY_DELTA_DEFLATED	1
 #define BINARY_LITERAL_DEFLATED 2
 
+enum patch_type {
+	CHANGE,
+	CREATE,
+	DELETE,
+	RENAME,
+	COPY
+};
+
 /*
  * This represents a "patch" to a file, both metainfo changes
  * such as creation/deletion, filemode and content changes represented
@@ -205,6 +213,7 @@ struct fragment {
 struct patch {
 	char *new_name, *old_name, *def_name;
 	unsigned int old_mode, new_mode;
+	enum patch_type type;
 	int is_new, is_delete;	/* -1 = unknown, 0 = false, 1 = true */
 	int rejected;
 	unsigned ws_rule;
@@ -229,6 +238,36 @@ struct patch {
 	struct object_id threeway_stage[3];
 };
 
+static int set_patch_type(struct patch *patch, enum patch_type type)
+{
+	if (patch->type != CHANGE && patch->type != type)
+		return error(_("conflicting patch types"));
+	patch->type = type;
+	switch (type) {
+	case CHANGE:
+		break;
+	case CREATE:
+		patch->is_new = 1;
+		patch->is_delete = 0;
+		free(patch->old_name);
+		patch->old_name = NULL;
+		break;
+	case DELETE:
+		patch->is_new = 0;
+		patch->is_delete = 1;
+		free(patch->new_name);
+		patch->new_name = NULL;
+		break;
+	case RENAME:
+		patch->is_rename = 1;
+		break;
+	case COPY:
+		patch->is_copy = 1;
+		break;
+	}
+	return 0;
+}
+
 static void free_fragment_list(struct fragment *list)
 {
 	while (list) {
@@ -907,13 +946,13 @@ static int parse_traditional_patch(struct apply_state *state,
 		}
 	}
 	if (is_dev_null(first)) {
-		patch->is_new = 1;
-		patch->is_delete = 0;
+		if (set_patch_type(patch, CREATE))
+			return -1;
 		name = find_name_traditional(state, second, NULL, state->p_value);
 		patch->new_name = name;
 	} else if (is_dev_null(second)) {
-		patch->is_new = 0;
-		patch->is_delete = 1;
+		if (set_patch_type(patch, DELETE))
+			return -1;
 		name = find_name_traditional(state, first, NULL, state->p_value);
 		patch->old_name = name;
 	} else {
@@ -922,12 +961,12 @@ static int parse_traditional_patch(struct apply_state *state,
 		name = find_name_traditional(state, second, first_name, state->p_value);
 		free(first_name);
 		if (has_epoch_timestamp(first)) {
-			patch->is_new = 1;
-			patch->is_delete = 0;
+			if (set_patch_type(patch, CREATE))
+				return -1;
 			patch->new_name = name;
 		} else if (has_epoch_timestamp(second)) {
-			patch->is_new = 0;
-			patch->is_delete = 1;
+			if (set_patch_type(patch, DELETE))
+				return -1;
 			patch->old_name = name;
 		} else {
 			patch->old_name = name;
@@ -1031,7 +1070,8 @@ static int gitdiff_delete(struct apply_state *state,
 			  const char *line,
 			  struct patch *patch)
 {
-	patch->is_delete = 1;
+	if (set_patch_type(patch, DELETE))
+		return -1;
 	free(patch->old_name);
 	patch->old_name = xstrdup_or_null(patch->def_name);
 	return gitdiff_oldmode(state, line, patch);
@@ -1041,7 +1081,8 @@ static int gitdiff_newfile(struct apply_state *state,
 			   const char *line,
 			   struct patch *patch)
 {
-	patch->is_new = 1;
+	if (set_patch_type(patch, CREATE))
+		return -1;
 	free(patch->new_name);
 	patch->new_name = xstrdup_or_null(patch->def_name);
 	return gitdiff_newmode(state, line, patch);
@@ -1051,7 +1092,8 @@ static int gitdiff_copysrc(struct apply_state *state,
 			   const char *line,
 			   struct patch *patch)
 {
-	patch->is_copy = 1;
+	if (set_patch_type(patch, COPY))
+		return -1;
 	free(patch->old_name);
 	patch->old_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);
 	return 0;
@@ -1061,7 +1103,8 @@ static int gitdiff_copydst(struct apply_state *state,
 			   const char *line,
 			   struct patch *patch)
 {
-	patch->is_copy = 1;
+	if (set_patch_type(patch, COPY))
+		return -1;
 	free(patch->new_name);
 	patch->new_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);
 	return 0;
@@ -1071,7 +1114,8 @@ static int gitdiff_renamesrc(struct apply_state *state,
 			     const char *line,
 			     struct patch *patch)
 {
-	patch->is_rename = 1;
+	if (set_patch_type(patch, RENAME))
+		return -1;
 	free(patch->old_name);
 	patch->old_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);
 	return 0;
@@ -1081,7 +1125,8 @@ static int gitdiff_renamedst(struct apply_state *state,
 			     const char *line,
 			     struct patch *patch)
 {
-	patch->is_rename = 1;
+	if (set_patch_type(patch, RENAME))
+		return -1;
 	free(patch->new_name);
 	patch->new_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);
 	return 0;
@@ -3704,10 +3749,8 @@ static int check_preimage(struct apply_state *state,
 	return 0;
 
  is_new:
-	patch->is_new = 1;
-	patch->is_delete = 0;
-	free(patch->old_name);
-	patch->old_name = NULL;
+	if (set_patch_type(patch, CREATE))
+		return -1;
 	return 0;
 }
 
-- 
2.12.0

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 28.02.2017 um 11:50 schrieb René Scharfe:
> Am 27.02.2017 um 23:33 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
>>>> René Scharfe <l.s.r@web.de> writes:
>>>>
>>>>>> diff --git a/apply.c b/apply.c
>>>>>> index cbf7cc7f2..9219d2737 100644
>>>>>> --- a/apply.c
>>>>>> +++ b/apply.c
>>>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>>>>>   	if (!old_name)
>>>>>>   		return 0;
>>>>>>
>>>>>> -	assert(patch->is_new <= 0);
>>>>>
>>>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
>>>>> line. Its intent was to handle diffs that contain an old name even for
>>>>> a file that's created.  Citing from its commit message: "When we
>>>>> cannot be sure by parsing the patch that it is not a creation patch,
>>>>> we shouldn't complain when if there is no such a file."  Why not stop
>>>>> complaining also in case we happen to know for sure that it's a
>>>>> creation patch? I.e., why not replace the assert() with:
>>>>>
>>>>> 	if (patch->is_new == 1)
>>>>> 		goto is_new;
>>>>>
>>>>>>   	previous = previous_patch(state, patch, &status);
>>>>
>>>> When the caller does know is_new is true, old_name must be made/left
>>>> NULL.  That is the invariant this assert is checking to catch an
>>>> error in the calling code.
>>>
>>> There are some places in apply.c that set ->is_new to 1, but none of
>>> them set ->old_name to NULL at the same time.
>>
>> I thought all of these are flipping ->is_new that used to be -1
>> (unknown) to (now we know it is new), and sets only new_name without
>> doing anything to old_name, because they know originally both names
>> are set to NULL.
>>
>>> Having to keep these two members in sync sounds iffy anyway.  Perhaps
>>> accessors can help, e.g. a setter which frees old_name when is_new is
>>> set to 1, or a getter which returns NULL for old_name if is_new is 1.
>>
>> Definitely, the setter would make it harder to make the mistake.
> 
> When I added setters, apply started to passed NULL to unlink(2) and
> rmdir(2) in some of the new tests, which still failed.
> 
> That's because three of the diffs trigger both gitdiff_delete(), which
> sets is_delete and old_name, and gitdiff_newfile(), which sets is_new
> and new_name.  Create and delete equals move, right?  Or should we
> error out at this point already?
> 
> The last new diff adds a new file that is copied.  Sounds impossible.
> How about something like this, which forbids combinations that make no
> sense.  Hope it's not too strict; at least all tests succeed.
> 
> ---
>   apply.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 61 insertions(+), 18 deletions(-)

Thought a bit more about it, and as a result here's a simpler approach:

-- >8 --
Subject: [PATCH] apply: check git diffs for mutually exclusive header lines

A file can either be added, removed, copied, or renamed, but no two of
these actions can be done by the same patch.  Some of these combinations
provoke error messages due to missing file names, and some are only
caught by an assertion.  Check git patches already as they are parsed
and report conflicting lines on sight.

Found by Vegard Nossum using AFL.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 apply.c                | 14 ++++++++++++++
 apply.h                |  1 +
 t/t4136-apply-check.sh | 18 ++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/apply.c b/apply.c
index 8cd6435c74..8a5e44c474 100644
--- a/apply.c
+++ b/apply.c
@@ -1312,6 +1312,18 @@ static char *git_header_name(struct apply_state *state,
 	}
 }
 
+static int check_header_line(struct apply_state *state, struct patch *patch)
+{
+	int extensions = (patch->is_delete == 1) + (patch->is_new == 1) +
+			 (patch->is_rename == 1) + (patch->is_copy == 1);
+	if (extensions > 1)
+		return error(_("inconsistent header lines %d and %d"),
+			     state->extension_linenr, state->linenr);
+	if (extensions && !state->extension_linenr)
+		state->extension_linenr = state->linenr;
+	return 0;
+}
+
 /* Verify that we recognize the lines following a git header */
 static int parse_git_header(struct apply_state *state,
 			    const char *line,
@@ -1378,6 +1390,8 @@ static int parse_git_header(struct apply_state *state,
 			res = p->fn(state, line + oplen, patch);
 			if (res < 0)
 				return -1;
+			if (check_header_line(state, patch))
+				return -1;
 			if (res > 0)
 				return offset;
 			break;
diff --git a/apply.h b/apply.h
index b3d6783d55..b52078b486 100644
--- a/apply.h
+++ b/apply.h
@@ -79,6 +79,7 @@ struct apply_state {
 
 	/* Various "current state" */
 	int linenr; /* current line number */
+	int extension_linenr; /* first line specifying delete/new/rename/copy */
 	struct string_list symlink_changes; /* we have to track symlinks */
 
 	/*
diff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh
index 4b0a374b63..6d92872318 100755
--- a/t/t4136-apply-check.sh
+++ b/t/t4136-apply-check.sh
@@ -29,4 +29,22 @@ test_expect_success 'apply exits non-zero with no-op patch' '
 	test_must_fail git apply --check input
 '
 
+test_expect_success 'invalid combination: create and copy' '
+	test_must_fail git apply --check - <<-\EOF
+	diff --git a/1 b/2
+	new file mode 100644
+	copy from 1
+	copy to 2
+	EOF
+'
+
+test_expect_success 'invalid combination: create and rename' '
+	test_must_fail git apply --check - <<-\EOF
+	diff --git a/1 b/2
+	new file mode 100644
+	rename from 1
+	rename to 2
+	EOF
+'
+
 test_done
-- 
2.13.2

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Thought a bit more about it, and as a result here's a simpler approach:
>
> -- >8 --
> Subject: [PATCH] apply: check git diffs for mutually exclusive header lines
>
> A file can either be added, removed, copied, or renamed, but no two of
> these actions can be done by the same patch.  Some of these combinations
> provoke error messages due to missing file names, and some are only
> caught by an assertion.  Check git patches already as they are parsed
> and report conflicting lines on sight.
>
> Found by Vegard Nossum using AFL.
>
> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  apply.c                | 14 ++++++++++++++
>  apply.h                |  1 +
>  t/t4136-apply-check.sh | 18 ++++++++++++++++++
>  3 files changed, 33 insertions(+)
>
> diff --git a/apply.c b/apply.c
> index 8cd6435c74..8a5e44c474 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1312,6 +1312,18 @@ static char *git_header_name(struct apply_state *state,
>  	}
>  }
>  
> +static int check_header_line(struct apply_state *state, struct patch *patch)
> +{
> +	int extensions = (patch->is_delete == 1) + (patch->is_new == 1) +
> +			 (patch->is_rename == 1) + (patch->is_copy == 1);
> +	if (extensions > 1)
> +		return error(_("inconsistent header lines %d and %d"),
> +			     state->extension_linenr, state->linenr);
> +	if (extensions && !state->extension_linenr)
> +		state->extension_linenr = state->linenr;

OK.  I wondered briefly what happens if the first git_header that
sets one of the extensions can be at line 0 (calusng
state->extension_linenr to be set to 0), but even in that case, the
second problematic one will correctly report the 0th and its own
line as culprit, so this is OK.  It makes me question if there is
any point checking !state->extension_linenr in the if() statement,
though.

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 27.06.2017 um 20:08 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Thought a bit more about it, and as a result here's a simpler approach:
>>
>> -- >8 --
>> Subject: [PATCH] apply: check git diffs for mutually exclusive header lines
>>
>> A file can either be added, removed, copied, or renamed, but no two of
>> these actions can be done by the same patch.  Some of these combinations
>> provoke error messages due to missing file names, and some are only
>> caught by an assertion.  Check git patches already as they are parsed
>> and report conflicting lines on sight.
>>
>> Found by Vegard Nossum using AFL.
>>
>> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>>   apply.c                | 14 ++++++++++++++
>>   apply.h                |  1 +
>>   t/t4136-apply-check.sh | 18 ++++++++++++++++++
>>   3 files changed, 33 insertions(+)
>>
>> diff --git a/apply.c b/apply.c
>> index 8cd6435c74..8a5e44c474 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -1312,6 +1312,18 @@ static char *git_header_name(struct apply_state *state,
>>   	}
>>   }
>>   
>> +static int check_header_line(struct apply_state *state, struct patch *patch)
>> +{
>> +	int extensions = (patch->is_delete == 1) + (patch->is_new == 1) +
>> +			 (patch->is_rename == 1) + (patch->is_copy == 1);
>> +	if (extensions > 1)
>> +		return error(_("inconsistent header lines %d and %d"),
>> +			     state->extension_linenr, state->linenr);
>> +	if (extensions && !state->extension_linenr)
>> +		state->extension_linenr = state->linenr;
> 
> OK.  I wondered briefly what happens if the first git_header that
> sets one of the extensions can be at line 0 (calusng
> state->extension_linenr to be set to 0), but even in that case, the
> second problematic one will correctly report the 0th and its own
> line as culprit, so this is OK.  It makes me question if there is
> any point checking !state->extension_linenr in the if() statement,
> though.

It makes sure that ->extension_linenr is set to the first line number of
an extension (like, say, "copy from") and not advanced again (e.g. when
we hit "copy to", or more importantly some line like "similarity index"
which does not set one of the extension bits and thus can't actually
cause a conflict).

Hmm, pondering that, it seems I forgot to reset its value after each
patch.  Or better just move it into struct patch, next to the extension
bits:

-- >8 --
Subject: fixup! apply: check git diffs for mutually exclusive header lines
---
 apply.c | 7 ++++---
 apply.h | 1 -
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/apply.c b/apply.c
index db38bc3cdd..c442b89328 100644
--- a/apply.c
+++ b/apply.c
@@ -211,6 +211,7 @@ struct patch {
 	unsigned ws_rule;
 	int lines_added, lines_deleted;
 	int score;
+	int extension_linenr; /* first line specifying delete/new/rename/copy */
 	unsigned int is_toplevel_relative:1;
 	unsigned int inaccurate_eof:1;
 	unsigned int is_binary:1;
@@ -1325,9 +1326,9 @@ static int check_header_line(struct apply_state *state, struct patch *patch)
 			 (patch->is_rename == 1) + (patch->is_copy == 1);
 	if (extensions > 1)
 		return error(_("inconsistent header lines %d and %d"),
-			     state->extension_linenr, state->linenr);
-	if (extensions && !state->extension_linenr)
-		state->extension_linenr = state->linenr;
+			     patch->extension_linenr, state->linenr);
+	if (extensions && !patch->extension_linenr)
+		patch->extension_linenr = state->linenr;
 	return 0;
 }
 
diff --git a/apply.h b/apply.h
index b52078b486..b3d6783d55 100644
--- a/apply.h
+++ b/apply.h
@@ -79,7 +79,6 @@ struct apply_state {
 
 	/* Various "current state" */
 	int linenr; /* current line number */
-	int extension_linenr; /* first line specifying delete/new/rename/copy */
 	struct string_list symlink_changes; /* we have to track symlinks */
 
 	/*
-- 
2.13.2

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Hmm, pondering that, it seems I forgot to reset its value after each
> patch.  Or better just move it into struct patch, next to the extension
> bits:

Good catch.

> -- >8 --
> Subject: fixup! apply: check git diffs for mutually exclusive header lines
> ---
>  apply.c | 7 ++++---
>  apply.h | 1 -
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index db38bc3cdd..c442b89328 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -211,6 +211,7 @@ struct patch {
>  	unsigned ws_rule;
>  	int lines_added, lines_deleted;
>  	int score;
> +	int extension_linenr; /* first line specifying delete/new/rename/copy */
>  	unsigned int is_toplevel_relative:1;
>  	unsigned int inaccurate_eof:1;
>  	unsigned int is_binary:1;
> @@ -1325,9 +1326,9 @@ static int check_header_line(struct apply_state *state, struct patch *patch)
>  			 (patch->is_rename == 1) + (patch->is_copy == 1);
>  	if (extensions > 1)
>  		return error(_("inconsistent header lines %d and %d"),
> -			     state->extension_linenr, state->linenr);
> -	if (extensions && !state->extension_linenr)
> -		state->extension_linenr = state->linenr;
> +			     patch->extension_linenr, state->linenr);
> +	if (extensions && !patch->extension_linenr)
> +		patch->extension_linenr = state->linenr;
>  	return 0;
>  }
>  
> diff --git a/apply.h b/apply.h
> index b52078b486..b3d6783d55 100644
> --- a/apply.h
> +++ b/apply.h
> @@ -79,7 +79,6 @@ struct apply_state {
>  
>  	/* Various "current state" */
>  	int linenr; /* current line number */
> -	int extension_linenr; /* first line specifying delete/new/rename/copy */
>  	struct string_list symlink_changes; /* we have to track symlinks */
>  
>  	/*

Re: [PATCH 2/2] apply: handle assertion failure gracefully

[René Scharfe]

Am 25.02.2017 um 11:13 schrieb Vegard Nossum:
> For the patches in the added testcases, we were crashing with:
> 
>      git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' failed.


> diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
> index d651af4a2..c440c48ad 100755
> --- a/t/t4154-apply-git-header.sh
> +++ b/t/t4154-apply-git-header.sh
> @@ -12,4 +12,40 @@ rename new 0
>   EOF
>   '
>   
> +test_expect_success 'apply deleted file mode / new file mode / wrong mode' '
> +	test_must_fail git apply << EOF
> +diff --git a/. b/.
> +deleted file mode
> +new file mode
> +EOF
> +'

-- >8 --
Subject: [PATCH] apply: check git diffs for invalid file modes

An empty string as mode specification is accepted silently by git apply,
as Vegard Nossum found out using AFL.  It's interpreted as zero.  Reject
such bogus file modes, and only accept ones consisting exclusively of
octal digits.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 apply.c                   | 17 ++++++++++++-----
 t/t4129-apply-samemode.sh | 16 +++++++++++++++-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/apply.c b/apply.c
index 8a5e44c474..db38bc3cdd 100644
--- a/apply.c
+++ b/apply.c
@@ -1001,20 +1001,27 @@ static int gitdiff_newname(struct apply_state *state,
 				   DIFF_NEW_NAME);
 }
 
+static int parse_mode_line(const char *line, int linenr, unsigned int *mode)
+{
+	char *end;
+	*mode = strtoul(line, &end, 8);
+	if (end == line || !isspace(*end))
+		return error(_("invalid mode on line %d: %s"), linenr, line);
+	return 0;
+}
+
 static int gitdiff_oldmode(struct apply_state *state,
 			   const char *line,
 			   struct patch *patch)
 {
-	patch->old_mode = strtoul(line, NULL, 8);
-	return 0;
+	return parse_mode_line(line, state->linenr, &patch->old_mode);
 }
 
 static int gitdiff_newmode(struct apply_state *state,
 			   const char *line,
 			   struct patch *patch)
 {
-	patch->new_mode = strtoul(line, NULL, 8);
-	return 0;
+	return parse_mode_line(line, state->linenr, &patch->new_mode);
 }
 
 static int gitdiff_delete(struct apply_state *state,
@@ -1128,7 +1135,7 @@ static int gitdiff_index(struct apply_state *state,
 	memcpy(patch->new_sha1_prefix, line, len);
 	patch->new_sha1_prefix[len] = 0;
 	if (*ptr == ' ')
-		patch->old_mode = strtoul(ptr+1, NULL, 8);
+		return gitdiff_oldmode(state, ptr + 1, patch);
 	return 0;
 }
 
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index c268298eaf..5cdd76dfa7 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -13,7 +13,9 @@ test_expect_success setup '
 	echo modified >file &&
 	git diff --stat -p >patch-0.txt &&
 	chmod +x file &&
-	git diff --stat -p >patch-1.txt
+	git diff --stat -p >patch-1.txt &&
+	sed "s/^\(new mode \).*/\1/" <patch-1.txt >patch-empty-mode.txt &&
+	sed "s/^\(new mode \).*/\1garbage/" <patch-1.txt >patch-bogus-mode.txt
 '
 
 test_expect_success FILEMODE 'same mode (no index)' '
@@ -59,4 +61,16 @@ test_expect_success FILEMODE 'mode update (index only)' '
 	git ls-files -s file | grep "^100755"
 '
 
+test_expect_success FILEMODE 'empty mode is rejected' '
+	git reset --hard &&
+	test_must_fail git apply patch-empty-mode.txt 2>err &&
+	test_i18ngrep "invalid mode" err
+'
+
+test_expect_success FILEMODE 'bogus mode is rejected' '
+	git reset --hard &&
+	test_must_fail git apply patch-bogus-mode.txt 2>err &&
+	test_i18ngrep "invalid mode" err
+'
+
 test_done
-- 
2.13.2

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[Philip Oakley]

From: "Vegard Nossum" <vegard.nossum@oracle.com>
> If we have a patch like the one in the new test-case, then we will

"the one in the new test-case" needs a clearer reference to the particular 
case so that future readers will know what it refers to. Noticed while 
browsing the commit message..

..reads further; Maybe it's "AFL (American fuzzy lop) found a failure. Add a 
new test case and fix the fault"?

[same for patch 2]

> try to rename a non-existant empty file, i.e. patch->old_name will
> be NULL. In this case, a NULL entry will be added to fn_table, which
> is not allowed (a subsequent binary search will die with a NULL
> pointer dereference).
>
> The patch file is completely bogus as it tries to rename something
> that is known not to exist, so we can throw an error for this.
>
> Found using AFL.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
> apply.c                     |  3 ++-
> t/t4154-apply-git-header.sh | 15 +++++++++++++++
> 2 files changed, 17 insertions(+), 1 deletion(-)
> create mode 100755 t/t4154-apply-git-header.sh
>
> diff --git a/apply.c b/apply.c
> index 0e2caeab9..cbf7cc7f2 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
>  patch->old_name = xstrdup(patch->def_name);
>  patch->new_name = xstrdup(patch->def_name);
>  }
> - if (!patch->is_delete && !patch->new_name) {
> + if ((!patch->is_delete && !patch->new_name) ||
> +     (patch->is_rename && !patch->old_name)) {
>  error(_("git diff header lacks filename information "
>       "(line %d)"), state->linenr);
>  return -128;
> diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
> new file mode 100755
> index 000000000..d651af4a2
> --- /dev/null
> +++ b/t/t4154-apply-git-header.sh
> @@ -0,0 +1,15 @@
> +#!/bin/sh
> +
> +test_description='apply with git/--git headers'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'apply old mode / rename new' '
> + test_must_fail git apply << EOF
> +diff --git a/1 b/1
> +old mode 0
> +rename new 0
> +EOF
> +'
> +
> +test_done
> -- 
> 2.12.0.rc0
--
Philip 

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[Vegard Nossum]

On 25/02/2017 12:59, Philip Oakley wrote:
> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>> If we have a patch like the one in the new test-case, then we will
>
> "the one in the new test-case" needs a clearer reference to the
> particular case so that future readers will know what it refers to.
> Noticed while browsing the commit message..

There is only one testcase added by this patch, so how is it possibly
unclear? In what situation would you read a commit message and not even
think to glance at the patch for more details?


Vegard

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[Philip Oakley]

From: "Vegard Nossum" <vegard.nossum@oracle.com>
> On 25/02/2017 12:59, Philip Oakley wrote:
>> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>>> If we have a patch like the one in the new test-case, then we will
>>
>> "the one in the new test-case" needs a clearer reference to the
>> particular case so that future readers will know what it refers to.
>> Noticed while browsing the commit message..
>
> There is only one testcase added by this patch, so how is it possibly
> unclear? In what situation would you read a commit message and not even
> think to glance at the patch for more details?
>
On initial reading of a commit message, the expectation is that the commit 
will be about a change from some previous state, so I immediately asked 
myself, where is that new (recent) test case from.

You could say "This patch presents a new test case" which would straight 
away set the expectation that one should read on to see what its about. It 
was just that as a reader of the log message I didn't pick up the sense you 
wanted to convey. It's easy to see with hindsight or fore-knowledge.

I, personally, think that bringing the AFL discovery to the fore would help 
in explaining why/how the patch appeared in the first place.

Hope that helps explain why I responded.

regards

Philip 

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[René Scharfe]

Am 25.02.2017 um 11:13 schrieb Vegard Nossum:
> If we have a patch like the one in the new test-case, then we will
> try to rename a non-existant empty file, i.e. patch->old_name will
> be NULL. In this case, a NULL entry will be added to fn_table, which
> is not allowed (a subsequent binary search will die with a NULL
> pointer dereference).
>
> The patch file is completely bogus as it tries to rename something
> that is known not to exist, so we can throw an error for this.
>
> Found using AFL.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  apply.c                     |  3 ++-
>  t/t4154-apply-git-header.sh | 15 +++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>  create mode 100755 t/t4154-apply-git-header.sh
>
> diff --git a/apply.c b/apply.c
> index 0e2caeab9..cbf7cc7f2 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
>  				patch->old_name = xstrdup(patch->def_name);
>  				patch->new_name = xstrdup(patch->def_name);
>  			}
> -			if (!patch->is_delete && !patch->new_name) {
> +			if ((!patch->is_delete && !patch->new_name) ||
> +			    (patch->is_rename && !patch->old_name)) {

Would it make sense to mirror the previously existing condition and 
check for is_new instead?  I.e.:

			if ((!patch->is_delete && !patch->new_name) ||
			    (!patch->is_new    && !patch->old_name)) {

or

			if (!(patch->is_delete || patch->new_name) ||
			    !(patch->is_new    || patch->old_name)) {

René

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Would it make sense to mirror the previously existing condition and
> check for is_new instead?  I.e.:
>
> 			if ((!patch->is_delete && !patch->new_name) ||
> 			    (!patch->is_new    && !patch->old_name)) {
>

Yes, probably.

> or
>
> 			if (!(patch->is_delete || patch->new_name) ||
> 			    !(patch->is_new    || patch->old_name)) {

This happens after calling parse_git_header() so we should know the
actual value of is_delete and is_new by now (instead of mistaking
-1 aka "unknown" as true), so this rewrite would also be OK.

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[René Scharfe]

Am 27.02.2017 um 21:10 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Would it make sense to mirror the previously existing condition and
>> check for is_new instead?  I.e.:
>>
>> 			if ((!patch->is_delete && !patch->new_name) ||
>> 			    (!patch->is_new    && !patch->old_name)) {
>>
>
> Yes, probably.
>
>> or
>>
>> 			if (!(patch->is_delete || patch->new_name) ||
>> 			    !(patch->is_new    || patch->old_name)) {
>
> This happens after calling parse_git_header() so we should know the
> actual value of is_delete and is_new by now (instead of mistaking
> -1 aka "unknown" as true), so this rewrite would also be OK.

The two variants are logically equivalent -- (!a && !b) == !(a || b).  I 
wonder if the second one may be harder to read, though.

René

Re: [PATCH 1/2] apply: guard against renames of non-existant empty files

[René Scharfe]

Am 27.02.2017 um 23:18 schrieb René Scharfe:
> Am 27.02.2017 um 21:10 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Would it make sense to mirror the previously existing condition and
>>> check for is_new instead?  I.e.:
>>>
>>>             if ((!patch->is_delete && !patch->new_name) ||
>>>                 (!patch->is_new    && !patch->old_name)) {
>>>
>>
>> Yes, probably.

So let's actually do it!

-- >8 --
Subject: [PATCH] apply: check git diffs for missing old filenames

2c93286a (fix "git apply --index ..." not to deref NULL) added a check
for git patches missing a +++ line, preventing a segfault.  Check for
missing --- lines as well, and add a test for each case.

Found by Vegard Nossum using AFL.

Original-patch-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 apply.c                    |  3 ++-
 t/t4133-apply-filenames.sh | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index b963d7d8fb..8cd6435c74 100644
--- a/apply.c
+++ b/apply.c
@@ -1575,7 +1575,8 @@ static int find_header(struct apply_state *state,
 				patch->old_name = xstrdup(patch->def_name);
 				patch->new_name = xstrdup(patch->def_name);
 			}
-			if (!patch->is_delete && !patch->new_name) {
+			if ((!patch->new_name && !patch->is_delete) ||
+			    (!patch->old_name && !patch->is_new)) {
 				error(_("git diff header lacks filename information "
 					     "(line %d)"), state->linenr);
 				return -128;
diff --git a/t/t4133-apply-filenames.sh b/t/t4133-apply-filenames.sh
index 2ecb4216b7..c5ed3b17c4 100755
--- a/t/t4133-apply-filenames.sh
+++ b/t/t4133-apply-filenames.sh
@@ -35,4 +35,28 @@ test_expect_success 'apply diff with inconsistent filenames in headers' '
 	test_i18ngrep "inconsistent old filename" err
 '
 
+test_expect_success 'apply diff with new filename missing from headers' '
+	cat >missing_new_filename.diff <<-\EOF &&
+	diff --git a/f b/f
+	index 0000000..d00491f
+	--- a/f
+	@@ -0,0 +1 @@
+	+1
+	EOF
+	test_must_fail git apply missing_new_filename.diff 2>err &&
+	test_i18ngrep "lacks filename information" err
+'
+
+test_expect_success 'apply diff with old filename missing from headers' '
+	cat >missing_old_filename.diff <<-\EOF &&
+	diff --git a/f b/f
+	index d00491f..0000000
+	+++ b/f
+	@@ -1 +0,0 @@
+	-1
+	EOF
+	test_must_fail git apply missing_old_filename.diff 2>err &&
+	test_i18ngrep "lacks filename information" err
+'
+
 test_done
-- 
2.13.2