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

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

[Junio C Hamano]

René Scharfe  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 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 
---
 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

[Philip Oakley]

From: "Vegard Nossum" 

On 25/02/2017 12:59, Philip Oakley wrote:

From: "Vegard Nossum" 

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

[Vegard Nossum]

On 25/02/2017 12:59, Philip Oakley wrote:

From: "Vegard Nossum" 

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" 

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

[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 
---
 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 0..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