{"thread":{"id":"44167","subject":"[PATCH 2/3] fsck: handle bad trees like other errors","startedAt":"2016-09-27T00:12:22Z","lastAt":"2016-09-27T04:54:06Z","messageCount":6,"participants":["David Turner","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"302660","messageId":"1474935093-26757-2-git-send-email-dturner@twosigma.com","threadId":"44167","inReplyTo":"1474935093-26757-1-git-send-email-dturner@twosigma.com","subject":"[PATCH 2/3] fsck: handle bad trees like other errors","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-09-27T00:11:32Z","receivedAt":"2016-09-27T00:12:22Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Instead of dying when fsck hits a malformed tree object, log the error\nlike any other and continue. Now fsck can tell the user which tree is\nbad, too.\n\nSigned-off-by: David Turner \n---\n fsck.c | 18 +++--\n t/t1450-fsck.sh | 17 ++++-\n t/t1450/bad-objects/.gitattributes | 1 +\n .../307e300745b82417cc1a903f875c7d22e45ef907 | Bin 0 -> 137 bytes\n .../f506a346749bb96f52d8605ffba9fb93d46b5ffd | Bin 0 -> 45 bytes\n tree-walk.c | 83 ++++++++++++++++++---\n tree-walk.h | 8 ++\n 7 files changed, 108 insertions(+), 19 deletions(-)\n create mode 100644 t/t1450/bad-objects/.gitattributes\n create mode 100644 t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907\n create mode 100644 t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd\n\ndiff --git a/fsck.c b/fsck.c\nindex c9cf3de..4a3069e 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\treturn -1;\n \n \tname = get_object_name(options, &tree->object);\n-\tinit_tree_desc(&desc, tree->buffer, tree->size);\n-\twhile (tree_entry(&desc, &entry)) {\n+\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n+\t\treturn -1;\n+\twhile (tree_entry_gently(&desc, &entry)) {\n \t\tstruct object *obj;\n \t\tint result;\n \n@@ -520,7 +521,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n \n static int fsck_tree(struct tree *item, struct fsck_options *options)\n {\n-\tint retval;\n+\tint retval = 0;\n \tint has_null_sha1 = 0;\n \tint has_full_path = 0;\n \tint has_empty_name = 0;\n@@ -535,7 +536,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \tunsigned o_mode;\n \tconst char *o_name;\n \n-\tinit_tree_desc(&desc, item->buffer, item->size);\n+\tif (init_tree_desc_gently(&desc, item->buffer, item->size)) {\n+\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\treturn retval;\n+\t}\n \n \to_mode = 0;\n \to_name = NULL;\n@@ -556,7 +560,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \t\t\t is_hfs_dotgit(name) ||\n \t\t\t is_ntfs_dotgit(name));\n \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n-\t\tupdate_tree_entry(&desc);\n+\t\tif (update_tree_entry_gently(&desc)) {\n+\t\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tbreak;\n+\t\t}\n \n \t\tswitch (mode) {\n \t\t/*\n@@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \t\to_name = name;\n \t}\n \n-\tretval = 0;\n \tif (has_null_sha1)\n \t\tretval += report(options, &item->object, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8f52da2..f456963 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -188,8 +188,7 @@ test_expect_success 'commit with NUL in header' '\n \tgrep \"error in commit $new.*unterminated header: NUL at offset\" out\n '\n \n-test_expect_success 'malformatted tree object' '\n-\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+test_expect_success 'tree object with duplicate entries' '\n \ttest_when_finished \"remove_object \\$T\" &&\n \tT=$(\n \t\tGIT_INDEX_FILE=test-index &&\n@@ -208,6 +207,20 @@ test_expect_success 'malformatted tree object' '\n \tgrep \"error in tree .*contains duplicate file entries\" out\n '\n \n+test_expect_success 'unparseable tree object' '\n+\ttest_when_finished \"git update-ref -d refs/heads/wrong\" &&\n+\ttest_when_finished \"remove_object 307e300745b82417cc1a903f875c7d22e45ef907\" &&\n+\ttest_when_finished \"remove_object f506a346749bb96f52d8605ffba9fb93d46b5ffd\" &&\n+\tmkdir -p .git/objects/30 mkdir -p .git/objects/f5 &&\n+\tcp ../t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 .git/objects/30/7e300745b82417cc1a903f875c7d22e45ef907 &&\n+\tcp ../t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd .git/objects/f5/06a346749bb96f52d8605ffba9fb93d46b5ffd &&\n+\tgit update-ref refs/heads/wrong 307e300745b82417cc1a903f875c7d22e45ef907 &&\n+\ttest_must_fail git fsck 2>out &&\n+\tgrep \"warning: empty filename in tree entry\" out &&\n+\tgrep \"f506a346749bb96f52d8605ffba9fb93d46b5ffd\" out &&\n+\t! grep \"fatal: empty filename in tree entry\" out\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tcat >invalid-tag <<-\\EOF &&\n \tobject ffffffffffffffffffffffffffffffffffffffff\ndiff --git a/t/t1450/bad-objects/.gitattributes b/t/t1450/bad-objects/.gitattributes\nnew file mode 100644\nindex 0000000..a173f27\n--- /dev/null\n+++ b/t/t1450/bad-objects/.gitattributes\n@@ -0,0 +1 @@\n+[0-9a-f]*[0-9a-f]\t-diff\ndiff --git a/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..6e23d625531856540364837ad76f8ce620b16102\nGIT binary patch\nliteral 137\nzcmV;40CxX)0iBLP4#FT1MO|}>xqxP{K%K-G7aqY2Fa=r?TM`QO`l9IxT>bpTd;bq<\nzo?`(?@=&t(5HuRwDbp)rCKL48T@30F*ivBXoHE>+6SkHqWq8;vI(XK+_zc%2ZT1z{\nr`<|zi#~Vo1WDn*Fxv)5_&?<#!Z>Hoon;loq@NdS%f\nB6F2|>\n\nliteral 0\nHcmV?d00001\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex ba544cf..0fb830b 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -22,33 +22,60 @@ static const char *get_mode(const char *str, unsigned int *modep)\n \treturn str;\n }\n \n-static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)\n+static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)\n {\n \tconst char *path;\n \tunsigned int mode, len;\n \n-\tif (size < 23 || buf[size - 21])\n-\t\tdie(\"too-short tree object\");\n+\tif (size < 23 || buf[size - 21]) {\n+\t\tstrbuf_addstr(err, \"too-short tree object\");\n+\t\treturn -1;\n+\t}\n \n \tpath = get_mode(buf, &mode);\n-\tif (!path)\n-\t\tdie(\"malformed mode in tree entry for tree\");\n-\tif (!*path)\n-\t\tdie(\"empty filename in tree entry for tree\");\n+\tif (!path) {\n+\t\tstrbuf_addstr(err, \"malformed mode in tree entry\");\n+\t\treturn -1;\n+\t}\n+\tif (!*path) {\n+\t\tstrbuf_addstr(err, \"empty filename in tree entry\");\n+\t\treturn -1;\n+\t}\n \tlen = strlen(path) + 1;\n \n \t/* Initialize the descriptor entry */\n \tdesc->entry.path = path;\n \tdesc->entry.mode = canon_mode(mode);\n \tdesc->entry.oid = (const struct object_id *)(path + len);\n+\n+\treturn 0;\n }\n \n-void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)\n+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)\n {\n \tdesc->buffer = buffer;\n \tdesc->size = size;\n \tif (size)\n-\t\tdecode_tree_entry(desc, buffer, size);\n+\t\treturn decode_tree_entry(desc, buffer, size, err);\n+\treturn 0;\n+}\n+\n+void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (init_tree_desc_internal(desc, buffer, size, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+}\n+\n+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint result = init_tree_desc_internal(desc, buffer, size, &err);\n+\tif (result)\n+\t\twarning(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+\treturn result;\n }\n \n void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)\n@@ -75,7 +102,7 @@ static void entry_extract(struct tree_desc *t, struct name_entry *a)\n \t*a = t->entry;\n }\n \n-void update_tree_entry(struct tree_desc *desc)\n+static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err)\n {\n \tconst void *buf = desc->buffer;\n \tconst unsigned char *end = desc->entry.oid->hash + 20;\n@@ -89,7 +116,30 @@ void update_tree_entry(struct tree_desc *desc)\n \tdesc->buffer = buf;\n \tdesc->size = size;\n \tif (size)\n-\t\tdecode_tree_entry(desc, buf, size);\n+\t\treturn decode_tree_entry(desc, buf, size, err);\n+\treturn 0;\n+}\n+\n+void update_tree_entry(struct tree_desc *desc)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (update_tree_entry_internal(desc, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+}\n+\n+int update_tree_entry_gently(struct tree_desc *desc)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (update_tree_entry_internal(desc, &err)) {\n+\t\twarning(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\t/* Stop processing this tree after error */\n+\t\tdesc->size = 0;\n+\t\treturn -1;\n+\t}\n+\tstrbuf_release(&err);\n+\treturn 0;\n }\n \n int tree_entry(struct tree_desc *desc, struct name_entry *entry)\n@@ -102,6 +152,17 @@ int tree_entry(struct tree_desc *desc, struct name_entry *entry)\n \treturn 1;\n }\n \n+int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)\n+{\n+\tif (!desc->size)\n+\t\treturn 0;\n+\n+\t*entry = desc->entry;\n+\tif (update_tree_entry_gently(desc))\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n void setup_traverse_info(struct traverse_info *info, const char *base)\n {\n \tint pathlen = strlen(base);\ndiff --git a/tree-walk.h b/tree-walk.h\nindex 97a7d69..68bb78b 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -25,14 +25,22 @@ static inline int tree_entry_len(const struct name_entry *ne)\n \treturn (const char *)ne->oid - ne->path - 1;\n }\n \n+/*\n+ * The _gently versions of these functions warn and return false on a\n+ * corrupt tree entry rather than dying,\n+ */\n+\n void update_tree_entry(struct tree_desc *);\n+int update_tree_entry_gently(struct tree_desc *);\n void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);\n+int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);\n \n /*\n * Helper function that does both tree_entry_extract() and update_tree_entry()\n * and returns true for success\n */\n int tree_entry(struct tree_desc *, struct name_entry *);\n+int tree_entry_gently(struct tree_desc *, struct name_entry *);\n \n void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);\n \n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"302661","messageId":"1474935093-26757-3-git-send-email-dturner@twosigma.com","threadId":"44167","inReplyTo":"1474935093-26757-1-git-send-email-dturner@twosigma.com","subject":"[PATCH 3/3] add David Turner's Two Sigma address","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-09-27T00:11:33Z","receivedAt":"2016-09-27T00:12:26Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: David Turner \n\nSigned-off-by: David Turner \n---\n .mailmap | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/.mailmap b/.mailmap\nindex 9441a54..9cc33e9 100644\n--- a/.mailmap\n+++ b/.mailmap\n@@ -48,6 +48,7 @@ David KÃ¥gedal \n David Reiss \n David S. Miller \n David Turner \n+David Turner \n Deskin Miller \n Dirk Süsserott \n Eric Blake \n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"302662","messageId":"1474935093-26757-1-git-send-email-dturner@twosigma.com","threadId":"44167","inReplyTo":null,"subject":"[PATCH 1/3] tree-walk: be more specific about corrupt tree errors","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-09-27T00:11:31Z","receivedAt":"2016-09-27T00:12:27Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: Jeff King \n\nWhen the tree-walker runs into an error, it just calls\ndie(), and the message is always \"corrupt tree file\".\nHowever, we are actually covering several cases here; let's\ngive the user a hint about what happened.\n\nLet's also avoid using the word \"corrupt\", which makes it\nseem like the data bit-rotted on disk. Our sha1 check would\nalready have found that. These errors are ones of data that\nis malformed in the first place.\n\nSigned-off-by: David Turner \nSigned-off-by: Jeff King \n---\n t/t1007-hash-object.sh | 15 +++++++++++++--\n t/t1007/.gitattributes | 1 +\n t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes\n t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes\n tree-walk.c | 12 +++++++-----\n 5 files changed, 21 insertions(+), 7 deletions(-)\n create mode 100644 t/t1007/.gitattributes\n create mode 100644 t/t1007/tree-with-empty-filename\n create mode 100644 t/t1007/tree-with-malformed-mode\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex acca9ac..f21848b 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -183,9 +183,20 @@ for args in \"-w --stdin-paths\" \"--stdin-paths -w\"; do\n \tpop_repo\n done\n \n-test_expect_success 'corrupt tree' '\n+test_expect_success 'too-short tree' '\n \techo abc >malformed-tree &&\n-\ttest_must_fail git hash-object -t tree malformed-tree\n+\ttest_must_fail git hash-object -t tree malformed-tree 2>err &&\n+\tgrep \"too-short tree object\" err\n+'\n+\n+test_expect_success 'malformed mode in tree' '\n+\ttest_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&\n+\tgrep \"malformed mode in tree entry\" err\n+'\n+\n+test_expect_success 'empty filename in tree' '\n+\ttest_must_fail git hash-object -t tree ../t1007/tree-with-empty-filename 2>err &&\n+\tgrep \"empty filename in tree entry\" err\n '\n \n test_expect_success 'corrupt commit' '\ndiff --git a/t/t1007/.gitattributes b/t/t1007/.gitattributes\nnew file mode 100644\nindex 0000000..7352ef5\n--- /dev/null\n+++ b/t/t1007/.gitattributes\n@@ -0,0 +1 @@\n+tree-with-*\t-diff\ndiff --git a/t/t1007/tree-with-empty-filename b/t/t1007/tree-with-empty-filename\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..aeb1ceb20e485eebd0acbb81c974d1c6fedcc1fe\nGIT binary patch\nliteral 28\nkcmXpsFfcPQQDAsB_tET47q2;ccWbUIkGgT_Nl)-Z0Hx{;SO5S3\n\nliteral 0\nHcmV?d00001\n\ndiff --git a/t/t1007/tree-with-malformed-mode b/t/t1007/tree-with-malformed-mode\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..24aa84d60ef8e269fb0b29c67b5208639b9da3ae\nGIT binary patch\nliteral 39\nvcmYewPcJRb%}+^HNXyJg%}dNpWq3CC(dbuffer = buf;\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"302663","messageId":"1474935191.26902.0.camel@frank","threadId":"44167","inReplyTo":"1474935093-26757-3-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH 3/3] add David Turner's Two Sigma address","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-09-27T00:13:11Z","receivedAt":"2016-09-27T00:13:33Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Sorry for the bad subject line, this is of course v2 of the series.\n\nOn Mon, 2016-09-26 at 20:11 -0400, David Turner wrote:\n> From: David Turner \n> \n> Signed-off-by: David Turner \n> ---\n> .mailmap | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/.mailmap b/.mailmap\n> index 9441a54..9cc33e9 100644\n> --- a/.mailmap\n> +++ b/.mailmap\n> @@ -48,6 +48,7 @@ David Kågedal \n> David Reiss \n> David S. Miller \n> David Turner \n> +David Turner \n> Deskin Miller \n> Dirk Süsserott \n> Eric Blake \n\n\n"},{"id":"302671","messageId":"xmqqtwd2ng8k.fsf@gitster.mtv.corp.google.com","threadId":"44167","inReplyTo":"1474935093-26757-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH 1/3] tree-walk: be more specific about corrupt tree errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T04:01:15Z","receivedAt":"2016-09-27T04:01:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> From: Jeff King \n>\n> When the tree-walker runs into an error, it just calls\n> die(), and the message is always \"corrupt tree file\".\n> However, we are actually covering several cases here; let's\n> give the user a hint about what happened.\n>\n> Let's also avoid using the word \"corrupt\", which makes it\n> seem like the data bit-rotted on disk. Our sha1 check would\n> already have found that. These errors are ones of data that\n> is malformed in the first place.\n>\n> Signed-off-by: David Turner \n> Signed-off-by: Jeff King \n> ---\n> t/t1007-hash-object.sh | 15 +++++++++++++--\n> t/t1007/.gitattributes | 1 +\n> t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes\n> t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes\n> tree-walk.c | 12 +++++++-----\n> 5 files changed, 21 insertions(+), 7 deletions(-)\n> create mode 100644 t/t1007/.gitattributes\n> create mode 100644 t/t1007/tree-with-empty-filename\n> create mode 100644 t/t1007/tree-with-malformed-mode\n\nI hate to report this, but this alone, or together with 2/2, when\nmerged to 'pu', I cannot get them to pass the tests in my automated\nintegration tests, even though they seem to pass when the problematic\ntests are run manually. I do not see offhand anything suspicious\n(like something that may be racy) in these two patches but I haven't\nfigured out where it goes wrong.\n\nIf somebody manages to find breakages in today's 'pu', please (1) do\nnot be too alarmed, and (2) help figure out where things are broken.\n\nThanks.\n"},{"id":"302673","messageId":"20160927045358.wledqitv4qaqmacl@sigill.intra.peff.net","threadId":"44167","inReplyTo":"xmqqtwd2ng8k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] tree-walk: be more specific about corrupt tree errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T04:53:58Z","receivedAt":"2016-09-27T04:54:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2016 at 09:01:15PM -0700, Junio C Hamano wrote:\n\n> > 5 files changed, 21 insertions(+), 7 deletions(-)\n> > create mode 100644 t/t1007/.gitattributes\n> > create mode 100644 t/t1007/tree-with-empty-filename\n> > create mode 100644 t/t1007/tree-with-malformed-mode\n> \n> I hate to report this, but this alone, or together with 2/2, when\n> merged to 'pu', I cannot get them to pass the tests in my automated\n> integration tests, even though they seem to pass when the problematic\n> tests are run manually. I do not see offhand anything suspicious\n> (like something that may be racy) in these two patches but I haven't\n> figured out where it goes wrong.\n> \n> If somebody manages to find breakages in today's 'pu', please (1) do\n> not be too alarmed, and (2) help figure out where things are broken.\n\nI think the problem is just that they refer to t/t1450 (and t1007) from\nthe trash directory as \"../t1450\". That breaks when the test is run with\n\"--root\" (and I imagine that like me, you have --root as part of your\nautomated tests but do not bother with it when doing a one-off run).\n\n-Peff\n"}]}