{"thread":{"id":"44173","subject":"[PATCH v3 1/3] tree-walk: be more specific about corrupt tree errors","startedAt":"2016-09-27T15:23:39Z","lastAt":"2016-09-27T19:03:33Z","messageCount":7,"participants":["David Turner","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"302706","messageId":"1474989806-5002-1-git-send-email-dturner@twosigma.com","threadId":"44173","inReplyTo":null,"subject":"[PATCH v3 1/3] tree-walk: be more specific about corrupt tree errors","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-09-27T15:23:24Z","receivedAt":"2016-09-27T15:23:39Z","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 | 21 +++++++++++++++++++--\n tree-walk.c | 12 +++++++-----\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex acca9ac..0691b88 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -183,9 +183,26 @@ 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+test_expect_success 'malformed mode in tree' '\n+\thex_sha1=$(echo foo | git hash-object --stdin -w) &&\n+\tbin_sha1=$(echo $hex_sha1 | perl -ne \"printf \\\"\\\\\\\\%03o\\\", ord for /../g\") &&\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 | perl -ne \"printf \\\"\\\\\\\\%03o\\\", ord for /../g\") &&\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":"302707","messageId":"1474989806-5002-2-git-send-email-dturner@twosigma.com","threadId":"44173","inReplyTo":"1474989806-5002-1-git-send-email-dturner@twosigma.com","subject":"[PATCH v3 2/3] fsck: handle bad trees like other errors","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-09-27T15:23:25Z","receivedAt":"2016-09-27T15:23:48Z","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 | 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..0fcd38a 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+\t! test_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":"302708","messageId":"1474989806-5002-3-git-send-email-dturner@twosigma.com","threadId":"44173","inReplyTo":"1474989806-5002-1-git-send-email-dturner@twosigma.com","subject":"[PATCH v3 3/3] add David Turner's Two Sigma address","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-09-27T15:23:26Z","receivedAt":"2016-09-27T15:23:52Z","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":"302719","messageId":"xmqqwphxp9iy.fsf@gitster.mtv.corp.google.com","threadId":"44173","inReplyTo":"1474989806-5002-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v3 1/3] tree-walk: be more specific about corrupt tree errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T16:55:33Z","receivedAt":"2016-09-27T16:55:56Z","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 | 21 +++++++++++++++++++--\n> tree-walk.c | 12 +++++++-----\n> 2 files changed, 26 insertions(+), 7 deletions(-)\n\nNice that we now prepare the test data ourselves without shipping as\npart of the source.\n\n"},{"id":"302720","messageId":"xmqqshslp96j.fsf@gitster.mtv.corp.google.com","threadId":"44173","inReplyTo":"1474989806-5002-3-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v3 3/3] add David Turner's Two Sigma address","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T17:03:00Z","receivedAt":"2016-09-27T17:03:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> From: David Turner \n>\n> Signed-off-by: David Turner \n> ---\n> .mailmap | 1 +\n> 1 file changed, 1 insertion(+)\n\nThanks. Queued separately in order to merge to master much earlier\nthan the tree-fsck topic.\n\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"},{"id":"302739","messageId":"20160927185925.hkqlq63oeafldnrz@sigill.intra.peff.net","threadId":"44173","inReplyTo":"1474989806-5002-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v3 1/3] tree-walk: be more specific about corrupt tree errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T18:59:25Z","receivedAt":"2016-09-27T18:59:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2016 at 11:23:24AM -0400, David Turner wrote:\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 | perl -ne \"printf \\\"\\\\\\\\%03o\\\", ord for /../g\") &&\n\nSorry, the perl snippet I posted earlier was totally braindead. It gives\nyou the octal for the raw bytes, but we really just want to convert the\nhex to octal (we could also print the raw bytes from the hex, but I\ndidn't want to take chances on shells that can't handle NULs in\nenvironment variables).\n\nI also find it helps to define a helper function outside of the test\nblock to avoid quoting hell. So something like:\n\n hex2oct () {\n\tperl -ne 'printf \"\\\\%03o\", hex for /../g'\n }\n\n test_expect_success ... '\n\tbin_sha1=$(echo $hex_sha1 | hex2oct)\n '\n\n-Peff\n"},{"id":"302741","messageId":"20160927190326.6wdmxodfisdiku2c@sigill.intra.peff.net","threadId":"44173","inReplyTo":"1474989806-5002-2-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v3 2/3] fsck: handle bad trees like other errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-27T19:03:26Z","receivedAt":"2016-09-27T19:03:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2016 at 11:23:25AM -0400, David Turner wrote:\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> +\t! test_i18ngrep \"fatal: empty filename in tree entry\" out\n> +'\n\nUnfortunately this last one needs to be spelled as:\n\n test_i18ngrep ! \"fatal: empty filename in tree entry\" out\n\nbecause the function always pretends to match when GETTEXT_POISON is\nset.\n\nOther than the minor test fixups, this all looks good to me.\n\n-Peff\n"}]}