"git fsck" not detecting garbage at the end of blob object files...

I was perusing StackOverflow this morning and ran across this
question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/

It was a simple question about why "checking objects" was not appearing, but in it was another issue. The user purposefully corrupted a blob object file to see if git fsck would catch it by tacking extra data on at the end. git fsck happily said everything was okay, but when I played with things locally I found out that git gc does not like that extra garbage. I'm not sure what the trade-off needs to be here, but my expectation is that if git fsck says everything is okay, then all operations using that object (file) should work too.

Is that unreasonable? What would be the impact of fixing this issue?

-John

If you do this with a commit object or tree object, fsck does complain. I think it's sensible to do so for blob objects as well.

Editing blob object:

hurricane:/tmp/moo (master)$ hexer .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485
hurricane:/tmp/moo (master)$ git status
On branch master
nothing to commit, working tree clean
hurricane:/tmp/moo (master)$ git fsck
Checking object directories: 100% (256/256), done.
hurricane:/tmp/moo (master)$ git gc
Counting objects: 3, done.
error: garbage at end of loose object 'a1b3ebb97f10ff8d85a9472bcba50cb575dbd485'
fatal: loose object a1b3ebb97f10ff8d85a9472bcba50cb575dbd485 (stored in .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485) is corrupt
error: failed to run repack

Editing tree object:

hurricane:/tmp/moo (master)$ hexer .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt

Editing commit object:

hurricane:/tmp/moo (master)$ echo test >> .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt !(128) hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt

D.

The existing extra-garbage check is in unpack_sha1_rest(), which is called as part of read_sha1_file(). And that's what we hit for commits and trees. However, we check the sha1 of blobs using the streaming interface (in case they're large). I think you'd want to put a similar check into read_istream_loose(). But note if you are grepping for it, it is hidden behind a macro; look for read_method_decl(loose).

I'm actually not sure if this should be downgrade to a warning. It's true that it's a form of corruption, but it doesn't actually prohibit us from getting the data we need to complete the operation. Arguably fsck should be more picky, but it is just relying on the same parse_object() code path that the rest of git uses.

I doubt anybody cares too much either way, though. It's not like this is a common thing.

I did notice another interesting case when looking at this. Fsck ends up in fsck_loose(), which has the sha1 and path of the loose object. It passes the sha1 to fsck_sha1(), and ignores the path entirely!

So if you have a duplicate copy of the object in a pack, we'd actually find and check the duplicate. This can happen, e.g., if you had a loose object and fetched a thin-pack which made a copy of the loose object to complete the pack).

Probably fsck_loose() should be more picky about making sure we are reading the data from the loose version we found.

-Peff

That's for the pointer.

I kind of wonder about that myself too, and I'm not sure what to think about it. On the one hand, I'd like to know about *anything* that has changed in an adverse way--it could indicate a failure somewhere else that needs to be handled. On the other hand, scaring the user isn't all that advantageous. I guess I'm in the former camp.

As to whether this is common, yeah, it's probably not. However, I was surprised by the number of results that turned up when I search for "garbage at end of loose object".

Interesting find! Thanks for the information Peff!

-John

So I figured I would knock this out as a fun morning exercise. But sheesh, it turned out to be a slog, because most of the functions rely on map_sha1_file() to convert the sha1 to an object path at the lowest level.

But I finally got something working, so here it is. I found another bug on the way, along with a few cleanups. And then I did the trailing garbage detection at the end, because by that point I knew right where it needed to go. :)

[1/6]: t1450: refactor loose-object removal [2/6]: sha1_file: fix error message for alternate objects [3/6]: t1450: test fsck of packed objects
[4/6]: sha1_file: add read_loose_object() function [5/6]: fsck: parse loose object paths directly [6/6]: fsck: detect trailing garbage in all object types

 builtin/fsck.c  |  46 +++++++++++----
 cache.h         |  13 ++++
 sha1_file.c     | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 t/t1450-fsck.sh |  86 +++++++++++++++++++++++----
 4 files changed, 284 insertions(+), 41 deletions(-)

-Peff

Commit 90cf590f5 (fsck: optionally show more helpful info for broken links, 2016-07-17) added a remove_loose_object() helper, but we already had a remove_object() helper that did the same thing. Let's combine these into one.

The implementations had a few subtle differences, so I've tried to take the best of both:

- the original used "sed", but the newer version avoids spawning an extra process

- the original processed "$*", which was nonsense, as it assumed only a single sha1. Use "$1" to make that more clear.

- the newer version ran an extra rev-parse, but it was not necessary; it's sole caller already converted the argument into a raw sha1

- the original used "rm -f", whereas the new one uses "rm". The latter is better because it may notice a bug or other unexpected failure in the test. (The original does check that the object exists before we remove it, which is good, but that's a subset of the possible unexpected conditions).

---
 t/t1450-fsck.sh | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

index ee7d4736d..3297d4cb2 100755
@@ -43,13 +43,13 @@ test_expect_success 'HEAD is part of refs, valid objects appear valid' '
 
 test_expect_success 'setup: helpers for corruption tests' '
 	sha1_file() {
-		echo "$*" | sed "s#..#.git/objects/&/#"
+		remainder=${1#??} &&
+		firsttwo=${1%$remainder} &&
+		echo ".git/objects/$firsttwo/$remainder"
 	} &&
 
 	remove_object() {
-		file=$(sha1_file "$*") &&
-		test -e "$file" &&
-		rm -f "$file"
+		rm "$(sha1_file "$1")"
 	}
 '
 

@@ -535,13 +535,6 @@ test_expect_success 'fsck --connectivity-only' '
 	)
 '
 
-remove_loose_object () {
-	sha1="$(git rev-parse "$1")" &&
-	remainder=${sha1#??} &&
-	firsttwo=${sha1%$remainder} &&
-	rm .git/objects/$firsttwo/$remainder
-}
-
 test_expect_success 'fsck --name-objects' '
 	rm -rf name-objects &&
 	git init name-objects &&

@@ -550,7 +543,7 @@ test_expect_success 'fsck --name-objects' '
 		test_commit julius caesar.t &&
 		test_commit augustus &&
 		test_commit caesar &&
-		remove_loose_object $(git rev-parse julius:caesar.t) &&
+		remove_object $(git rev-parse julius:caesar.t) &&
 		test_must_fail git fsck --name-objects >out &&
 		tree=$(git rev-parse --verify julius:) &&
 		grep "$tree (\(refs/heads/master\|HEAD\)@{[0-9]*}:" out

2.11.0.629.g10075098c

When we fail to open a corrupt loose object, we report an error and mention the filename via sha1_file_name(). However, that function will always give us a path in the local repository, whereas the corrupt object may have come from an alternate. The result is a very misleading error message.

Teach the open_sha1_file() and stat_sha1_file() helpers to pass back the path they found, so that we can report it correctly.

Note that the pointers we return go to static storage (e.g., from sha1_file_name()), which is slightly dangerous. However, these helpers are static local helpers, and the names are used for immediately generating error messages. The simplicity is an acceptable tradeoff for the danger.

---
 sha1_file.c     | 46 +++++++++++++++++++++++++++++++---------------
 t/t1450-fsck.sh | 10 ++++++++++
 2 files changed, 41 insertions(+), 15 deletions(-)

index 1eb47f611..c6b990f41 100644
@@ -1630,39 +1630,54 @@ int git_open_cloexec(const char *name, int flags)
 	return fd;
 }
 
-static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
+/*
+ * Find "sha1" as a loose object in the local repository or in an alternate.
+ * Returns 0 on success, negative on failure.
+ *
+ * The "path" out-parameter will give the path of the object we found (if any).
+ * Note that it may point to static storage and is only valid until another
+ * call to sha1_file_name(), etc.
+ */
+static int stat_sha1_file(const unsigned char *sha1, struct stat *st,
+			  const char **path)
 {
 	struct alternate_object_database *alt;
 
-	if (!lstat(sha1_file_name(sha1), st))
+	*path = sha1_file_name(sha1);
+	if (!lstat(*path, st))
 		return 0;
 
 	prepare_alt_odb();
 	errno = ENOENT;
 	for (alt = alt_odb_list; alt; alt = alt->next) {
-		const char *path = alt_sha1_path(alt, sha1);
-		if (!lstat(path, st))
+		*path = alt_sha1_path(alt, sha1);
+		if (!lstat(*path, st))
 			return 0;
 	}
 
 	return -1;
 }
 
-static int open_sha1_file(const unsigned char *sha1)
+/*
+ * Like stat_sha1_file(), but actually open the object and return the
+ * descriptor. See the caveats on the "path" parameter above.
+ */
+static int open_sha1_file(const unsigned char *sha1, const char **path)
 {
 	int fd;
 	struct alternate_object_database *alt;
 	int most_interesting_errno;
 
-	fd = git_open(sha1_file_name(sha1));
+	*path = sha1_file_name(sha1);
+	fd = git_open(*path);
 	if (fd >= 0)
 		return fd;
 	most_interesting_errno = errno;
 
 	prepare_alt_odb();
 	for (alt = alt_odb_list; alt; alt = alt->next) {
-		const char *path = alt_sha1_path(alt, sha1);
-		fd = git_open(path);
+		*path = alt_sha1_path(alt, sha1);
+		fd = git_open(*path);
 		if (fd >= 0)
 			return fd;
 		if (most_interesting_errno == ENOENT)

@@ -1674,10 +1689,11 @@ static int open_sha1_file(const unsigned char *sha1)
 
 void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
 {
+	const char *path;
 	void *map;
 	int fd;
 
-	fd = open_sha1_file(sha1);
+	fd = open_sha1_file(sha1, &path);
 	map = NULL;
 	if (fd >= 0) {
 		struct stat st;

@@ -1686,7 +1702,7 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)
 			*size = xsize_t(st.st_size);
 			if (!*size) {
 				/* mmap() is forbidden on empty files */
-				error("object file %s is empty", sha1_file_name(sha1));
+				error("object file %s is empty", path);
 				return NULL;
 			}
 			map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);

@@ -2806,8 +2822,9 @@ static int sha1_loose_object_info(const unsigned char *sha1,
 	 * object even exists.
 	 */
 	if (!oi->typep && !oi->typename && !oi->sizep) {
+		const char *path;
 		struct stat st;
-		if (stat_sha1_file(sha1, &st) < 0)
+		if (stat_sha1_file(sha1, &st, &path) < 0)
 			return -1;
 		if (oi->disk_sizep)
 			*oi->disk_sizep = st.st_size;

@@ -3003,6 +3020,8 @@ void *read_sha1_file_extended(const unsigned char *sha1,
 {
 	void *data;
 	const struct packed_git *p;
+	const char *path;
+	struct stat st;
 	const unsigned char *repl = lookup_replace_object_extended(sha1, flag);
 
 	errno = 0;

@@ -3018,12 +3037,9 @@ void *read_sha1_file_extended(const unsigned char *sha1,
 		die("replacement %s not found for %s",
 		    sha1_to_hex(repl), sha1_to_hex(sha1));
 
-	if (has_loose_object(repl)) {
-		const char *path = sha1_file_name(sha1);
-
+	if (!stat_sha1_file(repl, &st, &path))
 		die("loose object %s (stored in %s) is corrupt",
 		    sha1_to_hex(repl), path);
-	}
 
 	if ((p = has_packed_and_bad(repl)) != NULL)
 		die("packed object %s (stored in %s) is corrupt",

index 3297d4cb2..f95174c9d 100755
@@ -550,4 +550,14 @@ test_expect_success 'fsck --name-objects' '
 	)
 '
 
+test_expect_success 'alternate objects are correctly blamed' '
+	test_when_finished "rm -rf alt.git .git/objects/info/alternates" &&
+	git init --bare alt.git &&
+	echo "../../alt.git/objects" >.git/objects/info/alternates &&
+	mkdir alt.git/objects/12 &&
+	>alt.git/objects/12/34567890123456789012345678901234567890 &&
+	test_must_fail git fsck >out 2>&1 &&
+	grep alt.git out
+'
+
 test_done

2.11.0.629.g10075098c

The code paths in fsck for packed and loose objects are quite different, and it is not immediately obvious that the packed case behaves well. In particular:

1. The fsck_loose() function always returns "0" to tell the iterator to keep checking more objects. Whereas fsck_obj_buffer() (which handles packed objects) returns -1. This is OK, because the callback machinery for verify_pack() does not stop when it sees a non-zero return.

2. The fsck_loose() function sets the ERROR_OBJECT bit when fsck_obj() fails, whereas fsck_obj_buffer() sets it only when it sees a corrupt object. This turns out not to matter. We don't actually do anything with this bit except exit the program with a non-zero code, and that is handled already by the non-zero return from the function.

So there are no bugs here, but it was certainly confusing to me. And we do not test either of the properties in t1450 (neither that a non-corruption error will caused a non-zero exit for a packed object, nor that we keep going after seeing the first error). Let's test both of those conditions, so that we'll notice if any of those assumptions becomes invalid.

---
 t/t1450-fsck.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

index f95174c9d..c39d42120 100755
@@ -560,4 +560,25 @@ test_expect_success 'alternate objects are correctly blamed' '
 	grep alt.git out
 '
 
+test_expect_success 'fsck errors in packed objects' '
+	git cat-file commit HEAD >basis &&
+	sed "s/</one/" basis >one &&
+	sed "s/</foo/" basis >two &&
+	one=$(git hash-object -t commit -w one) &&
+	two=$(git hash-object -t commit -w two) &&
+	pack=$(
+		{
+			echo $one &&
+			echo $two
+		} | git pack-objects .git/objects/pack/pack
+	) &&
+	test_when_finished "rm -f .git/objects/pack/pack-$pack.*" &&
+	remove_object $one &&
+	remove_object $two &&
+	test_must_fail git fsck 2>out &&
+	grep "error in commit $one.* - bad name" out &&
+	grep "error in commit $two.* - bad name" out &&
+	! grep corrupt out
+'
+
 test_done

2.11.0.629.g10075098c

It's surprisingly hard to ask the sha1_file code to open a _specific_ incarnation of a loose object. Most of the functions take a sha1, and loop over the various object types (packed versus loose) and locations (local versus alternates) at a low level.

However, some tools like fsck need to look at a specific file. This patch gives them a function they can use to open the loose object at a given path.

The implementation unfortunately ends up repeating bits of related functions, but there's not a good way around it without some major refactoring of the whole sha1_file stack. We need to mmap the specific file, then partially read the zlib stream to know whether we're streaming or not, and then finally either stream it or copy the data to a buffer.

We can do that by assembling some of the more arcane internal sha1_file functions, but we end up having to essentially reimplement unpack_sha1_file(), along with the streaming bits of check_sha1_signature().

Still, most of the ugliness is contained in the new function, and the interface is clean enough that it may be reusable (though it seems unlikely anything but git-fsck would care about opening a specific file).

---
 cache.h     |  13 ++++++
 sha1_file.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 143 insertions(+), 3 deletions(-)

index 1b67f078d..33f1c2fa7 100644
@@ -1140,6 +1140,19 @@ extern int finalize_object_file(const char *tmpfile, const char *filename);
 
 extern int has_sha1_pack(const unsigned char *sha1);
 
+/*
+ * Open the loose object at path, check its sha1, and return the contents,
+ * type, and size. If the object is a blob, then "contents" may return NULL,
+ * to allow streaming of large blobs.
+ *
+ * Returns 0 on success, negative on error (details may be written to stderr).
+ */
+int read_loose_object(const char *path,
+		      const unsigned char *expected_sha1,
+		      enum object_type *type,
+		      unsigned long *size,
+		      void **contents);
+
 /*
  * Return true iff we have an object named sha1, whether local or in
  * an alternate object database, and whether packed or loose.  This

2.11.0.629.g10075098c

When we iterate over the list of loose objects to check, we get the actual path of each object. But we then throw it away and pass just the sha1 to fsck_sha1(), which will do a fresh lookup. Usually it would find the same object, but it may not if an object exists both as a loose and a packed object. We may end up checking the packed object twice, and never look at the loose one.

In practice this isn't too terrible, because if fsck doesn't complain, it means you have at least one good copy. But since the point of fsck is to look for corruption, we should be thorough.

The new read_loose_object() interface can help us get the data from disk, and then we replace parse_object() with parse_object_buffer(). As a bonus, our error messages now mention the path to a corrupted object, which should make it easier to track down errors when they do happen.

---
 builtin/fsck.c  | 46 +++++++++++++++++++++++++++++++++-------------
 t/t1450-fsck.sh | 16 ++++++++++++++++
 2 files changed, 49 insertions(+), 13 deletions(-)

index f01b81eeb..4b91ee95e 100644
@@ -362,18 +362,6 @@ static int fsck_obj(struct object *obj)
 	return 0;
 }
 
-static int fsck_sha1(const unsigned char *sha1)
-{
-	struct object *obj = parse_object(sha1);
-	if (!obj) {
-		errors_found |= ERROR_OBJECT;
-		return error("%s: object corrupt or missing",
-			     sha1_to_hex(sha1));
-	}
-	obj->flags |= HAS_OBJ;
-	return fsck_obj(obj);
-}
-
 static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,
 			   unsigned long size, void *buffer, int *eaten)
 {

@@ -488,9 +476,41 @@ static void get_default_heads(void)
 	}
 }
 
+static struct object *parse_loose_object(const unsigned char *sha1,
+					 const char *path)
+{
+	struct object *obj;
+	void *contents;
+	enum object_type type;
+	unsigned long size;
+	int eaten;
+
+	if (read_loose_object(path, sha1, &type, &size, &contents) < 0)
+		return NULL;
+
+	if (!contents && type != OBJ_BLOB)
+		die("BUG: read_loose_object streamed a non-blob");
+
+	obj = parse_object_buffer(sha1, type, size, contents, &eaten);
+
+	if (!eaten)
+		free(contents);
+	return obj;
+}
+
 static int fsck_loose(const unsigned char *sha1, const char *path, void *data)
 {
-	if (fsck_sha1(sha1))
+	struct object *obj = parse_loose_object(sha1, path);
+
+	if (!obj) {
+		errors_found |= ERROR_OBJECT;
+		error("%s: object corrupt or missing: %s",
+		      sha1_to_hex(sha1), path);
+		return 0; /* keep checking other objects */
+	}
+
+	obj->flags = HAS_OBJ;
+	if (fsck_obj(obj))
 		errors_found |= ERROR_OBJECT;
 	return 0;
 }

index c39d42120..455c186fe 100755
@@ -581,4 +581,20 @@ test_expect_success 'fsck errors in packed objects' '
 	! grep corrupt out
 '
 
+test_expect_success 'fsck finds problems in duplicate loose objects' '
+	rm -rf broken-duplicate &&
+	git init broken-duplicate &&
+	(
+		cd broken-duplicate &&
+		test_commit duplicate &&
+		# no "-d" here, so we end up with duplicates
+		git repack &&
+		# now corrupt the loose copy
+		file=$(sha1_file "$(git rev-parse HEAD)") &&
+		rm "$file" &&
+		echo broken >"$file" &&
+		test_must_fail git fsck
+	)
+'
+
 test_done

2.11.0.629.g10075098c

While playing around with having a GIT_TEST_FSCK=true as I suggested in
https://public-inbox.org/git/[email protected]/ I found that we've had an infinite loop in git-fsck since c68b489e56 ("fsck: parse loose object paths directly", 2017-01-13)

In particular in the while() loop added by f6371f9210 ("sha1_file: add read_loose_object() function", 2017-01-13) in the check_stream_sha1() function.

To reproduce just:

    (
        cd t &&
        ./t5000-tar-tree.sh -d &&
        git -C trash\ directory.t5000-tar-tree/ fsck
    )

Before we'd print:

    error: sha1 mismatch 19f9c8273ec45a8938e6999cb59b3ff66739902a
    error: 19f9c8273ec45a8938e6999cb59b3ff66739902a: object corrupt or missing
    Checking object directories: 100% (256/256), done.
    missing blob 19f9c8273ec45a8938e6999cb59b3ff66739902a

Now we just hang on:

    Checking object directories:   9% (24/256)

I have no idea if this makes sense, but this fixes it and we pass all the fsck tests with it:

    diff --git a/sha1-file.c b/sha1-file.c
    index dd0b6aa873..fffc31458e 100644
    --- a/sha1-file.c
    +++ b/sha1-file.c
    @@ -2182,7 +2182,7 @@ static int check_stream_sha1(git_zstream *stream,
     	git_hash_ctx c;
     	unsigned char real_sha1[GIT_MAX_RAWSZ];
     	unsigned char buf[4096];
    -	unsigned long total_read;
    +	unsigned long total_read, last_total_read;
     	int status = Z_OK;

     	the_hash_algo->init_fn(&c);
    @@ -2193,6 +2193,7 @@ static int check_stream_sha1(git_zstream *stream,
     	 * do not count against the object's content size.
     	 */
     	total_read = stream->total_out - strlen(hdr) - 1;
    +	last_total_read = total_read;

     	/*
     	 * This size comparison must be "<=" to read the final zlib packets;
    @@ -2207,6 +2208,9 @@ static int check_stream_sha1(git_zstream *stream,
     		status = git_inflate(stream, Z_FINISH);
     		the_hash_algo->update_fn(&c, buf, stream->next_out - buf);
     		total_read += stream->next_out - buf;
    +		if (last_total_read == total_read)
    +			return -1;
    +		last_total_read = total_read;
     	}
     	git_inflate_end(stream);

I.e. we get into a loop where total_read isn't increasing. We no longer print "sha1 mismatch" but maybe that's an emergent effect of something else. Haven't checked.

The test is easy, just add a 'git fsck' at the end of t5000-tar-tree.sh, but more generally it seems having something like GIT_TEST_FSCK=true is a good idea. We do a bunch of stress testing of the object store in the test suite that we're unlikely to encounter in the wild.

Of course my idea of how to do that in my <[email protected]> would be counterproductive, i.e. it seems we want to catch all the cases where there's a bad fsck, just that it returns in a certain way.

So maybe a good approach would be that we'd annotate all those test whose fsck fails with "this is how it should fail", and run those tests under GIT_TEST_FSCK=true, and GIT_TEST_FSCK=true would also be asserting that no tests other than those marked as failing the fsck check at the end fail it.

Thanks, I was easily able to reproduce.

The problem isn't actually a sha1 mismatch, though that's what parse_object() will report. The issue is actually that the file is truncated. So zlib does not say "this is corrupt", but rather "I need more bytes to keep going". And unfortunately it returns Z_BUF_ERROR both for "I need more bytes" (in which we know we are truncated, because we fed the whole mmap'd file in the first place) as well as "I need more output buffer space" (which just means we should keep looping!).

So we need to distinguish those cases. I think this is the simplest fix:

index dd0b6aa873..a7ff5fe25d 100644
@@ -2199,6 +2199,7 @@ static int check_stream_sha1(git_zstream *stream,
 	 * see the comment in unpack_sha1_rest for details.
 	 */
 	while (total_read <= size &&
+	       stream->avail_in > 0 &&
 	       (status == Z_OK || status == Z_BUF_ERROR)) {
 		stream->next_out = buf;
 		stream->avail_out = sizeof(buf);

This works just by checking that we are making forward progress in the output buffer. I think that would _probably_ be OK for this case, since we know we have all of the input available. But in a case where we're feeding the input in a stream, it would not be. It's possible there that we would not create any output in one round, but would do so after feeding more input bytes.

I think the patch I showed above addresses the root cause more directly. I'll wrap that up in a real commit, but I think there may be some related work:

- "git show 19f9c827" does complain with "sha1 mismatch" (which isn't strictly correct, but is probably good enough). However, "git cat-file blob 19f9c827" exits non-zero without printing anything. It probably should complain more loudly.

- the offending loop comes from f6371f9210. But that commit was mostly cargo-culting other parts of sha1-file.c. I'm worried that this bug exists elsewhere, too. I'll dig around to see if I can find other instances.

-Peff

Yes, exactly.

Thanks.

Hmm, yeah, good thinking. I think zlib could consume that final byte into its internal buffer.

As part of my digging, I looked at how the loose streaming code handles this. It checks that when we see Z_BUF_ERROR, we actually did run out of output bytes (so if we didn't, then we know it's not the case we expected to be looping on).

I have some patches almost ready to send; I'll use that technique.

-Peff

And here they are.

[1/3]: t1450: check large blob in trailing-garbage test [2/3]: check_stream_sha1(): handle input underflow [3/3]: cat-file: handle streaming failures consistently

 builtin/cat-file.c | 16 ++++++++++++----
 sha1-file.c        |  3 ++-
 t/t1450-fsck.sh    | 23 +++++++++++++++++++++--
 3 files changed, 35 insertions(+), 7 deletions(-)

-Peff

Commit cce044df7f (fsck: detect trailing garbage in all object types, 2017-01-13) added two tests of trailing garbage in a loose object file: one with a commit and one with a blob. The point of having two is that blobs would follow a different code path that streamed the contents, instead of loading it into a buffer as usual.

At the time, merely being a blob was enough to trigger the streaming code path. But since 7ac4f3a007 (fsck: actually fsck blob data, 2018-05-02), we now only stream blobs that are actually large. So since then, the streaming code path is not tested at all for this case.

We can restore the original intent of the test by tweaking core.bigFileThreshold to make our small blob seem large. There's no easy way to externally verify that we followed the streaming code path, but I did check before/after using a temporary debug statement.

---
I prepared this series on master, but it occurs to me you may want to apply patch 2 on top of f6371f9210 or thereabouts, which introduced the bug it fixes. If so, then obviously this one doesn't make sense back then, and should go on top of 7ac4f3a007. It should be semantically independent, though there may be a minor text conflict.

 t/t1450-fsck.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 0f2dd26f74..3421f12e8a 100755
@@ -673,13 +673,13 @@ test_expect_success 'fsck detects trailing loose garbage (commit)' '
 	test_i18ngrep "garbage.*$commit" out
 '
 
-test_expect_success 'fsck detects trailing loose garbage (blob)' '
+test_expect_success 'fsck detects trailing loose garbage (large blob)' '
 	blob=$(echo trailing | git hash-object -w --stdin) &&
 	file=$(sha1_file $blob) &&
 	test_when_finished "remove_object $blob" &&
 	chmod +w "$file" &&
 	echo garbage >>"$file" &&
-	test_must_fail git fsck 2>out &&
+	test_must_fail git -c core.bigfilethreshold=5 fsck 2>out &&
 	test_i18ngrep "garbage.*$blob" out
 '
 

2.19.1.1235.g6b27db57c2

This commit fixes an infinite loop when fscking large truncated loose objects.

The check_stream_sha1() function takes an mmap'd loose object buffer and streams 4k of output at a time, checking its sha1. The loop quits when we've output enough bytes (we know the size from the object header), or when zlib tells us anything except Z_OK or Z_BUF_ERROR.

The latter is expected because zlib may run out of room in our 4k buffer, and that is how it tells us to process the output and loop again.

But Z_BUF_ERROR also covers another case: one in which zlib cannot make forward progress because it needs more _input_. This should never happen in this loop, because though we're streaming the output, we have the entire deflated input available in the mmap'd buffer. But since we don't check this case, we'll just loop infinitely if we do see a truncated object, thinking that zlib is asking for more output space.

It's tempting to fix this by checking stream->avail_in as part of the loop condition (and quitting if all of our bytes have been consumed). But that assumes that once zlib has consumed the input, there is nothing left to do. That's not necessarily the case: it may have read our input into its internal state, but still have bytes to output.

Instead, let's continue on Z_BUF_ERROR only when we see the case we're expecting: the previous round filled our output buffer completely. If it didn't (and we still saw Z_BUF_ERROR), we know something is wrong and should break out of the loop.

The bug comes from commit f6371f9210 (sha1_file: add read_loose_object() function, 2017-01-13), which reimplemented some of the existing loose object functions. So it's worth checking if this bug was inherited from any of those. The answers seems to be no. The two obvious candidates are both OK:

1. unpack_sha1_rest(); this doesn't need to loop on Z_BUF_ERROR at all, since it allocates the expected output buffer in advance (which we can't do since we're explicitly streaming here)

2. check_object_signature(); the streaming path relies on the istream interface, which uses read_istream_loose() for this case. That function uses a similar "is our output buffer full" check with Z_BUF_ERROR (which is where I stole it from for this patch!)

---
 sha1-file.c     |  3 ++-
 t/t1450-fsck.sh | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

index dd0b6aa873..2daf7d9935 100644
@@ -2199,7 +2199,8 @@ static int check_stream_sha1(git_zstream *stream,
 	 * see the comment in unpack_sha1_rest for details.
 	 */
 	while (total_read <= size &&
-	       (status == Z_OK || status == Z_BUF_ERROR)) {
+	       (status == Z_OK ||
+		(status == Z_BUF_ERROR && !stream->avail_out))) {
 		stream->next_out = buf;
 		stream->avail_out = sizeof(buf);
 		if (size - total_read < stream->avail_out)

index 3421f12e8a..b5677d26a4 100755
@@ -683,6 +683,25 @@ test_expect_success 'fsck detects trailing loose garbage (large blob)' '
 	test_i18ngrep "garbage.*$blob" out
 '
 
+test_expect_success 'fsck detects truncated loose object' '
+	# make it big enough that we know we will truncate in the data
+	# portion, not the header
+	test-tool genrandom truncate 4096 >file &&
+	blob=$(git hash-object -w file) &&
+	file=$(sha1_file $blob) &&
+	test_when_finished "remove_object $blob" &&
+	test_copy_bytes 1024 <"$file" >tmp &&
+	rm "$file" &&
+	mv -f tmp "$file" &&
+
+	# check both regular and streaming code paths
+	test_must_fail git fsck 2>out &&
+	test_i18ngrep corrupt.*$blob out &&
+
+	test_must_fail git -c core.bigfilethreshold=128 fsck 2>out &&
+	test_i18ngrep corrupt.*$blob out
+'
+
 # for each of type, we have one version which is referenced by another object
 # (and so while unreachable, not dangling), and another variant which really is
 # dangling.

2.19.1.1235.g6b27db57c2

See 692f0bc7 to find who did the fix you stole from, and for what kind of breakage the original fix was made.

By the way, a very similar loop for pack_non_delta istream iterates while total_read is smaller than sz, but it does not have the same check upon BUF_ERROR to see if we've read everything.

Heh. I did not dig into it, but actually thought "I'll bet Junio had to get this right when he wrote the streaming code. No wonder he spotted my mistake so quickly!".

Indeed. Did you find that one by inspection, or did you peek at:

https://public-inbox.org/git/[email protected]/

? :)

-Peff

I looked for BUF_ERROR in the streaming.c and found two instances in a very similar looking loop with a subtle differnce, and the difference was due to one of them getting fixed in the past while the other one was left intact as written at its inception.

I should have looked for that message to read the part below three-dash mark. Or we may want to transplant that comment somehow to the function so next person will not be puzzled like I did?

Hmm. Reading that function, I am not sure if it actually might need fixing.

Might we actually get Z_BUF_ERROR asking for more input if zlib reads to the end of the pack window? That is probably quite unlikely in practice, but in theory you could feed a very large buffer for the output and use a very small pack window.

So I do not think we can use the same logic in that loop. But at the same time, what prevents use_pack() from getting to the very end of the pack and saying "I have no bytes left for you"? And then we'd loop infinitely, feeding zlib nothing.

I'm not sure what the solution is. I do not think this works:

index d1e6b2dce6..a92a85ed10 100644
@@ -394,6 +394,9 @@ static read_method_decl(pack_non_delta)
 		mapped = use_pack(st->u.in_pack.pack, &window,
 				  st->u.in_pack.pos, &st->z.avail_in);
 
+		if (!st->z.avail_in)
+			break;
+
 		st->z.next_out = (unsigned char *)buf + total_read;
 		st->z.avail_out = sz - total_read;
 		st->z.next_in = mapped;

because we may have read to the very end but still have bytes to output.

Though hrm. I think use_pack() will always tell us about the trailing 20-byte hash in the "avail" window. Which means we should never legitimately get to 0 there, because it means that either:

1. We're reading the trailing hash, which cannot possibly be right (in most cases I'd expect zlib to barf at that point anyway, but of course it's possible to have a hash that is valid zlib data ;) ).

2. We're truncated _before_ the hash, so we really did read to EOF, and there are no more bytes. I suspect we may actually detect this case upon opening the pack (since we do peek at the trailer then), but again that could be fooled by coincidence.

I guess that's not the whole story, though. use_pack() tries to promise at least 20 bytes (to simplify some of the other parsing routines). So we shouldn't actually ever get "0" here. If we really are that close to the end of the pack, we'd hit this logic in use_pack:

if (offset > (p->pack_size - the_hash_algo->rawsz))

	die("offset beyond end of packfile (truncated pack?)");

So actually, I think this code is OK as-is. We will always have at least 20 bytes of input, or use_pack() will die.

Phew. I almost just deleted all of the above, because now I think I'm ready to write that comment you asked for. ;) But I left it since maybe it makes sense to follow my thought process.

-Peff

So here it is in a more succinct form.

-Peff

-- >8 --
Subject: [PATCH] read_istream_pack_non_delta(): document input handling

Twice now we have scratched our heads about why the loose streaming code needs the protection added by 692f0bc7ae (avoid infinite loop in read_istream_loose, 2013-03-25), but the similar code in its pack counterpart does not.

The short answer is that use_pack() will die before it lets us run out of bytes. Note that this could mean reading garbage (including the trailing hash) from the packfile in some cases of corruption, but that's OK. zlib will notice and complain (and if not, certainly the end result will not match the object hash we expect).

Let's leave a comment this time to document our findings.

---
 streaming.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index d1e6b2dce6..ac7c7a22f9 100644
@@ -408,6 +408,15 @@ static read_method_decl(pack_non_delta)
 			st->z_state = z_done;
 			break;
 		}
+
+		/*
+		 * Unlike the loose object case, we do not have to worry here
+		 * about running out of input bytes and spinning infinitely. If
+		 * we get Z_BUF_ERROR due to too few input bytes, then we'll
+		 * replenish them in the next use_pack() call when we loop. If
+		 * we truly hit the end of the pack (i.e., because it's corrupt
+		 * or truncated), then use_pack() catches that and will die().
+		 */
 		if (status != Z_OK && status != Z_BUF_ERROR) {
 			git_inflate_end(&st->z);
 			st->z_state = z_error;

2.19.1.1298.g19f18f2a22

Thanks.

There are three ways to convince cat-file to stream a blob:

- cat-file -p $blob

- cat-file blob $blob

- echo $batch | cat-file --batch

In the first two, we simply exit with the error code of streaw_blob_to_fd(). That means that an error will cause us to exit with "-1" (which we try to avoid) without printing any kind of error message (which is confusing to the user).

Instead, let's match the third case, which calls die() on an error. Unfortunately we cannot be more specific, as stream_blob_to_fd() does not tell us whether the problem was on reading (e.g., a corrupt object) or on writing (e.g., ENOSPC). That might be an opportunity for future work, but for now we will at least exit with a sane message and exit code.

---
 builtin/cat-file.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

index 8d97c84725..0d403eb77d 100644
@@ -50,6 +50,13 @@ static int filter_object(const char *path, unsigned mode,
 	return 0;
 }
 
+static int stream_blob(const struct object_id *oid)
+{
+	if (stream_blob_to_fd(1, oid, NULL, 0))
+		die("unable to stream %s to stdout", oid_to_hex(oid));
+	return 0;
+}
+
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 			int unknown_type)
 {

@@ -132,7 +139,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 		}
 
 		if (type == OBJ_BLOB)
-			return stream_blob_to_fd(1, &oid, NULL, 0);
+			return stream_blob(&oid);
 		buf = read_object_file(&oid, &type, &size);
 		if (!buf)
 			die("Cannot read object %s", obj_name);

@@ -155,7 +162,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 				oidcpy(&blob_oid, &oid);
 
 			if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB)
-				return stream_blob_to_fd(1, &blob_oid, NULL, 0);
+				return stream_blob(&blob_oid);
 			/*
 			 * we attempted to dereference a tag to a blob
 			 * and failed; there may be new dereference

@@ -319,8 +326,9 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 				BUG("invalid cmdmode: %c", opt->cmdmode);
 			batch_write(opt, contents, size);
 			free(contents);
-		} else if (stream_blob_to_fd(1, oid, NULL, 0) < 0)
-			die("unable to stream %s to stdout", oid_to_hex(oid));
+		} else {
+			stream_blob(oid);
+		}
 	}
 	else {
 		enum object_type type;

2.19.1.1235.g6b27db57c2

This goes on top Jeff's "cat-file: handle streaming failures consistently" and implements the test mode I suggested in
https://public-inbox.org/git/[email protected]/

In the process I didn't find any other bugs than the 2.12..2.19 regression which is already fixed, but as noted in 3/3 I think it's worth it to stress test fsck like this. I'll be adding this to my regular build.

Ævar Arnfjörð Bjarmason (3):
tests: add a "env-bool" helper to test-tool
tests: mark those tests where "git fsck" fails at the end
tests: add a special test setup that runs "git fsck" before exiting

 Makefile                                |  1 +
 t/README                                |  5 ++++
 t/helper/test-env-bool.c                |  9 +++++++
 t/helper/test-tool.c                    |  1 +
 t/helper/test-tool.h                    |  1 +
 t/t0000-basic.sh                        | 26 +++++++++++++++++++
 t/t1006-cat-file.sh                     |  5 ++++
 t/t1305-config-include.sh               |  4 +++
 t/t1404-update-ref-errors.sh            |  4 +++
 t/t1410-reflog.sh                       |  4 +++
 t/t1515-rev-parse-outside-repo.sh       |  4 +++
 t/t3008-ls-files-lazy-init-name-hash.sh |  4 +++
 t/t3103-ls-tree-misc.sh                 |  6 +++++
 t/t3430-rebase-merges.sh                |  6 +++++
 t/t4046-diff-unmerged.sh                |  4 +++
 t/t4058-diff-duplicates.sh              |  5 ++++
 t/t4212-log-corrupt.sh                  |  6 +++++
 t/t5000-tar-tree.sh                     |  5 ++++
 t/t5300-pack-object.sh                  |  5 ++++
 t/t5303-pack-corruption-resilience.sh   |  8 ++++++
 t/t5307-pack-missing-commit.sh          |  7 ++++++
 t/t5312-prune-corruption.sh             |  4 +++
 t/t5504-fetch-receive-strict.sh         |  4 +++
 t/t5601-clone.sh                        |  8 ++++++
 t/t6007-rev-list-cherry-pick-file.sh    |  4 +++
 t/t6011-rev-list-with-bad-commit.sh     |  7 ++++++
 t/t6030-bisect-porcelain.sh             |  6 +++++
 t/t7007-show.sh                         |  6 +++++
 t/t7106-reset-unborn-branch.sh          |  4 +++
 t/t7415-submodule-names.sh              |  4 +++
 t/t7416-submodule-dash-url.sh           |  4 +++
 t/t7417-submodule-path-url.sh           |  4 +++
 t/t7509-commit-authorship.sh            |  4 +++
 t/t8003-blame-corner-cases.sh           |  4 +++
 t/t9130-git-svn-authors-file.sh         |  7 ++++++
 t/test-lib-functions.sh                 |  2 ++
 t/test-lib.sh                           | 33 +++++++++++++++++++++++++
 37 files changed, 225 insertions(+)
 create mode 100644 t/helper/test-env-bool.c

2.19.1.899.g0250525e69

This new helper is a wrapper around the git_env_bool() function. There are various GIT_TEST_* variables described in "Running tests with special setups" in t/README that use git_env_bool().

A GIT_TEST_* variable implemented in shellscript won't have access to the same semantics (historically we've used "test -n" for many of these).

So let's add this helper so we can expose the same environment variable behavior without exposing the implementation detail of whether that variable happens to be checked in C or shellscript.

---
 Makefile                 | 1 +
 t/helper/test-env-bool.c | 9 +++++++++
 t/helper/test-tool.c     | 1 +
 t/helper/test-tool.h     | 1 +
 4 files changed, 12 insertions(+)
 create mode 100644 t/helper/test-env-bool.c

index b08d5ea258..ca624c381f 100644
@@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o
 TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
+TEST_BUILTINS_OBJS += test-env-bool.o
 TEST_BUILTINS_OBJS += test-genrandom.o
 TEST_BUILTINS_OBJS += test-hashmap.o
 TEST_BUILTINS_OBJS += test-index-version.o

new file mode 100644
index 0000000000..956b0aa88e
@@ -0,0 +1,9 @@
+#include "test-tool.h"
+#include "cache.h"
+#include "config.h"
+
+int cmd__env_bool(int argc, const char **argv)
+{
+	assert(argc == 2);
+	return !git_env_bool(argv[1], 0);
+}

index 5df8b682aa..c4481085c4 100644
@@ -17,6 +17,7 @@ static struct test_cmd cmds[] = {
 	{ "dump-fsmonitor", cmd__dump_fsmonitor },
 	{ "dump-split-index", cmd__dump_split_index },
 	{ "dump-untracked-cache", cmd__dump_untracked_cache },
+	{ "env-bool", cmd__env_bool },
 	{ "example-decorate", cmd__example_decorate },
 	{ "genrandom", cmd__genrandom },
 	{ "hashmap", cmd__hashmap },

index 71f470b871..f7845fbc56 100644
@@ -13,6 +13,7 @@ int cmd__dump_cache_tree(int argc, const char **argv);
 int cmd__dump_fsmonitor(int argc, const char **argv);
 int cmd__dump_split_index(int argc, const char **argv);
 int cmd__dump_untracked_cache(int argc, const char **argv);
+int cmd__env_bool(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);
 int cmd__hashmap(int argc, const char **argv);

2.19.1.899.g0250525e69

Mark the tests where "git fsck" fails at the end with extra test code to check the fsck output. There fsck.{err,out} has been created for us.

A later change will add the support for GIT_TEST_FSCK_TESTS. They're being added first to ensure the test suite will never fail with GIT_TEST_FSCK=true during bisect.

---
 t/t1006-cat-file.sh                     | 5 +++++
 t/t1305-config-include.sh               | 4 ++++
 t/t1404-update-ref-errors.sh            | 4 ++++
 t/t1410-reflog.sh                       | 4 ++++
 t/t1515-rev-parse-outside-repo.sh       | 4 ++++
 t/t3008-ls-files-lazy-init-name-hash.sh | 4 ++++
 t/t3103-ls-tree-misc.sh                 | 6 ++++++
 t/t3430-rebase-merges.sh                | 6 ++++++
 t/t4046-diff-unmerged.sh                | 4 ++++
 t/t4058-diff-duplicates.sh              | 5 +++++
 t/t4212-log-corrupt.sh                  | 6 ++++++
 t/t5000-tar-tree.sh                     | 5 +++++
 t/t5300-pack-object.sh                  | 5 +++++
 t/t5303-pack-corruption-resilience.sh   | 8 ++++++++
 t/t5307-pack-missing-commit.sh          | 7 +++++++
 t/t5312-prune-corruption.sh             | 4 ++++
 t/t5504-fetch-receive-strict.sh         | 4 ++++
 t/t5601-clone.sh                        | 8 ++++++++
 t/t6007-rev-list-cherry-pick-file.sh    | 4 ++++
 t/t6011-rev-list-with-bad-commit.sh     | 7 +++++++
 t/t6030-bisect-porcelain.sh             | 6 ++++++
 t/t7007-show.sh                         | 6 ++++++
 t/t7106-reset-unborn-branch.sh          | 4 ++++
 t/t7415-submodule-names.sh              | 4 ++++
 t/t7416-submodule-dash-url.sh           | 4 ++++
 t/t7417-submodule-path-url.sh           | 4 ++++
 t/t7509-commit-authorship.sh            | 4 ++++
 t/t8003-blame-corner-cases.sh           | 4 ++++
 t/t9130-git-svn-authors-file.sh         | 7 +++++++
 29 files changed, 147 insertions(+)

index 43c4be1e5e..12b69e6fbe 100755
@@ -588,4 +588,9 @@ test_expect_success 'cat-file --unordered works' '
 	test_cmp expect actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "unable to unpack header of" fsck.err &&
+	test_i18ngrep "object corrupt or missing" fsck.err
+'
+
 test_done

index 635918505d..890d307d4e 100755
@@ -313,4 +313,8 @@ test_expect_success 'include cycles are detected' '
 	test_i18ngrep "exceeded maximum include depth" stderr
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "exceeded maximum include depth" fsck.err
+'
+
 test_done

index 51a4f4c0ac..6095b2d4b9 100755
@@ -618,4 +618,8 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' '
 	test_cmp unchanged actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "invalid sha1 pointer" fsck.err
+'
+
 test_done

index 388b0611d8..43b8e0c9c5 100755
@@ -368,4 +368,8 @@ test_expect_success 'continue walking past root commits' '
 	)
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "invalid reflog entry" fsck.err
+'
+
 test_done

index 3ec2971ee5..1d8fc3ad70 100755
@@ -42,4 +42,8 @@ test_expect_success 'rev-parse --resolve-git-dir' '
 	test_cmp expect actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "not a git repository" fsck.err
+'
+
 test_done

index 64f047332b..7fb2e5c177 100755
@@ -24,4 +24,8 @@ test_expect_success 'no buffer overflow in lazy_init_name_hash' '
 	test-tool lazy-init-name-hash -m
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "notice: No default references" fsck.err
+'
+
 test_done

index 14520913af..b7d8ae2e81 100755
@@ -22,4 +22,10 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '
 	test_must_fail git ls-tree -r HEAD
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "invalid sha1 pointer in cache-tree" fsck.err &&
+	test_i18ngrep "broken link from.*tree" fsck.out &&
+	test_i18ngrep "missing tree" fsck.out
+'
+
 test_done

index aa7bfc88ec..efac3a792b 100755
@@ -396,4 +396,10 @@ test_expect_success 'with --autosquash and --exec' '
 	grep "G: +G" actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "broken link from.*commit" fsck.out &&
+	test_i18ngrep "to.*tree" fsck.out &&
+	test_i18ngrep "missing tree" fsck.out
+'
+
 test_done

index ff7cfd884a..d868bc44a9 100755
@@ -84,4 +84,8 @@ test_expect_success 'diff-files -3' '
 	test_cmp diff-files-3.expect diff-files-3.actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "notice: No default references" fsck.err
+'
+
 test_done

index c24ee175ef..9c79410dc0 100755
@@ -76,4 +76,9 @@ test_expect_success 'diff-tree with renames' '
 	test_cmp expect actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "zeroPaddedFilemode" fsck.err &&
+	test_i18ngrep "duplicateEntries" fsck.err
+'
+
 test_done

index 03b952c90d..5f36c58a61 100755
@@ -85,4 +85,10 @@ test_expect_success 'absurdly far-in-future date' '
 	git log -1 --format=%ad $commit
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "badDate" fsck.err &&
+	test_i18ngrep "badDateOverflow" fsck.err &&
+	test_i18ngrep "missingSpaceBeforeDate" fsck.err
+'
+
 test_done

index 2a97b27b0a..88c768f232 100755
@@ -408,4 +408,9 @@ test_expect_success TAR_HUGE,TIME_IS_64BIT,TIME_T_IS_64BIT 'system tar can read
 	test_cmp expect actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "corrupt loose object" fsck.err &&
+	test_i18ngrep "object corrupt or missing" fsck.err
+'
+
 test_done

index 6c620cd540..b4ef9a447a 100755
@@ -493,4 +493,9 @@ test_expect_success \
     'test_must_fail git -c core.bigfilethreshold=1 index-pack -o bad.idx test-3.pack 2>msg &&
      test_i18ngrep "SHA1 COLLISION FOUND" msg'
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "sha1 mismatch for" fsck.err &&
+	test_i18ngrep "object corrupt or missing" fsck.err
+'
+
 test_done

index 41e6dc4dcf..79c9b307d0 100755
@@ -400,4 +400,12 @@ test_expect_success \
     'printf "\0\1\1X\0" > tail_garbage_opcode &&
      test_must_fail test-tool delta -p /dev/null tail_garbage_opcode /dev/null'
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "pack checksum mismatch" fsck.err &&
+	test_i18ngrep "index CRC mismatch.*at offset 12" fsck.err &&
+	test_i18ngrep "cannot unpack.*at offset 12" fsck.err &&
+	test_i18ngrep "failed to read delta base object.*at offset 12" fsck.err &&
+	test_i18ngrep "failed to read delta base object.*at offset 2032" fsck.err
+'
+
 test_done

index dacb440b27..2780f4ceeb 100755
@@ -36,4 +36,11 @@ test_expect_success 'pack-objects notices corruption' '
 	test_must_fail git pack-objects --revs pack
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "invalid sha1 pointer" fsck.err &&
+	test_i18ngrep "broken link from.*commit" fsck.out &&
+	test_i18ngrep "to.*commit" fsck.out &&
+	test_i18ngrep "missing commit" fsck.out
+'
+
 test_done

index da9d59940d..898d8906bc 100755
@@ -111,4 +111,8 @@ test_expect_success 'pack-refs does not drop broken refs during deletion' '
 	test_cmp expect actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "invalid sha1 pointer" fsck.err
+'
+
 test_done

index 7bc706873c..945a060992 100755
@@ -348,4 +348,8 @@ test_expect_success \
 	grep "Cannot demote unterminatedheader" act
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "missingEmail" fsck.err
+'
+
 test_done

index f1a49e94f5..35eca5881b 100755
@@ -726,6 +726,14 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe
 	git clone --filter=blob:limit=0 "file://$(pwd)/server" client
 '
 
+# We might have "test_done" through lib-httpd.sh. Need to tes
+# GIT_TEST_FSCK_TESTS here.
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "object.*is a tree, not a blob" fsck.err &&
+	test_i18ngrep "object.*is a commit, not a blob" fsck.err &&
+	test_i18ngrep "error in tree.*: broken links" fsck.err
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

index f0268372d2..a86ba0900b 100755
@@ -266,4 +266,8 @@ test_expect_success '--cherry-pick avoids looking at full diffs' '
 	git rev-list --cherry-pick ...shy-diff
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "missing blob" fsck.out
+'
+
 test_done

index 545b461e51..30d39ce925 100755
@@ -55,5 +55,12 @@ test_expect_success 'first commit is still available' \
    git log $first_commit
    '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "pack checksum mismatch" fsck.err &&
+	test_i18ngrep "index CRC mismatch for object.*at offset 487" fsck.err &&
+	test_i18ngrep "inflate: data stream error.*incorrect data check" fsck.err &&
+	test_i18ngrep "cannot unpack.*at offset 487" fsck.err
+'
+
 test_done
 

index f84ff941c3..5da668ed06 100755
@@ -911,4 +911,10 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
 	test_path_is_missing "$GIT_DIR/BISECT_START"
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "broken link from.*tree" fsck.out &&
+	test_i18ngrep "to.*tree" fsck.out &&
+	test_i18ngrep "missing tree" fsck.out
+'
+
 test_done

index 42d3db6246..d25cee8a72 100755
@@ -128,4 +128,10 @@ test_expect_success 'show --graph is forbidden' '
   test_must_fail git show --graph HEAD
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "broken link from.*tag" fsck.out &&
+	test_i18ngrep "to.*blob" fsck.out &&
+	test_i18ngrep "missing blob" fsck.out
+'
+
 test_done

index ecb85c3b82..0719261fe7 100755
@@ -64,4 +64,8 @@ test_expect_success 'reset --hard' '
 	test_path_is_missing a
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "missing tree" fsck.out
+'
+
 test_done

index 293e2e1963..3d8fa7831f 100755
@@ -191,4 +191,8 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
 	)
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "gitmodulesName" fsck.err
+'
+
 test_done

index 1cd2c1c1ea..e6c885784e 100755
@@ -46,4 +46,8 @@ test_expect_success 'fsck rejects unprotected dash' '
 	grep gitmodulesUrl err
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "gitmodulesUrl" fsck.err
+'
+
 test_done

index 756af8c4d6..8362442908 100755
@@ -25,4 +25,8 @@ test_expect_success 'fsck rejects unprotected dash' '
 	grep gitmodulesPath err
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "gitmodulesPath" fsck.err
+'
+
 test_done

index 500ab2fe72..cdcbfed61a 100755
@@ -174,4 +174,8 @@ test_expect_success '--reset-author with CHERRY_PICK_HEAD' '
 	test_cmp expect actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "invalid reflog entry" fsck.err
+'
+
 test_done

index c92a47b6d5..3a8affc3e7 100755
@@ -275,4 +275,8 @@ test_expect_success 'blame file with CRLF core.autocrlf=true' '
 	grep "A U Thor" actual
 '
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "missingNameBeforeEmail" fsck.err
+'
+
 test_done

index cb764bcadc..0c0c42c72c 100755
@@ -128,4 +128,11 @@ test_expect_success 'authors-file imported user without email' '
 
 test_debug 'GIT_DIR=gitconfig.clone/.git git log'
 
+GIT_TEST_FSCK_TESTS='
+	test_i18ngrep "object.*is a tree, not a blob" fsck.err &&
+	test_i18ngrep "object.*is a commit, not a blob" fsck.err &&
+	test_i18ngrep "tree.*: broken links" fsck.err &&
+	test_i18ngrep "missingTaggerEntry" fsck.err
+'
+
 test_done

2.19.1.899.g0250525e69

I am sympathetic to what step 3/3 (eh, rather, an earlier "let's not leave the repository in corrupt state, as that would make it inconvenient for us to later append new tests") wants to do, but not this one---these markings at the end makes it inconvenient for us to later add new tests to these script before them.

Add the ability to run the tests with GIT_TEST_FSCK=true in the environment. If set we'll run "git fsck" at the end of every test, and those tests that fail need to annotate what their failure was.

The goal is to detect regressions in fsck that our tests might otherwise miss. We had one such regression in c68b489e56 ("fsck: parse loose object paths directly", 2017-01-13) released with Git 2.12.0, which wasn't spotted more than a year and a half later during the 2.20.0 window.

As it turns out there already was a test for what triggerd that bug all along in the form of t5000-tar-tree.sh, we just weren't running "git fsck" at the end[1].

That specific bug has been fixed in ("check_stream_sha1(): handle input underflow", 2018-10-30)[1], but since we have a demonstrable history of not anticipating which tests which would make "git fsck" fail need to be made part of the "git fsck" test suite let's add this test mode to cover potential blind spots. The "git fsck" command is also something where we might expect that during our RC windows users aren't actively testing on already corrupt repositories, so "in the wild" test coverage will be spotty, so we need all the help we can get.

1. https://public-inbox.org/git/[email protected]/
2. https://public-inbox.org/git/[email protected]/

---
 t/README                |  5 +++++
 t/t0000-basic.sh        | 26 ++++++++++++++++++++++++++
 t/test-lib-functions.sh |  2 ++
 t/test-lib.sh           | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+)

index 8847489640..092f78b3d7 100644
@@ -343,6 +343,11 @@ of the index for the whole test suite by bypassing the default number of
 cache entries and thread minimums. Setting this to 1 will make the
 index loading single threaded.
 
+GIT_TEST_FSCK=<boolean> if true arranges for "git fsck" to be run at
+the end of the test scripts. Those tests that fail will need to set a
+"GIT_TEST_FSCK_TESTS" variable before we enter "test_done" with a test
+fragment to test that fsck.{out,err} is the expected failure.
+
 Naming Tests
 ------------
 

index 4d23373526..8e667e6691 100755
@@ -19,6 +19,7 @@ modification *should* take notice and update the test vectors here.
 '
 
 . ./test-lib.sh
+unset GIT_TEST_FSCK
 
 try_local_x () {
 	local x="local" &&

@@ -393,6 +394,31 @@ test_expect_success 'GIT_SKIP_TESTS sh pattern' "
 	)
 "
 
+test_expect_success 'GIT_TEST_FSCK=true' "
+	test_when_finished 'sane_unset GIT_TEST_FSCK' &&
+	GIT_TEST_FSCK=true &&
+	export GIT_TEST_FSCK &&
+	run_sub_test_lib_test run-git-fsck-test \
+		'--run basic' --run='1 3 5' <<-\\EOF &&
+	for i in 1 2 3 4 5 6
+	do
+		test_expect_success \"passing test #\$i\" 'true'
+	done
+	GIT_TEST_FSCK=true test_done
+	EOF
+	check_sub_test_lib_test run-git-fsck-test <<-\\EOF
+	> ok 1 - passing test #1
+	> ok 2 # skip passing test #2 (--run)
+	> ok 3 - passing test #3
+	> ok 4 # skip passing test #4 (--run)
+	> ok 5 - passing test #5
+	> ok 6 # skip passing test #6 (--run)
+	> ok 7 # skip git fsck at end (due to GIT_TEST_FSCK) (expected to succeed) (--run)
+	> # passed all 7 test(s)
+	> 1..7
+	EOF
+"
+
 test_expect_success '--run basic' "
 	run_sub_test_lib_test run-basic \
 		'--run basic' --run='1 3 5' <<-\\EOF &&

index 78d8c3783b..7d002ff5aa 100644
@@ -470,6 +470,7 @@ test_expect_success () {
 # Usage: test_external description command arguments...
 # Example: test_external 'Perl API' perl ../path/to/test.pl
 test_external () {
+	unset GIT_TEST_FSCK
 	test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
 	test "$#" = 3 ||
 	error >&5 "bug in the test script: not 3 or 4 parameters to test_external"

@@ -511,6 +512,7 @@ test_external () {
 # Like test_external, but in addition tests that the command generated
 # no output on stderr.
 test_external_without_stderr () {
+	unset GIT_TEST_FSCK
 	# The temporary file has no (and must have no) security
 	# implications.
 	tmp=${TMPDIR:-/tmp}

index 897e6fcc94..5f7f5595e3 100644
@@ -454,6 +454,8 @@ GIT_EXIT_OK=
 trap 'die' EXIT
 trap 'exit $?' INT
 
+GIT_TEST_FSCK_TESTS=
+
 # The user-facing functions are loaded from a separate file so that
 # test_perf subshells can have them too
 . "$TEST_DIRECTORY/test-lib-functions.sh"

@@ -789,7 +791,36 @@ test_at_end_hook_ () {
 	:
 }
 
+_test_done_fsck() {
+	desc='git fsck at end (due to GIT_TEST_FSCK)'
+	if test -n "$GIT_TEST_FSCK_TESTS"
+	then
+		test_expect_success "$desc (expected to fail)" '
+			test_must_fail git fsck 2>fsck.err >fsck.out
+		'
+		test_expect_success "$desc (expected to fail) -- assert failure mode" "
+			test_path_exists fsck.err &&
+			test_path_exists fsck.out &&
+			$GIT_TEST_FSCK_TESTS
+		"
+	else
+		test_expect_success "$desc (expected to succeed)" '
+			git fsck
+		'
+	fi
+}
+
 test_done () {
+	# Don't want to run this under TEST_NO_CREATE_REPO, otherwise
+	# we end up sloowly running "git fsck" against git.git
+	if test -z "$TEST_NO_CREATE_REPO" &&
+		    # test -n first so all --verbose output isn't
+		    # polluted with this check
+		    test -n "$GIT_TEST_FSCK" &&
+		    test_have_prereq TEST_FSCK
+	then
+		_test_done_fsck
+	fi
 	GIT_EXIT_OK=t
 
 	if test -z "$HARNESS_ACTIVE"

@@ -1268,3 +1299,5 @@ test_lazy_prereq CURL '
 test_lazy_prereq SHA1 '
 	test $(git hash-object /dev/null) = e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
 '
+
+test_lazy_prereq TEST_FSCK 'test-tool env-bool GIT_TEST_FSCK'

2.19.1.899.g0250525e69

I think that name makes sense, even though stream_blob() is just fine for a fuction that takes a single parameter oid, as there is no other sane choice than streaming to the standard output stream the blob data.

What would benefit from symbolic constant more in that function call may be CAN_SEEK thing, but s/1/STDOUT_FILENO/ adds negative value to that line, I would think. The name of the function already makes it clear this is sending the output to a file descriptor, and an integer 1 that specifies a file descriptor cannot mean anything other than the standard output stream.

I was trying to keep the name small since it is a static-local convenience helper. I'd rather write it as:

  stream_blob(1, oid);

than change the name. ;)

Yes, I'd agree (there are very few cases where I think STDOUT_FILENO actually increases the readability, since it is usually pretty clear from the context when something is a descriptor).

-Peff

Your "m" got confused and ended up upside-down.

Heh. I'm not sure how I managed that. They're not exactly next to each other on a qwerty keyboard.

-Peff

WIP patch for doing that:

    diff --git a/Makefile b/Makefile
    index b08d5ea258..ca624c381f 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o
     TEST_BUILTINS_OBJS += test-dump-split-index.o
     TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
     TEST_BUILTINS_OBJS += test-example-decorate.o
    +TEST_BUILTINS_OBJS += test-env-bool.o
     TEST_BUILTINS_OBJS += test-genrandom.o
     TEST_BUILTINS_OBJS += test-hashmap.o
     TEST_BUILTINS_OBJS += test-index-version.o
    diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
    index 5df8b682aa..c4481085c4 100644
    --- a/t/helper/test-tool.c
    +++ b/t/helper/test-tool.c
    @@ -17,6 +17,7 @@ static struct test_cmd cmds[] = {
     	{ "dump-fsmonitor", cmd__dump_fsmonitor },
     	{ "dump-split-index", cmd__dump_split_index },
     	{ "dump-untracked-cache", cmd__dump_untracked_cache },
    +	{ "env-bool", cmd__env_bool },
     	{ "example-decorate", cmd__example_decorate },
     	{ "genrandom", cmd__genrandom },
     	{ "hashmap", cmd__hashmap },
    diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
    index 71f470b871..f7845fbc56 100644
    --- a/t/helper/test-tool.h
    +++ b/t/helper/test-tool.h
    @@ -13,6 +13,7 @@ int cmd__dump_cache_tree(int argc, const char **argv);
     int cmd__dump_fsmonitor(int argc, const char **argv);
     int cmd__dump_split_index(int argc, const char **argv);
     int cmd__dump_untracked_cache(int argc, const char **argv);
    +int cmd__env_bool(int argc, const char **argv);
     int cmd__example_decorate(int argc, const char **argv);
     int cmd__genrandom(int argc, const char **argv);
     int cmd__hashmap(int argc, const char **argv);
    diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
    index 635918505d..92fbce2920 100755
    --- a/t/t1305-config-include.sh
    +++ b/t/t1305-config-include.sh
    @@ -313,4 +313,8 @@ test_expect_success 'include cycles are detected' '
     	test_i18ngrep "exceeded maximum include depth" stderr
     '

    +GIT_FSCK_FAILS=true
    +GIT_FSCK_FAILS_TEST='
    +	test_i18ngrep "exceeded maximum include depth" fsck.err
    +'
     test_done
    diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh
    index 14520913af..06abf84ef4 100755
    --- a/t/t3103-ls-tree-misc.sh
    +++ b/t/t3103-ls-tree-misc.sh
    @@ -22,4 +22,10 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '
     	test_must_fail git ls-tree -r HEAD
     '

    +GIT_FSCK_FAILS=true
    +GIT_FSCK_FAILS_TEST='
    +	test_i18ngrep "invalid sha1 pointer in cache-tree" fsck.err &&
    +	test_i18ngrep "broken link from" fsck.out &&
    +	test_i18ngrep "missing tree" fsck.out
    +'
     test_done
    diff --git a/t/test-lib.sh b/t/test-lib.sh
    index 897e6fcc94..d4ebb94998 100644
    --- a/t/test-lib.sh
    +++ b/t/test-lib.sh
    @@ -454,6 +454,8 @@ GIT_EXIT_OK=
     trap 'die' EXIT
     trap 'exit $?' INT

    +GIT_FSCK_FAILS=
    +
     # The user-facing functions are loaded from a separate file so that
     # test_perf subshells can have them too
     . "$TEST_DIRECTORY/test-lib-functions.sh"
    @@ -790,6 +792,25 @@ test_at_end_hook_ () {
     }

     test_done () {
    +	if test_have_prereq TEST_FSCK
    +	then
    +		desc='git fsck at end (due to GIT_TEST_FSCK)'
    +		if test -n "$GIT_FSCK_FAILS"
    +		then
    +			test_expect_success "$desc (expected to fail)" '
    +				test_must_fail git fsck 2>fsck.err >fsck.out
    +			'
    +			test_expect_success "$descriptor (expected to fail) -- assert failure mode" "
    +				test_path_exists fsck.err &&
    +				test_path_exists fsck.out &&
    +				$GIT_FSCK_FAILS_TEST
    +			"
    +		else
    +			test_expect_success "$desc" '
    +				git fsck
    +			'
    +		fi
    +	fi
     	GIT_EXIT_OK=t

     	if test -z "$HARNESS_ACTIVE"
    @@ -1268,3 +1289,5 @@ test_lazy_prereq CURL '
     test_lazy_prereq SHA1 '
     	test $(git hash-object /dev/null) = e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
     '
    +
    +test_lazy_prereq TEST_FSCK 'test-tool env-bool GIT_TEST_FSCK'

Could be made prettier by turning that work in test_done() into a utility function, but is (I think) worth the effort to do.

Jeff: Gotta turn in for the night, but maybe Something you're maybe interested in carrying forward for this fix? It's not that much work to mark up the failing tests, there's 10-20 of them from some quick eyeballing.

For this fix, I'd much rather add a specific test to the existing fsck tests. Otherwise, we're relying on what a bunch of other tests happen to be doing now, but there's little hope that they won't get refactored in a way that puts a gap in our test coverage.

IOW, I think of things like GIT_TEST_FSCK as a kind of shotgun approach. They may find things, and we should fix them and make sure it runs clean. But ultimately, specific cases of interest should get their own tests.

-Peff

When a loose tree or commit is read by fsck (or any git program), unpack_sha1_rest() checks whether there is extra cruft at the end of the object file, after the zlib data. Blobs that are streamed, however, do not have this check.

For normal git operations, it's not a big deal. We know the sha1 and size checked out, so we have the object bytes we wanted. The trailing garbage doesn't affect what we're trying to do.

But since the point of fsck is to find corruption or other problems, it should be more thorough. This patch teaches its loose-sha1 reader to detect extra bytes after the zlib stream and complain.

---
 sha1_file.c     |  5 +++++
 t/t1450-fsck.sh | 22 ++++++++++++++++++++++
 2 files changed, 27 insertions(+)

index c0fccb73c..b77ab6d5c 100644
@@ -3864,6 +3864,11 @@ static int check_stream_sha1(git_zstream *stream,
 		error("corrupt loose object '%s'", sha1_to_hex(expected_sha1));
 		return -1;
 	}
+	if (stream->avail_in) {
+		error("garbage at end of loose object '%s'",
+		      sha1_to_hex(expected_sha1));
+		return -1;
+	}
 
 	git_SHA1_Final(real_sha1, &c);
 	if (hashcmp(expected_sha1, real_sha1)) {

index 455c186fe..8975b4d1b 100755
@@ -597,4 +597,26 @@ test_expect_success 'fsck finds problems in duplicate loose objects' '
 	)
 '
 
+test_expect_success 'fsck detects trailing loose garbage (commit)' '
+	git cat-file commit HEAD >basis &&
+	echo bump-commit-sha1 >>basis &&
+	commit=$(git hash-object -w -t commit basis) &&
+	file=$(sha1_file $commit) &&
+	test_when_finished "remove_object $commit" &&
+	chmod +w "$file" &&
+	echo garbage >>"$file" &&
+	test_must_fail git fsck 2>out &&
+	test_i18ngrep "garbage.*$commit" out
+'
+
+test_expect_success 'fsck detects trailing loose garbage (blob)' '
+	blob=$(echo trailing | git hash-object -w --stdin) &&
+	file=$(sha1_file $blob) &&
+	test_when_finished "remove_object $blob" &&
+	chmod +w "$file" &&
+	echo garbage >>"$file" &&
+	test_must_fail git fsck 2>out &&
+	test_i18ngrep "garbage.*$blob" out
+'
+
 test_done

2.11.0.629.g10075098c

Yeah, I discovered the same thing when I took a look at it a week or so ago. :-(

I don't know if my opinion counts for much, but the changes look good to me.

-John

Also very good information. Thanks Dennis!

-John