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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 14/11/26 06:50, Tanga Joris wrote:
> 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.


The leak is actually in parse_git_diff_header(), we overwrite 
patch->def_name without freeing the old value, so it leaks when the 
function runs again for the next "diff --git". A free() before the 
assignment fixes it, and both callers zero the patch first so it's safe. 
Can't we do that in a prep patch and keep the test?

Nit: the notes under "---" describe both patches, so a cover letter with 
"--cover-letter --range-diff=<v2>" would be nicer for v3.

Thanks,
Siddharth


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