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

[Junio C Hamano]

René Scharfe  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 27.06.2017 um 20:08 schrieb Junio C Hamano:
> René Scharfe  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 
>> Signed-off-by: Rene Scharfe 
>> ---
>>   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  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 
> Signed-off-by: Rene Scharfe 
> ---
>  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 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 
Signed-off-by: Rene Scharfe 
---
 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, , 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, >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, >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-empty-mode.txt &&
+   sed "s/^\(new mode \).*/\1garbage/" 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 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  writes:
>>
>>> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
 René Scharfe  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, );

 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 
Signed-off-by: Rene Scharfe 
---
 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 

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

[René Scharfe]

Am 27.02.2017 um 23:33 schrieb Junio C Hamano:
> René Scharfe  writes:
> 
>> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
>>> René Scharfe  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, );
>>>
>>> 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))
+ 

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

[Junio C Hamano]

René Scharfe  writes:

> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
>> René Scharfe  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, );
>>
>> 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 21:04 schrieb Junio C Hamano:

René Scharfe  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, );


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  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, );

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 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 

---

(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, );

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

[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 

---

(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, );
 
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