[Outreachy PATCH v2 1/2] apply: reject a patch after an empty "diff --git" line

[Tanga Joris <tangajoris@gmail.com>]

When a "diff --git" line is followed by a line that is not a git
header line, such as a blank line or an indented "new file mode"
line, parse_git_diff_header() stops right after the "diff --git"
line. find_header() then treats it as an empty git header and keeps
looking, as it has done since 206de27efa (git-apply: ignore empty git
headers, 2005-06-12). It finds the "--- /dev/null" and "+++ b/<path>"
lines and reads the rest as a traditional patch.

But parse_git_diff_header() has already set old_name and new_name
from the "diff --git" line. These names stay in the patch.
parse_traditional_patch() sets is_new for "--- /dev/null", but it
does not clear old_name. So "git apply --check" dies on the assertion
that 5c47f4c6e7 (builtin-apply: accept patch to an empty file,
2008-05-17) added to check_preimage():

  check_preimage: Assertion `patch->is_new <= 0' failed.

The stale new_name causes the same crash when a deletion with such a
header is applied with "git apply -R". When such a patch modifies a
file, it applies. But its "old mode" and "new mode" lines are
ignored.

These are corrupt patches. "git diff" does not produce them. So
report the empty header as an error, instead of reading the patch
that follows it. This also rejects a "diff --git" line that is
followed by other text and then a non-git patch, which used to apply.
A "diff --git" line followed by another "diff --git" line is handled
as before, because 206de27efa ignores empty headers on purpose. Only
a patch that is read under an empty header is a problem.

Reported-by: Gingrspacecadet <jackrawlings2011@gmail.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Tanga Joris <tangajoris@gmail.com>
---
Changes since v1:
 - 1/2 no longer reads the patch that follows an empty "diff --git"
   line. It reports the empty header as an error instead, as Phillip
   and Junio suggested:

     error: corrupt patch: empty "diff --git" header at <file>:<line>

 - A blank line after "diff --git" in a patch that modifies a file is
   now rejected too, as Phillip suggested. v1 and master apply it
   without its "old mode" and "new mode" lines.
 - One case that works on master is now rejected: a "diff --git" line
   followed by other text and then a non-git patch.
 - Dropped the test for an empty "diff --git" section followed by
   another section. That case behaves as on master, but the test fails
   under SANITIZE=leak, because of a leak in git_header_name() that is
   already on master. Also dropped the subdirectory test, which only
   made sense while the patch was applied.
 - 2/2: the code is unchanged. Its commit message now says why patches
   made by Git are not affected.

 apply.c                    | 13 +++++-
 t/t4133-apply-filenames.sh | 86 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index f00b7ba4d3..c5d4fe7887 100644
--- a/apply.c
+++ b/apply.c
@@ -1604,6 +1604,7 @@ static int find_header(struct apply_state *state,
 		       struct patch *patch)
 {
 	unsigned long offset, len;
+	int empty_git_hdr_linenr = 0;
 
 	patch->is_toplevel_relative = 0;
 	patch->is_rename = patch->is_copy = 0;
@@ -1644,6 +1645,7 @@ static int find_header(struct apply_state *state,
 		 * or mode change, so we handle that specially
 		 */
 		if (!memcmp("diff --git ", line, 11)) {
+			int linenr = state->linenr;
 			int git_hdr_len = parse_git_diff_header(&state->root,
 								state->patch_input_file,
 								&state->linenr,
@@ -1651,8 +1653,10 @@ static int find_header(struct apply_state *state,
 								size, patch);
 			if (git_hdr_len < 0)
 				return -128;
-			if (git_hdr_len <= len)
+			if (git_hdr_len <= len) {
+				empty_git_hdr_linenr = linenr;
 				continue;
+			}
 			*hdrsize = git_hdr_len;
 			return offset;
 		}
@@ -1670,6 +1674,13 @@ static int find_header(struct apply_state *state,
 		if (size < nextlen + 14 || memcmp("@@ -", line + len + nextlen, 4))
 			continue;
 
+		/* Git does not write a patch under an empty git header */
+		if (empty_git_hdr_linenr) {
+			error(_("corrupt patch: empty \"diff --git\" header at %s:%d"),
+			      state->patch_input_file, empty_git_hdr_linenr);
+			return -128;
+		}
+
 		/* Ok, we'll consider it a patch */
 		if (parse_traditional_patch(state, line, line+len, patch))
 			return -128;
diff --git a/t/t4133-apply-filenames.sh b/t/t4133-apply-filenames.sh
index 3cab1038cf..751a01f92f 100755
--- a/t/t4133-apply-filenames.sh
+++ b/t/t4133-apply-filenames.sh
@@ -60,4 +60,90 @@ test_expect_success 'apply diff with old filename missing from headers' '
 	test_grep "lacks filename information" err
 '
 
+reject_empty_git_header () {
+	file=$1 &&
+	line=$2 &&
+	shift 2 &&
+	test_must_fail git apply --check "$@" "$file" 2>err &&
+	test_grep "corrupt patch: empty \"diff --git\" header at $file:$line" err
+}
+
+test_expect_success 'reject a new file with a blank line after "diff --git"' '
+	cat >blank_line.diff <<-\EOF &&
+	diff --git a/f b/f
+
+	new file mode 100644
+	index 0000000..d00491f
+	--- /dev/null
+	+++ b/f
+	@@ -0,0 +1 @@
+	+1
+	EOF
+	reject_empty_git_header blank_line.diff 1 &&
+	test_must_fail git apply blank_line.diff &&
+	test_path_is_missing f
+'
+
+test_expect_success 'reject an indented "new file mode" line' '
+	cat >indented_mode.diff <<-\EOF &&
+	diff --git a/f b/f
+	 new file mode 100644
+	index 0000000..d00491f
+	--- /dev/null
+	+++ b/f
+	@@ -0,0 +1 @@
+	+1
+	EOF
+	reject_empty_git_header indented_mode.diff 1
+'
+
+test_expect_success 'reject a deletion with a blank line after "diff --git"' '
+	cat >blank_line_delete.diff <<-\EOF &&
+	diff --git a/f b/f
+
+	deleted file mode 100644
+	index d00491f..0000000
+	--- a/f
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-1
+	EOF
+	reject_empty_git_header blank_line_delete.diff 1 &&
+	reject_empty_git_header blank_line_delete.diff 1 -R
+'
+
+test_expect_success 'reject a modification with a blank line after "diff --git"' '
+	test_when_finished "rm -f g" &&
+	echo 1 >g &&
+	cat >blank_line_modify.diff <<-\EOF &&
+	diff --git a/g b/g
+
+	old mode 100644
+	new mode 100755
+	index d00491f..0cfbf08
+	--- a/g
+	+++ b/g
+	@@ -1 +1 @@
+	-1
+	+2
+	EOF
+	reject_empty_git_header blank_line_modify.diff 1 &&
+	echo 1 >expect &&
+	test_cmp expect g
+'
+
+test_expect_success 'name the empty "diff --git" line in the error' '
+	cat >later_line.diff <<-\EOF &&
+	some text before the patch
+
+	diff --git a/f b/f
+	some text after it
+	--- /dev/null
+	+++ b/f
+	@@ -0,0 +1 @@
+	+1
+	EOF
+	reject_empty_git_header later_line.diff 3
+'
+
 test_done

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
-- 
2.43.0