[PATCH] apply: avoid leaking abandoned git-header state

[PATCH] apply: avoid leaking abandoned git-header state

[Zephyr Yao]

When find_header() sees a "diff --git" line, it calls
parse_git_diff_header() to parse the git-style extended header. That parser
updates the caller's struct patch as it goes, filling in the default name,
old/new names, and new/delete state.

But not every "diff --git" line found while scanning is ultimately accepted
as the patch header. If parse_git_diff_header() returns a length that covers
only the "diff --git" line, find_header() continues scanning for another
header. In that case the partially parsed git-header state must not interfere
with the later traditional "---" / "+++" header.

Leaving that state behind can combine incompatible metadata from the
abandoned git header and the later traditional header. For example, after:

	diff --git a/foo b/foo

	--- /dev/null
	+++ b/foo
	@@ -0,0 +1 @@
	+x

the abandoned git header can leave an old name in the patch, while the
traditional header marks the patch as creating a new file. That impossible
state later trips the check_preimage() assertion that a creation patch should
not have a preimage.

Parse a candidate git header into a temporary patch and line number. Commit
that temporary state to the real patch only when the git header is actually
accepted; otherwise release it and keep scanning with the original patch
state unchanged.

Also reject an empty parsed default name from the "diff --git" line.
An empty patch->def_name is not a valid pathname, and should not be
used later as a fallback when old_name and new_name are missing.

Add regression tests for both the empty default-name case and the non-empty
abandoned-header case above.

Co-authored-by: Mahya SamDaliri <ms3539@njit.edu>
Signed-off-by: Mahya SamDaliri <ms3539@njit.edu>
Co-authored-by: Haotian Zhang <haotian.zhang@njit.edu>
Signed-off-by: Haotian Zhang <haotian.zhang@njit.edu>
Co-authored-by: Martin Kellogg <martin.kellogg@njit.edu>
Signed-off-by: Martin Kellogg <martin.kellogg@njit.edu>
Signed-off-by: Zephyr Yao <zhihao.yao@njit.edu>
---
 apply.c               | 29 ++++++++++++++++++++++-------
 t/t4100-apply-stat.sh | 25 +++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/apply.c b/apply.c
index 5e54453..2ce9b6a 100644
--- a/apply.c
+++ b/apply.c
@@ -1362,6 +1362,9 @@ int parse_git_diff_header(struct strbuf *root,
 	 * the default name from the header.
 	 */
 	patch->def_name = git_header_name(p_value, line, len);
+	if (patch->def_name && !*patch->def_name)
+		FREE_AND_NULL(patch->def_name);
+
 	if (patch->def_name && root->len) {
 		char *s = xstrfmt("%s%s", root->buf, patch->def_name);
 		free(patch->def_name);
@@ -1632,15 +1635,27 @@ static int find_header(struct apply_state *state,
 		 * or mode change, so we handle that specially
 		 */
 		if (!memcmp("diff --git ", line, 11)) {
-			int git_hdr_len = parse_git_diff_header(&state->root,
-								state->patch_input_file,
-								&state->linenr,
-								state->p_value, line, len,
-								size, patch);
-			if (git_hdr_len < 0)
+			struct patch git_patch = { 0 };
+			int git_linenr = state->linenr;
+			int git_hdr_len;
+
+			git_patch.inaccurate_eof = patch->inaccurate_eof;
+			git_patch.recount = patch->recount;
+			git_hdr_len = parse_git_diff_header(&state->root,
+							    state->patch_input_file,
+							    &git_linenr,
+							    state->p_value, line, len,
+							    size, &git_patch);
+			if (git_hdr_len < 0) {
+				release_patch(&git_patch);
 				return -128;
-			if (git_hdr_len <= len)
+			}
+			if (git_hdr_len <= len) {
+				release_patch(&git_patch);
 				continue;
+			}
+			*patch = git_patch;
+			state->linenr = git_linenr;
 			*hdrsize = git_hdr_len;
 			return offset;
 		}
diff --git a/t/t4100-apply-stat.sh b/t/t4100-apply-stat.sh
index 8393076..d3406ed 100755
--- a/t/t4100-apply-stat.sh
+++ b/t/t4100-apply-stat.sh
@@ -113,6 +113,31 @@ test_expect_success 'applying a patch with a missing filename reports the input'
 	test_cmp expect err
 '
 
+test_expect_success 'empty default filename reports the input' '
+	cat >empty-name.patch <<-\EOF &&
+	diff --git "a/""b/"
+
+	--- /dev/null
+	+++ "
+	@@ -0,0 +1 @@
+	+
+	EOF
+	test_must_fail git apply empty-name.patch 2>err &&
+	test_grep "git diff header lacks filename information" err
+'
+
+test_expect_success 'abandoned git header does not reuse names' '
+	cat >abandoned-git-header.patch <<-\EOF &&
+	diff --git a/foo b/foo
+
+	--- /dev/null
+	+++ b/foo
+	@@ -0,0 +1 @@
+	+x
+	EOF
+	git apply --check abandoned-git-header.patch
+'
+
 test_expect_success 'applying a patch with an invalid mode reports the input' '
 	cat >mode.patch <<-\EOF &&
 	diff --git a/f b/f
-- 
2.47.0

Re: [PATCH] apply: avoid leaking abandoned git-header state

[Junio C Hamano]

Zephyr Yao <zot.zot.yao@gmail.com> writes:

> When find_header() sees a "diff --git" line, it calls
> parse_git_diff_header() to parse the git-style extended header. That parser
> updates the caller's struct patch as it goes, filling in the default name,
> old/new names, and new/delete state.
>
> But not every "diff --git" line found while scanning is ultimately accepted
> as the patch header. If parse_git_diff_header() returns a length that covers
> only the "diff --git" line, find_header() continues scanning for another
> header. In that case the partially parsed git-header state must not interfere
> with the later traditional "---" / "+++" header.

This patch has gathered no response.  Perhaps the e-mail received no
reply because it was sent in early July around the holiday, or
perhaps nobody was interested in the topic.  In any case, I am
cleaning up the "What's cooking" report and noticed that this has
been in the "Needs review" state for a long time.

So I took a look.

These cross checks are primarily sanity checks.  Having the parser
notice a discrepancy and abort is a good thing.  The user is
supposed to inspect the situation and fix a malformed patch (such
as one containing a stray 'diff --git' header unrelated to the
actual patch).

So I do not think we want to apply this patch.

Thanks.