Re: [PATCH v2] tag: refuse tag messages that contain NULs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 21, 2012, 20:47 UTC
- Message-ID
- <7vpqd7gbhx.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1329741483-22567-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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.
Show 16 quoted lines
> 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?
Show 12 quoted lines
> 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.