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@xxxxxxxxxx>
---
(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