{"thread":{"id":"37620","subject":"[RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","startedAt":"2014-09-23T15:47:51Z","lastAt":"2014-10-20T09:21:57Z","messageCount":8,"participants":["Jeff King","Edward Thomson","Ben Aveling"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249793","messageId":"20140923154751.GA19319@peff.net","threadId":"37620","inReplyTo":null,"subject":"[RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-23T15:47:51Z","receivedAt":"2014-09-23T15:47:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Doing so means that we do not actually get to see bogus\nmodes; they are converted into one of our known-good modes\nby decode_tree_entry. We want to see the raw data so that we\ncan complain about it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs far as I can tell, fsck's mode-checking has been totally broken\nbasically forever. Which makes me a little nervous to fix it. :)\nlinux.git does have some bogus modes, but they are 100664, which is\nspecifically ignored here unless \"fsck --strict\" is in effect.\n\nThe implementation feels a bit hacky. The global is ugly, but it avoids\nhaving to pass options all through the call chain of init_tree_entry,\nupdate_tree_entry, etc.\n\nAnother option would be to have decode_tree_entry leave the raw mode in\nthe tree_desc, and only canonicalize it during tree_entry_extract (and\nteach fsck to look at the raw data, not _extract). That would mean\nreverting 7146e66 (tree-walk: finally switch over tree descriptors to\ncontain a pre-parsed entry, 2014-02-06). That commit says there's no\nreal benefit at the time, but that future code might benefit. I don't\nknow if that future code ever materialized (Kirill cc'd).\n\nI thought at first that commit might have been responsible for this\nbreakage, but I don't think so. The canonicalization has happened in\ntree_entry_extract since at least 2006, and we have always called that\nfunction in fsck.\n\n fsck.c          |  2 ++\n t/t1450-fsck.sh | 21 +++++++++++++++++++++\n tree-walk.c     |  4 +++-\n tree-walk.h     |  3 +++\n 4 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 56156ff..ef79396 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -153,6 +153,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \tunsigned o_mode;\n \tconst char *o_name;\n \n+\tdecode_tree_raw = 1;\n \tinit_tree_desc(&desc, item->buffer, item->size);\n \n \to_mode = 0;\n@@ -213,6 +214,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \t\to_name = name;\n \t}\n \n+\tdecode_tree_raw = 0;\n \tretval = 0;\n \tif (has_null_sha1)\n \t\tretval += error_func(&item->object, FSCK_WARN, \"contains entries pointing to null sha1\");\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex b52397a..ba40c6f 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -176,6 +176,27 @@ test_expect_success 'malformatted tree object' '\n \tgrep \"error in tree .*contains duplicate file entries\" out\n '\n \n+# Basically an 8-bit clean version of sed 's/$1/$2/g'.\n+munge_tree_bytes () {\n+\tperl -e '\n+\t\tbinmode(STDIN); binmode(STDOUT);\n+\t\t$_ = do { local $/; <STDIN> };\n+\t\ts/$ARGV[0]/$ARGV[1]/g;\n+\t\tprint\n+\t' \"$@\"\n+}\n+\n+test_expect_success 'bogus mode in tree' '\n+\ttest_when_finished \"remove_object \\$T\" &&\n+\tT=$(\n+\t\tgit cat-file tree HEAD >good &&\n+\t\tmunge_tree_bytes 100644 123456 <good >bad &&\n+\t\tgit hash-object -w -t tree bad\n+\t) &&\n+\tgit fsck 2>out &&\n+\tgrep \"warning.*contains bad file modes\" out\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tcat >invalid-tag <<-\\EOF &&\n \tobject ffffffffffffffffffffffffffffffffffffffff\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 5dd9a71..baca766 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -5,6 +5,8 @@\n #include \"tree.h\"\n #include \"pathspec.h\"\n \n+int decode_tree_raw;\n+\n static const char *get_mode(const char *str, unsigned int *modep)\n {\n \tunsigned char c;\n@@ -37,7 +39,7 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned\n \n \t/* Initialize the descriptor entry */\n \tdesc->entry.path = path;\n-\tdesc->entry.mode = canon_mode(mode);\n+\tdesc->entry.mode = decode_tree_raw ? mode : canon_mode(mode);\n \tdesc->entry.sha1 = (const unsigned char *)(path + len);\n }\n \ndiff --git a/tree-walk.h b/tree-walk.h\nindex ae7fb3a..9f78e85 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -25,6 +25,9 @@ static inline int tree_entry_len(const struct name_entry *ne)\n \treturn (const char *)ne->sha1 - ne->path - 1;\n }\n \n+/* If set, do not canonicalize modes in tree entries. */\n+extern int decode_tree_raw;\n+\n void update_tree_entry(struct tree_desc *);\n void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);\n \n-- \n2.1.1.496.g1ba8081\n"},{"id":"249797","messageId":"20140923162343.GA20379@debian","threadId":"37620","inReplyTo":"20140923154751.GA19319@peff.net","subject":"Re: [RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2014-09-23T16:23:43Z","receivedAt":"2014-09-23T16:23:43Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"On Tue, Sep 23, 2014 at 11:47:51AM -0400, Jeff King wrote:\n> As far as I can tell, fsck's mode-checking has been totally broken\n> basically forever. Which makes me a little nervous to fix it. :)\n> linux.git does have some bogus modes, but they are 100664, which is\n> specifically ignored here unless \"fsck --strict\" is in effect.\n\nI'm in favor of checking the mode in fsck, at least when --strict.  \nBut I would suggest we be lax (by default) about other likely-to-exist\nbut strictly invalid modes to prevent peoples previously workable\nrepositories from being now broken.\n\nI have, for example, encountered 100775 in the wild, and would argue that\nlike 100644, it should probably not fail unless we are in --strict mode.\n\nThanks-\n-ed\n"},{"id":"249798","messageId":"20140923163008.GA21591@peff.net","threadId":"37620","inReplyTo":"20140923162343.GA20379@debian","subject":"Re: [RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-23T16:30:09Z","receivedAt":"2014-09-23T16:30:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[-cc Kirill, as his address seem out-of-date]\n\nOn Tue, Sep 23, 2014 at 04:23:43PM +0000, Edward Thomson wrote:\n\n> On Tue, Sep 23, 2014 at 11:47:51AM -0400, Jeff King wrote:\n> > As far as I can tell, fsck's mode-checking has been totally broken\n> > basically forever. Which makes me a little nervous to fix it. :)\n> > linux.git does have some bogus modes, but they are 100664, which is\n> > specifically ignored here unless \"fsck --strict\" is in effect.\n> \n> I'm in favor of checking the mode in fsck, at least when --strict.  \n> But I would suggest we be lax (by default) about other likely-to-exist\n> but strictly invalid modes to prevent peoples previously workable\n> repositories from being now broken.\n> \n> I have, for example, encountered 100775 in the wild, and would argue that\n> like 100644, it should probably not fail unless we are in --strict mode.\n\nYeah, I'd agree with that. The big question is: what breakage have we\nseen in the wild? :)\n\nI think treating 100775 the same as 100664 makes sense (want to do a\npatch?). Do we know of any others? I guess we can collect them as time\ngoes on and reports come in. That's not the nicest thing for people with\nsuch repos, but then again, their repos _are_ broken (and it's only\nreally a showstopper if they are trying to push to somebody with\nreceive.fsckObjects turned on).\n\n-Peff\n"},{"id":"250532","messageId":"543B02A7.9040807@optusnet.com.au","threadId":"37620","inReplyTo":"20140923163008.GA21591@peff.net","subject":"Re: [RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","fromName":"Ben Aveling","fromEmail":"bena.001@optusnet.com.au","sentAt":"2014-10-12T22:37:27Z","receivedAt":"2014-10-12T22:37:27Z","isPatch":true,"sender":{"key":"bena.001@optusnet.com.au","avatar":null},"body":"\nHi,\n\nA question about fsck - is there a reason it doesn't have an option to \ndelete bad objects?\n\nRegards, Ben\n\nOn 24/09/2014 02:30, Jeff King wrote:\n> [-cc Kirill, as his address seem out-of-date]\n>\n> On Tue, Sep 23, 2014 at 04:23:43PM +0000, Edward Thomson wrote:\n>\n>> On Tue, Sep 23, 2014 at 11:47:51AM -0400, Jeff King wrote:\n>>> As far as I can tell, fsck's mode-checking has been totally broken\n>>> basically forever. Which makes me a little nervous to fix it. :)\n>>> linux.git does have some bogus modes, but they are 100664, which is\n>>> specifically ignored here unless \"fsck --strict\" is in effect.\n>> I'm in favor of checking the mode in fsck, at least when --strict.\n>> But I would suggest we be lax (by default) about other likely-to-exist\n>> but strictly invalid modes to prevent peoples previously workable\n>> repositories from being now broken.\n>>\n>> I have, for example, encountered 100775 in the wild, and would argue that\n>> like 100644, it should probably not fail unless we are in --strict mode.\n> Yeah, I'd agree with that. The big question is: what breakage have we\n> seen in the wild? :)\n>\n> I think treating 100775 the same as 100664 makes sense (want to do a\n> patch?). Do we know of any others? I guess we can collect them as time\n> goes on and reports come in. That's not the nicest thing for people with\n> such repos, but then again, their repos _are_ broken (and it's only\n> really a showstopper if they are trying to push to somebody with\n> receive.fsckObjects turned on).\n>\n> -Peff\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"250607","messageId":"20141014082120.GA16686@peff.net","threadId":"37620","inReplyTo":"543B02A7.9040807@optusnet.com.au","subject":"Re: [RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-14T08:21:21Z","receivedAt":"2014-10-14T08:21:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 13, 2014 at 09:37:27AM +1100, Ben Aveling wrote:\n\n> A question about fsck - is there a reason it doesn't have an option to\n> delete bad objects?\n\nIf the objects are reachable, then deleting them would create other big\nproblems (i.e., we would be breaking the object graph!).\n\nIf they are not, then it is probably safest for them to go away via the\nnormal means (repack/prune via \"git gc\").  Deleting via fsck would mean\nreplicating the reachability and deletion code found elsewhere.\n\n-Peff\n"},{"id":"250740","messageId":"20141016002021.GA27509@peff.net","threadId":"37620","inReplyTo":"543F074B.2050907@optusnet.com.au","subject":"Re: [RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-16T00:20:21Z","receivedAt":"2014-10-16T00:20:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2014 at 10:46:19AM +1100, Ben Aveling wrote:\n\n> On 14/10/2014 19:21, Jeff King wrote:\n> >On Mon, Oct 13, 2014 at 09:37:27AM +1100, Ben Aveling wrote:\n> >>A question about fsck - is there a reason it doesn't have an option to\n> >>delete bad objects?\n> >If the objects are reachable, then deleting them would create other big\n> >problems (i.e., we would be breaking the object graph!).\n> \n> The man page for fsck advises:\n> \n>    \"Any corrupt objects you will have to find in backups or other\n>    archives (i.e., you can just remove them and do an /rsync/ with some\n>    other site in the hopes that somebody else has the object you have\n>    corrupted).\"\n\nI think there are really two different types of corruption worth\nconsidering here.\n\nOne is where the data bytes of the object were correct at one point, but\nnow aren't anymore. The sha1 does not check out, and the object is\nbroken. You need to go find another copy of the object somewhere else.\n\nThe other is where the object was malformed in the first place,\ngenerally due to a software bug (e.g., syntactically bogus email\naddresses on committer lines, trees that are not sorted properly, etc).\nThere is no point in finding another object, as any you find will be\nbyte-for-byte identical.\n\nFor this second type (which is what I thought we were talking about in\nthis thread), deleting the object would be Very Bad.\n\nFor the first type, deleting the object _could_ be a path forward. But\nit's not the usual method for fixing corruption. If you copy a good\nversion of the object into the repository, then a subsequent repack\nshould throw away the broken copy in favor of the good one. Note that\nyou need to copy the objects in manually; you can't use the git protocol\nto do so. But that is true whether you have the broken object or not\n(because git does not communicate with the other side about _every_\nobject you have; it talks about commits, and assumes that you have the\nrest of the object graph that a commit refers to).\n\nSo I don't think deleting really helps in that case. And it may hurt if\nit later turns out you don't have another copy of the object and need to\nrecover what you can from the corrupted pieces (which has actually\nhappened before).\n\n> I ask because I have a corrupt repository, and every time I run fsck, it\n> reports one corrupt object, then stops.\n\nCorrupt how? Bit-corruption, or a malformed object?\n\n> I could write a script to repeatedly call fsck and then remove the\n> next corrupt object, but it raises the question for me; could it make\n> sense to extend fsck with the option to do to the removes? Or even\n> better, do the removes and then do the necessary [r]sync, assuming the\n> user has another repository that has a good copy of the bad objects,\n> which in this case I do.\n\nIf you have a non-corrupted version of the repository, the simplest\nthing is to just pack it, copy the resulting packfile to the broken\nrepository (again, using \"cp\" or \"rsync\" and not git), and then repack\nthere. That won't transfer the minimum amount of data, but it's simple\nand only requires one round-trip (bearing in mind that git doesn't know\nwhat out-of-band method you're using, nor whether we can even initiate a\nrequest from the broken repo to the good one, so each round trip\ngenerally does need to be done by hand).\n\n-Peff\n"},{"id":"250826","messageId":"5443B136.4060009@optusnet.com.au","threadId":"37620","inReplyTo":"20141016002021.GA27509@peff.net","subject":"Re: [RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","fromName":"Ben Aveling","fromEmail":"bena.001@optusnet.com.au","sentAt":"2014-10-19T12:40:22Z","receivedAt":"2014-10-19T12:40:22Z","isPatch":true,"sender":{"key":"bena.001@optusnet.com.au","avatar":null},"body":"On 16/10/14 11:20, Jeff King wrote:\n> On Thu, Oct 16, 2014 at 10:46:19AM +1100, Ben Aveling wrote:\n>\n> I have a corrupt repository, and every time I run fsck, it\n> reports one corrupt object, then stops.\n> Corrupt how? Bit-corruption, or a malformed object?\n\nBit-corruption, in multiple places.\n\n> If you have a non-corrupted version of the repository, the simplest \n> thing is to just pack it, copy the resulting packfile to the broken \n> repository (again, using \"cp\" or \"rsync\" and not git), and then repack \n> there. \n\nThis seems to have worked. I also had to move away the existing .idx and \ncopy in a new one before it was happy.\n\nI'm not sure that what I've done is so different from simply copying the \nother version of the repository - there shouldn't have been anything in \nthe corrupt version that wasn't also in the good one. But any rate, it \nworked.\n\nThanks, Ben\n"},{"id":"250848","messageId":"20141020092157.GC12913@peff.net","threadId":"37620","inReplyTo":"5443B136.4060009@optusnet.com.au","subject":"Re: [RFC/PATCH] fsck: do not canonicalize modes in trees we are checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-20T09:21:57Z","receivedAt":"2014-10-20T09:21:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 19, 2014 at 11:40:22PM +1100, Ben Aveling wrote:\n\n> This seems to have worked. I also had to move away the existing .idx and\n> copy in a new one before it was happy.\n\nSorry if I wasn't clear; you do need to copy the .idx files along with\nthe packfiles (you can regenerate the .idx files from the packfiles on\nthe destination, but they're not that big; it's probably easier just to\ncopy them).\n\n> I'm not sure that what I've done is so different from simply copying the\n> other version of the repository - there shouldn't have been anything in the\n> corrupt version that wasn't also in the good one. But any rate, it worked.\n\nRight, a valid technique for repairing corruption is to just blow away\nthe original repo entirely and replace it with a good copy. But this is a\nway of ensuring that no commits are missed, and keeping the original set\nof refs, config options, and reflogs.\n\n-Peff\n"}]}