{"thread":{"id":"44893","subject":"[PATCH 0/6] fsck --connectivity-check misses some corruption","startedAt":"2017-01-16T21:22:41Z","lastAt":"2017-01-17T21:34:04Z","messageCount":15,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"309491","messageId":"20170116212231.ojoqzlajpszifaf3@sigill.intra.peff.net","threadId":"44893","inReplyTo":null,"subject":"[PATCH 0/6] fsck --connectivity-check misses some corruption","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-16T21:22:31Z","receivedAt":"2017-01-16T21:22:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I came across a repository today that was missing an object, and for\nwhich \"git fsck\" reported the error but \"git fsck --connectivity-check\"\ndid not. It turns out that the shortcut taken by --connectivity-check\nviolates some assumptions made by the rest of fsck (namely, that every\nobject in the repo has a \"struct object\" loaded).\n\nAnd fsck being a generally neglected tool, I couldn't help but find\nseveral more bugs on the way. :)\n\n [1/6]: t1450: clean up sub-objects in duplicate-entry test\n [2/6]: fsck: report trees as dangling\n [3/6]: fsck: prepare dummy objects for --connectivity-check\n [4/6]: fsck: tighten error-checks of \"git fsck \"\n [5/6]: fsck: do not fallback \"git fsck \" to \"git fsck\"\n [6/6]: fsck: check HAS_OBJ more consistently\n\n builtin/fsck.c | 131 ++++++++++++++++++++++++++++++++++++++++++++------------\n t/t1450-fsck.sh | 70 ++++++++++++++++++++++++++++--\n 2 files changed, 171 insertions(+), 30 deletions(-)\n\n-Peff\n"},{"id":"309492","messageId":"20170116212403.l7ca7crmt47id3mu@sigill.intra.peff.net","threadId":"44893","inReplyTo":"20170116212231.ojoqzlajpszifaf3@sigill.intra.peff.net","subject":"[PATCH 1/6] t1450: clean up sub-objects in duplicate-entry test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-16T21:24:03Z","receivedAt":"2017-01-16T21:24:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This test creates a multi-level set of trees, but its\ncleanup routine only removes the top-level tree. After the\ntest finishes, the inner tree and the blob it points to\nremain, making the inner tree dangling.\n\nA later test (\"cleaned up\") verifies that we've removed any\ncruft and \"git fsck\" output is clean. This passes only\nbecause of a bug in git-fsck which fails to notice dangling\ntrees.\n\nIn preparation for fixing the bug, let's teach this earlier\ntest to clean up after itself correctly. We have to remove\nthe inner tree (and therefore the blob, too, which becomes\ndangling after removing that tree).\n\nSince the setup code happens inside a subshell, we can't\njust set a variable for each object. However, we can stuff\nall of the sha1s into the $T output variable, which is not\nused for anything except cleanup.\n\nSigned-off-by: Jeff King \n---\n t/t1450-fsck.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex ee7d4736d..6eef8b28e 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -189,14 +189,16 @@ test_expect_success 'commit with NUL in header' '\n '\n \n test_expect_success 'tree object with duplicate entries' '\n-\ttest_when_finished \"remove_object \\$T\" &&\n+\ttest_when_finished \"for i in \\$T; do remove_object \\$i; done\" &&\n \tT=$(\n \t\tGIT_INDEX_FILE=test-index &&\n \t\texport GIT_INDEX_FILE &&\n \t\trm -f test-index &&\n \t\t>x &&\n \t\tgit add x &&\n+\t\tgit rev-parse :x &&\n \t\tT=$(git write-tree) &&\n+\t\techo $T &&\n \t\t(\n \t\t\tgit cat-file tree $T &&\n \t\t\tgit cat-file tree $T\n-- \n2.11.0.642.gd6f8cda6c\n\n"},{"id":"309493","messageId":"20170116212535.cohvikwkju5zehr4@sigill.intra.peff.net","threadId":"44893","inReplyTo":"20170116212231.ojoqzlajpszifaf3@sigill.intra.peff.net","subject":"[PATCH 2/6] fsck: report trees as dangling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-16T21:25:35Z","receivedAt":"2017-01-16T21:25:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After checking connectivity, fsck looks through the list of\nany objects we've seen mentioned, and reports unreachable\nand un-\"used\" ones as dangling. However, it skips any object\nwhich is not marked as \"parsed\", as that is an object that\nwe _don't_ have (but that somebody mentioned).\n\nSince 6e454b9a3 (clear parsed flag when we free tree\nbuffers, 2013-06-05), that flag can't be relied on, and the\ncorrect method is to check the HAS_OBJ flag. The cleanup in\nthat commit missed this callsite, though. As a result, we\nwould generally fail to report dangling trees.\n\nWe never noticed because there were no tests in this area\n(for trees or otherwise). Let's add some.\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 2 +-\n t/t1450-fsck.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex f01b81eeb..3e67203f9 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -225,7 +225,7 @@ static void check_unreachable_object(struct object *obj)\n \t * to complain about it being unreachable (since it does\n \t * not exist).\n \t */\n-\tif (!obj->parsed)\n+\tif (!(obj->flags & HAS_OBJ))\n \t\treturn;\n \n \t/*\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 6eef8b28e..e88ec7747 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -559,4 +559,31 @@ test_expect_success 'fsck --name-objects' '\n \t)\n '\n \n+# for each of type, we have one version which is referenced by another object\n+# (and so while unreachable, not dangling), and another variant which really is\n+# dangling.\n+test_expect_success 'fsck notices dangling objects' '\n+\tgit init dangling &&\n+\t(\n+\t\tcd dangling &&\n+\t\tblob=$(echo not-dangling | git hash-object -w --stdin) &&\n+\t\tdblob=$(echo dangling | git hash-object -w --stdin) &&\n+\t\ttree=$(printf \"100644 blob %s\\t%s\\n\" $blob one | git mktree) &&\n+\t\tdtree=$(printf \"100644 blob %s\\t%s\\n\" $blob two | git mktree) &&\n+\t\tcommit=$(git commit-tree $tree) &&\n+\t\tdcommit=$(git commit-tree -p $commit $tree) &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\tdangling blob $dblob\n+\t\tdangling commit $dcommit\n+\t\tdangling tree $dtree\n+\t\tEOF\n+\n+\t\tgit fsck >actual &&\n+\t\t# the output order is non-deterministic, as it comes from a hash\n+\t\tsort actual.sorted &&\n+\t\ttest_cmp expect actual.sorted\n+\t)\n+'\n+\n test_done\n-- \n2.11.0.642.gd6f8cda6c\n\n"},{"id":"309494","messageId":"20170116213420.d3e6ziu4gt3app4x@sigill.intra.peff.net","threadId":"44893","inReplyTo":"20170116212231.ojoqzlajpszifaf3@sigill.intra.peff.net","subject":"[PATCH 5/6] fsck: do not fallback \"git fsck \" to \"git fsck\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-16T21:34:21Z","receivedAt":"2017-01-16T21:34:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since fsck tries to continue as much as it can after seeing\nan error, we still do the reachability check even if some\nheads we were given on the command-line are bogus. But if\n_none_ of the heads is is valid, we fallback to checking all\nrefs and the index, which is not what the user asked for at\nall.\n\nInstead of checking \"heads\", the number of successful heads\nwe got, check \"argc\" (which we know only has non-options in\nit, because parse_options removed the others).\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 2 +-\n t/t1450-fsck.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex c7d0590e5..8ae065b2d 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -778,7 +778,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t * default ones from .git/refs. We also consider the index file\n \t * in this case (ie this implies --cache).\n \t */\n-\tif (!heads) {\n+\tif (!argc) {\n \t\tget_default_heads();\n \t\tkeep_cache_objects = 1;\n \t}\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 2f3b05276..96b74dc9a 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -610,4 +610,15 @@ test_expect_success 'fsck $name notices bogus $name' '\n \ttest_must_fail git fsck $_z40\n '\n \n+test_expect_success 'bogus head does not fallback to all heads' '\n+\t# set up a case that will cause a reachability complaint\n+\techo to-be-deleted >foo &&\n+\tgit add foo &&\n+\tblob=$(git rev-parse :foo) &&\n+\ttest_when_finished \"git rm --cached foo\" &&\n+\tremove_object $blob &&\n+\ttest_must_fail git fsck $_z40 >out 2>&1 &&\n+\t! grep $blob out\n+'\n+\n test_done\n-- \n2.11.0.642.gd6f8cda6c\n\n"},{"id":"309495","messageId":"20170116213457.n7twsvr2vozdpgcj@sigill.intra.peff.net","threadId":"44893","inReplyTo":"20170116212231.ojoqzlajpszifaf3@sigill.intra.peff.net","subject":"[PATCH 6/6] fsck: check HAS_OBJ more consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-16T21:34:57Z","receivedAt":"2017-01-16T21:35:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are two spots that call lookup_object() and assume\nthat a non-NULL result means we have the object:\n\n 1. When we're checking the objects given to us by the user\n on the command line.\n\n 2. When we're checking if a reflog entry is valid.\n\nThis generally follows fsck's mental model that we will have\nlooked at and loaded a \"struct object\" for each object in\nthe repository. But it misses one case: if another object\n_mentioned_ an object, but we didn't actually parse it or\nverify that it exists, it will still have a struct.\n\nIt's not clear if this is a triggerable bug or not.\nCertainly the later parts of the reachability check need to\nbe careful of this, and do so by checking the HAS_OBJ flag.\nBut both of these steps happen before we start traversing,\nso probably we won't have followed any links yet. Still,\nit's easy enough to be defensive here.\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 8ae065b2d..28d3cbb14 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -398,7 +398,7 @@ static void fsck_handle_reflog_sha1(const char *refname, unsigned char *sha1,\n \n \tif (!is_null_sha1(sha1)) {\n \t\tobj = lookup_object(sha1);\n-\t\tif (obj) {\n+\t\tif (obj && (obj->flags & HAS_OBJ)) {\n \t\t\tif (timestamp && name_objects)\n \t\t\t\tadd_decoration(fsck_walk_options.object_names,\n \t\t\t\t\tobj,\n@@ -755,7 +755,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tif (!get_sha1(arg, sha1)) {\n \t\t\tstruct object *obj = lookup_object(sha1);\n \n-\t\t\tif (!obj) {\n+\t\t\tif (!obj || !(obj->flags & HAS_OBJ)) {\n \t\t\t\terror(\"%s: object missing\", sha1_to_hex(sha1));\n \t\t\t\terrors_found |= ERROR_OBJECT;\n \t\t\t\tcontinue;\n-- \n2.11.0.642.gd6f8cda6c\n"},{"id":"309496","messageId":"20170116213204.e7ykwowqzafkexqd@sigill.intra.peff.net","threadId":"44893","inReplyTo":"20170116212231.ojoqzlajpszifaf3@sigill.intra.peff.net","subject":"[PATCH 3/6] fsck: prepare dummy objects for --connectivity-check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-16T21:32:04Z","receivedAt":"2017-01-16T21:39:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Normally fsck makes a pass over all objects to check their\nintegrity, and then follows up with a reachability check to\nmake sure we have all of the referenced objects (and to know\nwhich ones are dangling). The latter checks for the HAS_OBJ\nflag in obj->flags to see if we found the object in the\nfirst pass.\n\nCommit 02976bf85 (fsck: introduce `git fsck --connectivity-only`,\n2015-06-22) taught fsck to skip the initial pass, and to\nfallback to has_sha1_file() instead of the HAS_OBJ check.\n\nHowever, it converted only one HAS_OBJ check to use\nhas_sha1_file(). But there are many other places in\nbuiltin/fsck.c that assume that the flag is set (or that\nlookup_object() will return an object at all). This leads to\nseveral bugs with --connectivity-only:\n\n 1. mark_object() will not queue objects for examination,\n so recursively following links from commits to trees,\n etc, did nothing. I.e., we were checking the\n reachability of hardly anything at all.\n\n 2. When a set of heads is given on the command-line, we\n use lookup_object() to see if they exist. But without\n the initial pass, we assume nothing exists.\n\n 3. When loading reflog entries, we do a similar\n lookup_object() check, and complain that the reflog is\n broken if the object doesn't exist in our hash.\n\nSo in short, --connectivity-only is broken pretty badly, and\nwill claim that your repository is fine when it's not.\nPresumably nobody noticed for a few reasons.\n\nOne is that the embedded test does not actually test the\nrecursive nature of the reachability check. All of the\nmissing objects are still in the index, and we directly\ncheck items from the index. This patch modifies the test to\ndelete the index, which shows off breakage (1).\n\nAnother is that --connectivity-only just skips the initial\npass for loose objects. So on a real repository, the packed\nobjects were still checked correctly. But on the flipside,\nit means that \"git fsck --connectivity-only\" still checks\nthe sha1 of all of the packed objects, nullifying its\noriginal purpose of being a faster git-fsck.\n\nAnd of course the final problem is that the bug only shows\nup when there _is_ corruption, which is rare. So anybody\nrunning \"git fsck --connectivity-only\" proactively would\nassume it was being thorough, when it was not.\n\nOne possibility for fixing this is to find all of the spots\nthat rely on HAS_OBJ and tweak them for the connectivity-only\ncase. But besides the risk that we might miss a spot (and I\nfound three already, corresponding to the three bugs above),\nthere are other parts of fsck that _can't_ work without a\nfull list of objects. E.g., the list of dangling objects.\n\nInstead, let's make the connectivity-only case look more\nlike the normal case. Rather than skip the initial pass\ncompletely, we'll do an abbreviated one that sets up the\nHAS_OBJ flag for each object, without actually loading the\nobject data.\n\nThat's simple and fast, and we don't have to care about the\nconnectivity_only flag in the rest of the code at all.\nWhile we're at it, let's make sure we treat loose and packed\nobjects the same (i.e., setting up dummy objects for both\nand skipping the actual sha1 check). That makes the\nconnectivity-only check actually fast on a real repo (40\nseconds versus 180 seconds on my copy of linux.git).\n\nSigned-off-by: Jeff King \n---\nThe cmd_fsck() part of the diff is pretty nasty without\n\"-b\".\n\nI had originally planned to pull the \"stop calling\nverify_pack()\" bit out into its own patch. But doing this\nwithout treating loose and packed objects the same raises a\nlot of questions about why one and not the other. It seemed\nsimpler to just explain it all together.\n\n builtin/fsck.c | 118 +++++++++++++++++++++++++++++++++++++++++++++-----------\n t/t1450-fsck.sh | 23 ++++++++++-\n 2 files changed, 117 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 3e67203f9..f527d8a02 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -205,8 +205,6 @@ static void check_reachable_object(struct object *obj)\n \tif (!(obj->flags & HAS_OBJ)) {\n \t\tif (has_sha1_pack(obj->oid.hash))\n \t\t\treturn; /* it is in pack - forget about it */\n-\t\tif (connectivity_only && has_object_file(&obj->oid))\n-\t\t\treturn;\n \t\tprintf(\"missing %s %s\\n\", typename(obj->type),\n \t\t\tdescribe_object(obj));\n \t\terrors_found |= ERROR_REACHABLE;\n@@ -584,6 +582,79 @@ static int fsck_cache_tree(struct cache_tree *it)\n \treturn err;\n }\n \n+static void mark_object_for_connectivity(const unsigned char *sha1)\n+{\n+\tstruct object *obj = lookup_object(sha1);\n+\n+\t/*\n+\t * Setting the object type here isn't strictly necessary here for a\n+\t * connectivity check. In most cases, our walk will expect a certain\n+\t * type (e.g., a tree referencing a blob) and will use lookup_blob() to\n+\t * assign the type. But doing it here has two advantages:\n+\t *\n+\t * 1. When the fsck_walk code looks at objects that _don't_ come from\n+\t * links (e.g., the tip of a ref), it may complain about the\n+\t * \"unknown object type\".\n+\t *\n+\t * 2. This serves as a nice cross-check that the graph links are\n+\t * sane. So --connectivity-only does not check that the bits of\n+\t * blobs are not corrupted, but it _does_ check that 100644 tree\n+\t * entries point to blobs, and so forth.\n+\t *\n+\t * Unfortunately we can't just use parse_object() here, because the\n+\t * whole point of --connectivity-only is to avoid reading the object\n+\t * data more than necessary.\n+\t */\n+\tif (!obj || obj->type == OBJ_NONE) {\n+\t\tenum object_type type = sha1_object_info(sha1, NULL);\n+\t\tswitch (type) {\n+\t\tcase OBJ_BAD:\n+\t\t\terror(\"%s: unable to read object type\",\n+\t\t\t sha1_to_hex(sha1));\n+\t\t\tbreak;\n+\t\tcase OBJ_COMMIT:\n+\t\t\tobj = (struct object *)lookup_commit(sha1);\n+\t\t\tbreak;\n+\t\tcase OBJ_TREE:\n+\t\t\tobj = (struct object *)lookup_tree(sha1);\n+\t\t\tbreak;\n+\t\tcase OBJ_BLOB:\n+\t\t\tobj = (struct object *)lookup_blob(sha1);\n+\t\t\tbreak;\n+\t\tcase OBJ_TAG:\n+\t\t\tobj = (struct object *)lookup_tag(sha1);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\terror(\"%s: unknown object type %d\",\n+\t\t\t sha1_to_hex(sha1), type);\n+\t\t}\n+\n+\t\tif (!obj || obj->type == OBJ_NONE) {\n+\t\t\terrors_found |= ERROR_OBJECT;\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n+\tobj->flags |= HAS_OBJ;\n+}\n+\n+static int mark_loose_for_connectivity(const unsigned char *sha1,\n+\t\t\t\t const char *path,\n+\t\t\t\t void *data)\n+{\n+\tmark_object_for_connectivity(sha1);\n+\treturn 0;\n+}\n+\n+static int mark_packed_for_connectivity(const unsigned char *sha1,\n+\t\t\t\t\tstruct packed_git *pack,\n+\t\t\t\t\tuint32_t pos,\n+\t\t\t\t\tvoid *data)\n+{\n+\tmark_object_for_connectivity(sha1);\n+\treturn 0;\n+}\n+\n static char const * const fsck_usage[] = {\n \tN_(\"git fsck [] [