Infinite loop regression in git-fsck in v2.12.0

While playing around with having a GIT_TEST_FSCK=true as I suggested in https://public-inbox.org/git/20181030184331.27264-3-avarab@gmail.com/ 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 <20181030184331.27264-3-avarab@gmail.com> 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.