[PATCH 1/2] fsck_commit_buffer(): do not special case the last validation

[PATCH 1/2] fsck_commit_buffer(): do not special case the last validation

[Junio C Hamano]

The pattern taken by all the validations in this function is:

	if (notice a violation exists) {
		err = report(... VIOLATION_KIND ...);
		if (err)
			return err;
	}

where report() returns zero if specified kind of violation is set to
be ignored, and otherwise shows an error message and returns non-zero.

The last validation in the function immediately after the function
returns 0 to declare "all good" can cheat and directly return the
return value from report(), and the current code does so, i.e.

	if (notice a violation exists)
		return report(... VIOLATION_KIND ...);
	return 0;

But that is a selfish code that declares it is the ultimate and
final form of the function, never to be enhanced later.  To allow
and invite future enhancements, make the last test follow the same
pattern.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 fsck.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fsck.c b/fsck.c
index ca4c685..21dfa5f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -666,9 +666,11 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
 	err = fsck_ident(&buffer, &commit->object, options);
 	if (err)
 		return err;
-	if (!commit->tree)
-		return report(options, &commit->object, FSCK_MSG_BAD_TREE, "could not load commit's tree %s", sha1_to_hex(tree_sha1));
-
+	if (!commit->tree) {
+		err = report(options, &commit->object, FSCK_MSG_BAD_TREE, "could not load commit's tree %s", sha1_to_hex(tree_sha1));
+		if (err)
+			return err;
+	}
 	return 0;
 }
 
-- 
2.8.1-355-gcea30bb

[PATCH 2/2] fsck: detect and warn a commit with embedded NUL

[Junio C Hamano]

Even though a Git commit object is designed to be capable of storing
any binary data as its payload, in practice people use it to describe
the changes in textual form, and tools like "git log" are designed to
treat the payload as text.

Detect and warn when we see any commit object with a NUL byte in
it.

Note that a NUL byte in the header part is already detected as a
grave error.  This change is purely about the message part.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 fsck.c          |  8 ++++++++
 t/t1450-fsck.sh | 17 +++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/fsck.c b/fsck.c
index 21dfa5f..3366b3f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -59,6 +59,7 @@
 	FUNC(HAS_DOTGIT, WARN) \
 	FUNC(NULL_SHA1, WARN) \
 	FUNC(ZERO_PADDED_FILEMODE, WARN) \
+	FUNC(NUL_IN_COMMIT, WARN) \
 	/* infos (reported as warnings, but ignored by default) */ \
 	FUNC(BAD_TAG_NAME, INFO) \
 	FUNC(MISSING_TAGGER_ENTRY, INFO)
@@ -610,6 +611,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
 	struct commit_graft *graft;
 	unsigned parent_count, parent_line_count = 0, author_count;
 	int err;
+	const char *buffer_begin = buffer;
 
 	if (verify_headers(buffer, size, &commit->object, options))
 		return -1;
@@ -671,6 +673,12 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
 		if (err)
 			return err;
 	}
+	if (memchr(buffer_begin, '\0', size)) {
+		err = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,
+			     "NUL byte in the commit object body");
+		if (err)
+			return err;
+	}
 	return 0;
 }
 
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index e66b7cb..9012046 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -427,6 +427,23 @@
 	)
 '
 
+test_expect_success 'NUL in commit' '
+	rm -fr nul-in-commit &&
+	git init nul-in-commit &&
+	(
+		cd nul-in-commit &&
+		git commit --allow-empty -m "initial commitQNUL after message" &&
+		git cat-file commit HEAD >original &&
+		q_to_nul <original >munged &&
+		git hash-object -w -t commit --stdin <munged >name &&
+		git branch bad $(cat name) &&
+
+		test_must_fail git -c fsck.nulInCommit=error fsck 2>warn.1 &&
+		git fsck 2>warn.2 &&
+		grep nulInCommit warn.2
+	)
+'
+
 # create a static test repo which is broken by omitting
 # one particular object ($1, which is looked up via rev-parse
 # in the new repository).
-- 
2.8.1-355-gcea30bb

Re: [PATCH 1/2] fsck_commit_buffer(): do not special case the last validation

[Jeff King]

On Thu, Apr 14, 2016 at 11:07:08AM -0700, Junio C Hamano wrote:

> The pattern taken by all the validations in this function is:
> 
> 	if (notice a violation exists) {
> 		err = report(... VIOLATION_KIND ...);
> 		if (err)
> 			return err;
> 	}
> 
> where report() returns zero if specified kind of violation is set to
> be ignored, and otherwise shows an error message and returns non-zero.
> 
> The last validation in the function immediately after the function
> returns 0 to declare "all good" can cheat and directly return the
> return value from report(), and the current code does so, i.e.
> 
> 	if (notice a violation exists)
> 		return report(... VIOLATION_KIND ...);
> 	return 0;
> 
> But that is a selfish code that declares it is the ultimate and
> final form of the function, never to be enhanced later.  To allow
> and invite future enhancements, make the last test follow the same
> pattern.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  fsck.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Patch looks good, and nicely explained.

-Peff

Re: [PATCH 1/2] fsck_commit_buffer(): do not special case the last validation

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 14, 2016 at 11:07:08AM -0700, Junio C Hamano wrote:
>
>> The pattern taken by all the validations in this function is:
>> 
>> 	if (notice a violation exists) {
>> 		err = report(... VIOLATION_KIND ...);
>> 		if (err)
>> 			return err;
>> 	}
>> 
>> where report() returns zero if specified kind of violation is set to
>> be ignored, and otherwise shows an error message and returns non-zero.
>> 
>> The last validation in the function immediately after the function

s/after/before/

>> returns 0 to declare "all good" can cheat and directly return the
>> return value from report(), and the current code does so, i.e.
>> 
>> 	if (notice a violation exists)
>> 		return report(... VIOLATION_KIND ...);
>> 	return 0;
>> 
>> But that is a selfish code that declares it is the ultimate and
>> final form of the function, never to be enhanced later.  To allow
>> and invite future enhancements, make the last test follow the same
>> pattern.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  fsck.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> Patch looks good, and nicely explained.
>
> -Peff

Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL

[Jeff King]

On Thu, Apr 14, 2016 at 11:07:09AM -0700, Junio C Hamano wrote:

> Even though a Git commit object is designed to be capable of storing
> any binary data as its payload, in practice people use it to describe
> the changes in textual form, and tools like "git log" are designed to
> treat the payload as text.
> 
> Detect and warn when we see any commit object with a NUL byte in
> it.
> 
> Note that a NUL byte in the header part is already detected as a
> grave error.  This change is purely about the message part.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks, I was just reading over some of the old threads, and wondering
if it was time to resurrect this idea.

> @@ -610,6 +611,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
>  	struct commit_graft *graft;
>  	unsigned parent_count, parent_line_count = 0, author_count;
>  	int err;
> +	const char *buffer_begin = buffer;
>  
>  	if (verify_headers(buffer, size, &commit->object, options))
>  		return -1;

You need this "buffer_begin" because we move the "buffer" pointer
forward as we parse. But perhaps whole-buffer checks should simply go at
the top (next to verify_headers) before we start advancing the pointer.
To me, that makes the function's flow more natural.

But alternatively...

> @@ -671,6 +673,12 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
>  		if (err)
>  			return err;
>  	}
> +	if (memchr(buffer_begin, '\0', size)) {
> +		err = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,
> +			     "NUL byte in the commit object body");
> +		if (err)
> +			return err;
> +	}

Here we've parsed to the end of the headers we know about. We know
there's no NUL there, because verify_headers() would have complained.
And because the individual header parsers would have complained. So I
actually think we could check from "buffer" (of course we do still need
to record the beginning of the buffer to adjust "size" appropriately).

It's a little more efficient (we don't have to memchr over the same
bytes again). But I'd worry a little that doing it that way would
introduce coupling between this check and verify_headers(), though (so
that if the latter ever changes, our check may start missing cases).

So yet another alternative would be to include this check in
verify_headers(). It would parse to the end of the headers as now, and
then from there additionally look for a NUL in the body.

Of the three approaches, I think I like that third one. It's the most
efficient, and I think the flow is pretty clear. We'd probably want to
rename verify_headers(), though. :)

-Peff

Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +	const char *buffer_begin = buffer;
>>  
>>  	if (verify_headers(buffer, size, &commit->object, options))
>>  		return -1;
>
> You need this "buffer_begin" because we move the "buffer" pointer
> forward as we parse. But perhaps whole-buffer checks should simply go at
> the top (next to verify_headers) before we start advancing the pointer.
> To me, that makes the function's flow more natural.

That was my second iteration.  I didn't want the function return
with warning without checking more serious errors that may be in the
object.

> But alternatively...
>
>> @@ -671,6 +673,12 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
>>  		if (err)
>>  			return err;
>>  	}
>> +	if (memchr(buffer_begin, '\0', size)) {
>> +		err = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,
>> +			     "NUL byte in the commit object body");
>> +		if (err)
>> +			return err;
>> +	}
>
> Here we've parsed to the end of the headers we know about. We know
> there's no NUL there, because verify_headers() would have complained.
> And because the individual header parsers would have complained. So I
> actually think we could check from "buffer" (of course we do still need
> to record the beginning of the buffer to adjust "size" appropriately).

Yes, keeping the "begin" pointer is a cheap way to do an equivalent
of "adjusting size".

> It's a little more efficient (we don't have to memchr over the same
> bytes again). But I'd worry a little that doing it that way would
> introduce coupling between this check and verify_headers(), though (so
> that if the latter ever changes, our check may start missing cases).
>
> So yet another alternative would be to include this check in
> verify_headers(). It would parse to the end of the headers as now, and
> then from there additionally look for a NUL in the body.
>
> Of the three approaches, I think I like that third one. It's the most
> efficient, and I think the flow is pretty clear. We'd probably want to
> rename verify_headers(), though. :)

Sounds sensible, except the "should a mere warning hide potentially
more serious errors?" question remains.

Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL

[Jeff King]

On Thu, Apr 14, 2016 at 02:21:03PM -0400, Jeff King wrote:

> So yet another alternative would be to include this check in
> verify_headers(). It would parse to the end of the headers as now, and
> then from there additionally look for a NUL in the body.

Hmm. Looking at verify_headers(), I think it has another bug.

It wants to "return report(...)" as soon as it sees a problem, and stops
looking for other ones. But the new model for reporting errors is that
report() might ignore a problem if configured to do so.

So I think if you were to ignore NUL_IN_HEADER, then a commit with
NUL_IN_HEADER could still pass fsck, even though it should trigger
UNTERMINATED_HEADER.

-Peff

Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL

[Jeff King]

On Thu, Apr 14, 2016 at 11:25:29AM -0700, Junio C Hamano wrote:

> > You need this "buffer_begin" because we move the "buffer" pointer
> > forward as we parse. But perhaps whole-buffer checks should simply go at
> > the top (next to verify_headers) before we start advancing the pointer.
> > To me, that makes the function's flow more natural.
> 
> That was my second iteration.  I didn't want the function return
> with warning without checking more serious errors that may be in the
> object.

Ah, I didn't consider that. In general I'm not sure the distinction
between "warning" and "error" is all that important, or has been applied
all that consistently. transfer.fsckObjects will barf on either.

But I do agree in general that we should be checking as many things as
we can. And that we are already wrong to return an error immediately
when verify_headers() complains. We should be accumulating problems in
an error code and progressing as far as possible. I think fsck_tree() is
a good example of how to do this.

-Peff

Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 14, 2016 at 02:21:03PM -0400, Jeff King wrote:
>
>> So yet another alternative would be to include this check in
>> verify_headers(). It would parse to the end of the headers as now, and
>> then from there additionally look for a NUL in the body.
>
> Hmm. Looking at verify_headers(), I think it has another bug.
>
> It wants to "return report(...)" as soon as it sees a problem, and stops
> looking for other ones. But the new model for reporting errors is that
> report() might ignore a problem if configured to do so.

Yup, it should be doing the "do not return early unless report says
it is questionable", at least.  Accumulating the error with err |=
report() is an improvement, but even without that, doing the usual

	case '\0':
        	err = report(...);
                return err if err;

would be a real bugfix.

Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I do agree in general that we should be checking as many things as
> we can.

I was about to say "I agree with that in principle, but there are
cases where you would want to say 'if the object does not pass even
this basic check, it is not worth validating it further', and
verify-headers may fall into that category".  That's another way of
saying that something that we cannot even parse into constituent
fields we cannot check the validity of each field for the semantics.

However, with the current "violations of various classes can be
configured out" way of doing things, that is a difficult stance to
take.  If you choose to accept syntax violation that makes us unable
to parse, you must accept warnings and errors coming from "missing"
fields due to our inability to parse and your telling us to proceed
anyway.

So in the end, I do agree that we should be checking as many things
as we can.

Re: [PATCH 1/2] fsck_commit_buffer(): do not special case the last validation

[Johannes Schindelin]

Hi Junio,

On Thu, 14 Apr 2016, Junio C Hamano wrote:

> The pattern taken by all the validations in this function is:
> 
> 	if (notice a violation exists) {
> 		err = report(... VIOLATION_KIND ...);
> 		if (err)
> 			return err;
> 	}
> 
> where report() returns zero if specified kind of violation is set to
> be ignored, and otherwise shows an error message and returns non-zero.
> 
> The last validation in the function immediately after the function
> returns 0 to declare "all good" can cheat and directly return the
> return value from report(), and the current code does so, i.e.
> 
> 	if (notice a violation exists)
> 		return report(... VIOLATION_KIND ...);
> 	return 0;
> 
> But that is a selfish code that declares it is the ultimate and
> final form of the function, never to be enhanced later.  To allow
> and invite future enhancements, make the last test follow the same
> pattern.

FWIW I agree with this reasoning. Sorry for leaving this to you to clean
up.

Ciao,
Dscho

Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL

[Johannes Schindelin]

Hi Junio,

On Thu, 14 Apr 2016, Junio C Hamano wrote:

> Even though a Git commit object is designed to be capable of storing
> any binary data as its payload, in practice people use it to describe
> the changes in textual form, and tools like "git log" are designed to
> treat the payload as text.
> 
> Detect and warn when we see any commit object with a NUL byte in
> it.
> 
> Note that a NUL byte in the header part is already detected as a
> grave error.  This change is purely about the message part.

IIRC there was some discussion going on about this, and since it was 1)
legal and 2) not checked before, I decided not to make this an error.

But of course it is good to introduce a warning!

Thank you,
Dscho

Re: [PATCH 1/2] fsck_commit_buffer(): do not special case the last validation

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> But that is a selfish code that declares it is the ultimate and
>> final form of the function, never to be enhanced later.  To allow
>> and invite future enhancements, make the last test follow the same
>> pattern.
>
> FWIW I agree with this reasoning. Sorry for leaving this to you to clean
> up.

Thanks. I often fall into the same trap, and I suspect everybody
does. After doing a large-ish change, it is easy to think that what
was just finished is the final one for quite some time to come,
without even realizing that I am being selfish.