git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Vegard Nossum <vegard.nossum@oracle.com>
To: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Cc: Christian Couder <christian.couder@gmail.com>,
	Michal Zalewski <lcamtuf@google.com>,
	Vegard Nossum <vegard.nossum@oracle.com>
Subject: [PATCH 2/2] apply: handle assertion failure gracefully
Date: Sat, 25 Feb 2017 11:13:07 +0100	[thread overview]
Message-ID: <20170225101307.24067-2-vegard.nossum@oracle.com> (raw)
In-Reply-To: <20170225101307.24067-1-vegard.nossum@oracle.com>

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


  reply	other threads:[~2017-02-25 10:13 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-25 10:13 [PATCH 1/2] apply: guard against renames of non-existant empty files Vegard Nossum
2017-02-25 10:13 ` Vegard Nossum [this message]
2017-02-25 21:21   ` [PATCH 2/2] apply: handle assertion failure gracefully René Scharfe
2017-02-27 20:04     ` Junio C Hamano
2017-02-27 22:18       ` René Scharfe
2017-02-27 22:33         ` Junio C Hamano
2017-02-28 10:50           ` René Scharfe
2017-06-27 17:03             ` René Scharfe
2017-06-27 18:08               ` Junio C Hamano
2017-06-27 20:20                 ` René Scharfe
2017-06-27 21:39                   ` Junio C Hamano
2017-06-27 17:03   ` René Scharfe
2017-02-25 11:59 ` [PATCH 1/2] apply: guard against renames of non-existant empty files Philip Oakley
2017-02-25 12:06   ` Vegard Nossum
2017-02-25 12:47     ` Philip Oakley
2017-02-25 20:51 ` René Scharfe
2017-02-27 20:10   ` Junio C Hamano
2017-02-27 22:18     ` René Scharfe
2017-06-27 17:03       ` René Scharfe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170225101307.24067-2-vegard.nossum@oracle.com \
    --to=vegard.nossum@oracle.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=lcamtuf@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).