Re: [PATCH v2 2/3] mailinfo.c::convert_to_utf8: reuse strlen info

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2020, 19:59 UTC
Message-ID: <xmqqv9luq66u.fsf@gitster.c.googlers.com>
In-Reply-To: <e3e542d38818b762f8d6d6b50bc42e01b070772b.1587289680.git.congdanhqx@gmail.com>

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Show 9 quoted lines

> diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
> index ddd35498db..98cda32d0a 100755
> --- a/t/t4254-am-corrupt.sh
> +++ b/t/t4254-am-corrupt.sh
> @@ -3,6 +3,36 @@
>  test_description='git am with corrupt input'
>  . ./test-lib.sh
>  
> +write_nul_patch() {

Style: SP on both sides of (), i.e.

	write_nul_patch () {

But isn't this misnamed? You are interested in injecting '\0' byte in the e-mail headers and bodies, not necessarily part of the patch, but "nul-patch" somehow hints readers that we are writing out a Null Patch (something that does not do anything, perhaps?).

sample_mbox_with_nul is the best alternative I can come up with offhand, which is not great either, but at least it does not say patch.

Show 5 quoted lines

> +	space=' '
> +	qNUL=
> +	case "$1" in
> +		subject) qNUL='=00' ;;
> +	esac

Style: case/esac aligns with case arms, i.e.

	case "$1" in
	subject) qNUL='=00' ;;
	esac

Show 10 quoted lines

> +	cat <<-EOF
> +	From ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001
> +	From: A U Thor <author@example.com>
> +	Date: Sun, 19 Apr 2020 13:42:07 +0700
> +	Subject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=
> +	MIME-Version: 1.0
> +	Content-Type: text/plain; charset=ISO-8859-1
> +	Content-Transfer-Encoding: 8bit
> +
> +	EOF

Since the above does have ${qNUL} interpolated, not quoting <<-EOF is correct. Good.

> +	if test "$1" = body
> +	then
> +		printf "%s\0%s\n" abc def
> +	fi

OK. So we won't be able to inject NUL byte in both header and body at the same time. If you wanted to allow it, you could write

	case ",$1," in
	*,subject,*)	qNUL="=00" ;;
	esac

in the early part, and then rewrite this one like so:

	case ",$1," in
	*,body,*)	printf "..." ;;
	esac

Then those callers who want to ask for both can say

	sample_mbox_with_nul subject,body

Show 8 quoted lines

> +	cat <<-\EOF
> +	---
> +	diff --git a/afile b/afile
> +	new file mode 100644
> +	index 0000000000..e69de29bb2
> +	--$space
> +	2.26.1
> +	EOF

Doesn't this want to interpolate $space in the output? I think you want to say <<-EOF, without quoting.

    cd t && sh t4254-am-corrupt.sh -d && cat trash*.t4254-*/body.patch

tells me that "--$space" is left in the output, not "-- ".

Show 24 quoted lines

> +}
> +
>  test_expect_success setup '
>  	# Note the missing "+++" line:
>  	cat >bad-patch.diff <<-\EOF &&
> @@ -32,4 +62,18 @@ test_expect_success 'try to apply corrupted patch' '
>  	test_i18ncmp expected actual
>  '
>  
> +test_expect_success "NUL in commit message's body" '
> +	test_when_finished "git am --abort" &&
> +	write_nul_patch body >body.patch &&
> +	test_must_fail git am body.patch 2>err &&
> +	grep "a NUL byte in commit log message not allowed" err
> +'
> +
> +test_expect_failure "NUL in commit message's header" '
> +	test_when_finished "git am --abort" &&
> +	write_nul_patch subject >subject.patch &&
> +	test_must_fail git am subject.patch 2>err &&
> +	grep "a NUL byte in Subject is not allowed" err
> +'
> +
>  test_done