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

["René Scharfe" <l.s.r@web.de>]

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