Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> It's not about after those. It's about right before write_sha1_file().
> I wanted to catch all NULs no matter how they come. But yes the check
> should happen early to avoid wasting user's time (e.g. doing signing)
No, it is not about that. It is about checking _the input_. If we had
bugs in do_sign() that adds what we do not want, that is not a user's
fault and "a NUL byte in tag message not allowed" is an inappropriate
thing to give to the user.
And giving "We screwed up and added NUL that you cannot work around to
remove, sorry, you hit a bug." is not very useful.
> So how about this?
> ...
> diff --git a/sha1_file.c b/sha1_file.c
> index 88f2151..2fc8623 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2519,6 +2519,12 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
> char hdr[32];
> int hdrlen;
>
> + /* GIT_HASH_NUL is for the test suite to hash abitrary content */
> + if (!getenv("GIT_HASH_NUL") &&
> + (!strcmp(type, commit_type) || !strcmp(type, tag_type)) &&
> + memchr(buf, '\0', len))
> + return error("BUG: %s message contains NUL.", type);
> +
This is yucky. Is this really worth it?
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index e93ac73..8cb13e5 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1269,4 +1269,8 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
> test_must_fail git tag -v -s
> '
>
> +test_expect_success 'tag content contains NUL' '
> + test_must_fail git tag -F "$TEST_DIRECTORY"/t3900/UTF-16.txt utf16
> +'
> +
This is caught without the change to write_sha1_file(), isn't it? If so,
I would say we should drop that GIT_HASH_NUL hunk.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html