{"thread":{"id":"44177","subject":"[PATCH v4 2/2] fsck: handle bad trees like other errors","startedAt":"2016-09-27T21:01:00Z","lastAt":"2016-09-27T21:45:09Z","messageCount":3,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"302768","messageId":"1475009991-16368-2-git-send-email-dturner@twosigma.com","threadId":"44177","inReplyTo":"1475009991-16368-1-git-send-email-dturner@twosigma.com","subject":"[PATCH v4 2/2] fsck: handle bad trees like other errors","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-09-27T20:59:51Z","receivedAt":"2016-09-27T21:01:00Z","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 <dturner@twosigma.com>\n---\n fsck.c          | 18 ++++++++-----\n t/t1450-fsck.sh | 16 +++++++++--\n tree-walk.c     | 83 +++++++++++++++++++++++++++++++++++++++++++++++++--------\n tree-walk.h     |  8 ++++++\n 4 files changed, 106 insertions(+), 19 deletions(-)\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..ee7d473 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,19 @@ 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 \\$tree_sha1\" &&\n+\ttest_when_finished \"remove_object \\$commit_sha1\" &&\n+\ttree_sha1=$(printf \"100644 \\0twenty-bytes-of-junk\" | git hash-object -t tree --stdin -w --literally) &&\n+\tcommit_sha1=$(git commit-tree $tree_sha1) &&\n+\tgit update-ref refs/heads/wrong $commit_sha1 &&\n+\ttest_must_fail git fsck 2>out &&\n+\ttest_i18ngrep \"error: empty filename in tree entry\" out &&\n+\ttest_i18ngrep \"$tree_sha1\" out &&\n+\ttest_i18ngrep ! \"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/tree-walk.c b/tree-walk.c\nindex 24f9a0f..828f435 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\terror(\"%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\terror(\"%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":"302769","messageId":"1475009991-16368-1-git-send-email-dturner@twosigma.com","threadId":"44177","inReplyTo":null,"subject":"[PATCH v4 1/2] tree-walk: be more specific about corrupt tree errors","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-09-27T20:59:50Z","receivedAt":"2016-09-27T21:01:03Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: Jeff King <peff@peff.net>\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 <dturner@twosigma.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t1007-hash-object.sh | 25 +++++++++++++++++++++++--\n tree-walk.c            | 12 +++++++-----\n 2 files changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex acca9ac..c5245c5 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -183,9 +183,30 @@ 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+\ttest_i18ngrep \"too-short tree object\" err\n+'\n+\n+hex2oct() {\n+    perl -ne 'printf \"\\\\%03o\", hex for /../g'\n+}\n+\n+test_expect_success 'malformed mode in tree' '\n+\thex_sha1=$(echo foo | git hash-object --stdin -w) &&\n+\tbin_sha1=$(echo $hex_sha1 | hex2oct) &&\n+\tprintf \"9100644 \\0$bin_sha1\" >tree-with-malformed-mode &&\n+\ttest_must_fail git hash-object -t tree tree-with-malformed-mode 2>err &&\n+\ttest_i18ngrep \"malformed mode in tree entry\" err\n+'\n+\n+test_expect_success 'empty filename in tree' '\n+\thex_sha1=$(echo foo | git hash-object --stdin -w) &&\n+\tbin_sha1=$(echo $hex_sha1 | hex2oct) &&\n+\tprintf \"100644 \\0$bin_sha1\" >tree-with-empty-filename &&\n+\ttest_must_fail git hash-object -t tree tree-with-empty-filename 2>err &&\n+\ttest_i18ngrep \"empty filename in tree entry\" err\n '\n \n test_expect_success 'corrupt commit' '\ndiff --git a/tree-walk.c b/tree-walk.c\nindex ce27842..24f9a0f 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -27,12 +27,14 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned\n \tconst char *path;\n \tunsigned int mode, len;\n \n-\tif (size < 24 || buf[size - 21])\n-\t\tdie(\"corrupt tree file\");\n+\tif (size < 23 || buf[size - 21])\n+\t\tdie(_(\"too-short tree object\"));\n \n \tpath = get_mode(buf, &mode);\n-\tif (!path || !*path)\n-\t\tdie(\"corrupt tree file\");\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 \tlen = strlen(path) + 1;\n \n \t/* Initialize the descriptor entry */\n@@ -81,7 +83,7 @@ void update_tree_entry(struct tree_desc *desc)\n \tunsigned long len = end - (const unsigned char *)buf;\n \n \tif (size < len)\n-\t\tdie(\"corrupt tree file\");\n+\t\tdie(_(\"too-short tree file\"));\n \tbuf = end;\n \tsize -= len;\n \tdesc->buffer = buf;\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"302772","messageId":"xmqq1t05m2zn.fsf@gitster.mtv.corp.google.com","threadId":"44177","inReplyTo":"1475009991-16368-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v4 1/2] tree-walk: be more specific about corrupt tree errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T21:45:00Z","receivedAt":"2016-09-27T21:45:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, will queue both.\n"}]}