Re: Infinite loop regression in git-fsck in v2.12.0
On Tue, Oct 30, 2018 at 06:56:03PM -0400, Jeff King wrote:
> > > while (total_read <= size &&
> > > +stream->avail_in > 0 &&
> > > (status == Z_OK || status == Z_BUF_ERROR)) {
> > > stream->next_out = buf;
> > > stream->avail_out = sizeof(buf);
> >
> > Hmph. If the last round consumed the final input byte and needed
> > output space of N bytes, but only M (< N) bytes of the output space
> > was available, then it would have reduced both avail_in and
> > avail_out down to zero and yielded Z_BUF_ERROR, no? Or would zlib
> > refrain from consuming that final byte (leaving avail_in to at least
> > one) and give us Z_BUF_ERROR in such a case?
>
> 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.
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
Re: Infinite loop regression in git-fsck in v2.12.0
On Tue, Oct 30, 2018 at 10:56:22PM +0100, Ævar Arnfjörð Bjarmason wrote: > > 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. > [...] > 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
Re: Infinite loop regression in git-fsck in v2.12.0
On Wed, Oct 31, 2018 at 07:28:00AM +0900, Junio C Hamano wrote:
> > So we need to distinguish those cases. I think this is the simplest fix:
> >
> > diff --git a/sha1-file.c b/sha1-file.c
> > index dd0b6aa873..a7ff5fe25d 100644
> > --- a/sha1-file.c
> > +++ b/sha1-file.c
> > @@ -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);
>
> Hmph. If the last round consumed the final input byte and needed
> output space of N bytes, but only M (< N) bytes of the output space
> was available, then it would have reduced both avail_in and
> avail_out down to zero and yielded Z_BUF_ERROR, no? Or would zlib
> refrain from consuming that final byte (leaving avail_in to at least
> one) and give us Z_BUF_ERROR in such a case?
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
Re: Infinite loop regression in git-fsck in v2.12.0
Jeff King writes:
> 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:
>
> diff --git a/sha1-file.c b/sha1-file.c
> index dd0b6aa873..a7ff5fe25d 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -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);
Hmph. If the last round consumed the final input byte and needed
output space of N bytes, but only M (< N) bytes of the output space
was available, then it would have reduced both avail_in and
avail_out down to zero and yielded Z_BUF_ERROR, no? Or would zlib
refrain from consuming that final byte (leaving avail_in to at least
one) and give us Z_BUF_ERROR in such a case?
> 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.
Yes, exactly.
> 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.
Thanks.
Re: Infinite loop regression in git-fsck in v2.12.0
On Tue, Oct 30 2018, Ævar Arnfjörð Bjarmason wrote:
> 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-ava...@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.
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
Re: Infinite loop regression in git-fsck in v2.12.0
On Tue, Oct 30, 2018 at 09:03:24PM +0100, Ævar Arnfjörð Bjarmason wrote:
> While playing around with having a GIT_TEST_FSCK=true as I suggested in
> https://public-inbox.org/git/20181030184331.27264-3-ava...@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
> )
Thanks, I was easily able to reproduce.
> Before we'd print:
>
> error: sha1 mismatch 19f9c8273ec45a8938e6999cb59b3ff66739902a
> error: 19f9c8273ec45a8938e6999cb59b3ff66739902a: object corrupt or missing
> Checking object directories: 100% (256/256), done.
> missing blob 19f9c8273ec45a8938e6999cb59b3ff66739902a
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:
diff --git a/sha1-file.c b/sha1-file.c
index dd0b6aa873..a7ff5fe25d 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -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);
> 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();
> @@ -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 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
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-ava...@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();
@@ -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(, 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-ava...@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.
