{"thread":{"id":"58273","subject":"fsck: BAD_FILEMODE diagnostic broken / never triggers","startedAt":"2022-08-06T13:44:07Z","lastAt":"2022-08-14T07:03:39Z","messageCount":17,"participants":["Xavier Morel","Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"460747","messageId":"B3488A12-BCE3-48C8-915C-E2AC4E71ECD2@masklinn.net","threadId":"58273","inReplyTo":null,"subject":"fsck: BAD_FILEMODE diagnostic broken / never triggers","fromName":"Xavier Morel","fromEmail":"xavier.morel@masklinn.net","sentAt":"2022-08-06T13:43:56Z","receivedAt":"2022-08-06T13:44:07Z","isPatch":false,"sender":{"key":"xavier.morel@masklinn.net","avatar":null},"body":"While looking at git's handling of invalid objects, I stumbled upon\ngit apparently not caring much about file mode values, except for the\nfollowing requirements:\n\n- must be non-empty (badTree)\n- must be octal (badTree)\n- should not be zero-padded (zeroPaddedFilemode)\n\nLooking at tree-walk this is obvious from decode_tree_entry: it calls\ncanon_mode, which\n\n- checks the S_IFMT exactly, and falls back on 160000 (S_IFGITLINK)\n  for cases other than S_ISREG, S_ISLNK, or S_ISDIR\n- returns the corresponding S_IF (with the access modes unset) for all \n  cases other than S_ISREG, for which it returns 755 if S_IXUSR is set,\n  otherwise 644\n\nHowever looking at fsck_tree, it does have a fair amount of code to\nvalidate entry modes and a dedicated message id for this (BAD_FILEMODE\n/ badFilemode), it even has a code path for legacy entries with\nS_IWGRP set (extensively documented under `git fsck --strict`).\n\nI guess, over time mode canonicalisation has slowly creeped earlier\nthe tree-parsing code, and (seemingly for several years) it has been\noccurring before \"git fsck\" gets tree entry information, so git fsck\nsimply can not see invalid entry modes?\n\nReproduction:\n\n- create an empty git repository\n- create tree for (or with) any possible entry type value\n- run `git fsck --strict --no-dangling`\n\nExpected:\n\nall the trees except the 5 with valid filemodes should be flagged\n\nObserved:\n\n> git fsck --strict --no-dangling\nChecking object directories: 100% (256/256), done.\nnotice: HEAD points to an unborn branch (master)\nnotice: No default references\n\nScript to generate one tree for each filemode between 1 and 777777\n(inclusive), can be run from a (non-bare) repository root, beware that\nthe resulting repository takes about 1GB.\n\n\nfrom hashlib import sha1\nfrom pathlib import Path\nfrom zlib import compress\n\ndef hash_object(type, content):\n    obj = b\"%s %d\\0%s\" % (type.encode(), len(content), content)\n    oid = sha1(obj).hexdigest()\n\n    e_dir = GIT_ROOT / 'objects' / oid[:2]\n    e_dir.mkdir(exist_ok=True, parents=True)\n\n    e = e_dir / oid[2:]\n    if not e.exists():\n        e.write_bytes(compress(obj))\n\n    return oid\n\n# technically S_IFMT caps out at 200_000 - 1, but get_mode doesn't\n# actually have a limit\nMAX_ENTRY = 0o777_777\nif __name__ == '__main__':\n    GIT_ROOT = Path.cwd() / '.git'\n    assert GIT_ROOT.is_dir()\n    \n    blob_hex = hash_object('blob', b\"\")\n    blob_oid = bytes.fromhex(blob_hex)\n    print(\"empty blob\", blob_hex)\n\n    tree_hex = hash_object(\"tree\", b\"100644 empty_blob\\0\" + blob_oid)\n    tree_oid = bytes.fromhex(tree_hex)\n    print(\"base tree\", tree_hex)\n\n    for mode in range(1, MAX_ENTRY+1):\n        print(f\"\\r{mode:06o}/{MAX_ENTRY:o}\", end=' ')\n        tree = b\"%o x\\0%s\" % (\n            mode,\n            tree_oid if (mode & 0o170_000) == 0o040_000 else blob_oid,\n        )\n        h = hash_object(\"tree\", tree)\n        print(\"=\", h, end='', flush=True)\n\n"},{"id":"460952","messageId":"YvLkU3X9lBsG8wXp@coredump.intra.peff.net","threadId":"58273","inReplyTo":"B3488A12-BCE3-48C8-915C-E2AC4E71ECD2@masklinn.net","subject":"Re: fsck: BAD_FILEMODE diagnostic broken / never triggers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-09T22:48:51Z","receivedAt":"2022-08-09T22:48:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 06, 2022 at 03:43:56PM +0200, Xavier Morel wrote:\n\n> However looking at fsck_tree, it does have a fair amount of code to\n> validate entry modes and a dedicated message id for this (BAD_FILEMODE\n> / badFilemode), it even has a code path for legacy entries with\n> S_IWGRP set (extensively documented under `git fsck --strict`).\n> \n> I guess, over time mode canonicalisation has slowly creeped earlier\n> the tree-parsing code, and (seemingly for several years) it has been\n> occurring before \"git fsck\" gets tree entry information, so git fsck\n> simply can not see invalid entry modes?\n\nYes, I think you're right. I didn't bisect, but I suspect this goes back\nto 7146e66f08 (tree-walk: finally switch over tree descriptors to\ncontain a pre-parsed entry, 2014-02-06).\n\nWe probably need to provide version of decode_tree_entry() which gives\nthe non-canonical mode, and to call it from fsck.\n\n-Peff\n"},{"id":"460953","messageId":"YvLttyyaZWm4pitu@coredump.intra.peff.net","threadId":"58273","inReplyTo":"YvLkU3X9lBsG8wXp@coredump.intra.peff.net","subject":"Re: fsck: BAD_FILEMODE diagnostic broken / never triggers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-09T23:28:55Z","receivedAt":"2022-08-09T23:29:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 09, 2022 at 06:48:51PM -0400, Jeff King wrote:\n\n> On Sat, Aug 06, 2022 at 03:43:56PM +0200, Xavier Morel wrote:\n> \n> > However looking at fsck_tree, it does have a fair amount of code to\n> > validate entry modes and a dedicated message id for this (BAD_FILEMODE\n> > / badFilemode), it even has a code path for legacy entries with\n> > S_IWGRP set (extensively documented under `git fsck --strict`).\n> > \n> > I guess, over time mode canonicalisation has slowly creeped earlier\n> > the tree-parsing code, and (seemingly for several years) it has been\n> > occurring before \"git fsck\" gets tree entry information, so git fsck\n> > simply can not see invalid entry modes?\n> \n> Yes, I think you're right. I didn't bisect, but I suspect this goes back\n> to 7146e66f08 (tree-walk: finally switch over tree descriptors to\n> contain a pre-parsed entry, 2014-02-06).\n\nHmm, actually, I think this may have been broken since the beginning of\nfsck. At least I could not find a version for which the test below\nbehaves as expected.\n\n> We probably need to provide version of decode_tree_entry() which gives\n> the non-canonical mode, and to call it from fsck.\n\nPerhaps something like the patch below.\n\nNote that these are treated with a severity of \"warning\", so fsck won't\ngive a non-zero exit. I think it still enough for transfer.fsckObjects\nto mark them. I kind of wonder if fixing this at this point might create\nmore problems than it solves though (e.g., if people have broken modes\nin historical objects that servers may now reject).\n\n---\ndiff --git a/fsck.c b/fsck.c\nindex dd4822ba1b..b3da1d68c0 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -308,7 +308,7 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\treturn -1;\n \n \tname = fsck_get_object_name(options, &tree->object.oid);\n-\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n+\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size, 0))\n \t\treturn -1;\n \twhile (tree_entry_gently(&desc, &entry)) {\n \t\tstruct object *obj;\n@@ -578,7 +578,7 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tconst char *o_name;\n \tstruct name_stack df_dup_candidates = { NULL };\n \n-\tif (init_tree_desc_gently(&desc, buffer, size)) {\n+\tif (init_tree_desc_gently(&desc, buffer, size, TREE_DESC_RAW_MODES)) {\n \t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t FSCK_MSG_BAD_TREE,\n \t\t\t\t \"cannot be parsed as a tree\");\ndiff --git a/packfile.c b/packfile.c\nindex 6b0eb9048e..5ae3ce8ea9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2231,7 +2231,7 @@ static int add_promisor_object(const struct object_id *oid,\n \t\tstruct tree *tree = (struct tree *)obj;\n \t\tstruct tree_desc desc;\n \t\tstruct name_entry entry;\n-\t\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n+\t\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size, 0))\n \t\t\t/*\n \t\t\t * Error messages are given when packs are\n \t\t\t * verified, so do not print any here.\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex ab7f31f1dc..d2051e0fda 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -364,6 +364,17 @@ test_expect_success 'tree entry with type mismatch' '\n \ttest_i18ngrep ! \"dangling blob\" out\n '\n \n+test_expect_success 'tree entry with bogus mode' '\n+\ttest_when_finished \"remove_object \\$blob\" &&\n+\ttest_when_finished \"remove_object \\$tree\" &&\n+\tblob=$(echo blob | git hash-object -w --stdin) &&\n+\tblob_oct=$(echo $blob | hex2oct) &&\n+\ttree=$(printf \"100000 foo\\0${blob_oct}\" |\n+\t       git hash-object -t tree --stdin -w --literally) &&\n+\tgit fsck 2>err &&\n+\tgrep \"$tree: badFilemode\" err\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tbadoid=$(test_oid deadbeef) &&\n \tcat >invalid-tag <<-EOF &&\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 506234b4b8..74f4d710e8 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -47,17 +47,20 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l\n \n \t/* Initialize the descriptor entry */\n \tdesc->entry.path = path;\n-\tdesc->entry.mode = canon_mode(mode);\n+\tdesc->entry.mode = (desc->flags & TREE_DESC_RAW_MODES) ? mode : canon_mode(mode);\n \tdesc->entry.pathlen = len - 1;\n \toidread(&desc->entry.oid, (const unsigned char *)path + len);\n \n \treturn 0;\n }\n \n-static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)\n+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer,\n+\t\t\t\t   unsigned long size, struct strbuf *err,\n+\t\t\t\t   enum tree_desc_flags flags)\n {\n \tdesc->buffer = buffer;\n \tdesc->size = size;\n+\tdesc->flags = flags;\n \tif (size)\n \t\treturn decode_tree_entry(desc, buffer, size, err);\n \treturn 0;\n@@ -66,15 +69,16 @@ static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, u\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+\tif (init_tree_desc_internal(desc, buffer, size, &err, 0))\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+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size,\n+\t\t\t  enum tree_desc_flags flags)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tint result = init_tree_desc_internal(desc, buffer, size, &err);\n+\tint result = init_tree_desc_internal(desc, buffer, size, &err, flags);\n \tif (result)\n \t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\ndiff --git a/tree-walk.h b/tree-walk.h\nindex a5058469e9..6305d53150 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -34,6 +34,11 @@ struct tree_desc {\n \n \t/* counts the number of bytes left in the `buffer`. */\n \tunsigned int size;\n+\n+\t/* option flags passed via init_tree_desc_gently() */\n+\tenum tree_desc_flags {\n+\t\tTREE_DESC_RAW_MODES = (1 << 0),\n+\t} flags;\n };\n \n /**\n@@ -79,7 +84,8 @@ int update_tree_entry_gently(struct tree_desc *);\n  */\n void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);\n \n-int init_tree_desc_gently(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+\t\t\t  enum tree_desc_flags flags);\n \n /*\n  * Visit the next entry in a tree. Returns 1 when there are more entries\n"},{"id":"460985","messageId":"6688857F-B899-4253-933B-0DFE25155D13@masklinn.net","threadId":"58273","inReplyTo":"YvLttyyaZWm4pitu@coredump.intra.peff.net","subject":"Re: fsck: BAD_FILEMODE diagnostic broken / never triggers","fromName":"Xavier Morel","fromEmail":"xavier.morel@masklinn.net","sentAt":"2022-08-10T15:01:34Z","receivedAt":"2022-08-10T15:01:44Z","isPatch":false,"sender":{"key":"xavier.morel@masklinn.net","avatar":null},"body":"\n>> We probably need to provide version of decode_tree_entry() which gives\n>> the non-canonical mode, and to call it from fsck.\n\nThat makes sense.\n\n> Perhaps something like the patch below.\n\nI fear I'm really not able to judge so I'll let you fine folks decide on what to do.\n\n> Note that these are treated with a severity of \"warning\", so fsck won't\n> give a non-zero exit.\n\nYeah from what I understand it is classified as a warning already\n(just not emitted), like the zero padded filemodes. And that can be\nupgraded to error using fsck.msgid=error so shouldn't be an issue.\n\n> I think it still enough for transfer.fsckObjects\n> to mark them. I kind of wonder if fixing this at this point might create\n> more problems than it solves though (e.g., if people have broken modes\n> in historical objects that servers may now reject).\n\nMaybe downgrade to info or ignore by default then? It might still be\nan issue for people who wilfully upgraded the diagnostic to error\nhoping to catch the, but hopefully if they did that they'd rather get\nthe notice later than never?\n\nThat's pretty much the `-Werror` problem (or one of them anyway)"},{"id":"461025","messageId":"YvQPQUuC+IxO0uo4@coredump.intra.peff.net","threadId":"58273","inReplyTo":"6688857F-B899-4253-933B-0DFE25155D13@masklinn.net","subject":"Re: fsck: BAD_FILEMODE diagnostic broken / never triggers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T20:04:17Z","receivedAt":"2022-08-10T20:04:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2022 at 05:01:34PM +0200, Xavier Morel wrote:\n\n> > Note that these are treated with a severity of \"warning\", so fsck won't\n> > give a non-zero exit.\n> \n> Yeah from what I understand it is classified as a warning already\n> (just not emitted), like the zero padded filemodes. And that can be\n> upgraded to error using fsck.msgid=error so shouldn't be an issue.\n\nRight.\n\n> > I think it still enough for transfer.fsckObjects\n> > to mark them. I kind of wonder if fixing this at this point might create\n> > more problems than it solves though (e.g., if people have broken modes\n> > in historical objects that servers may now reject).\n> \n> Maybe downgrade to info or ignore by default then? It might still be\n> an issue for people who wilfully upgraded the diagnostic to error\n> hoping to catch the, but hopefully if they did that they'd rather get\n> the notice later than never?\n\nYeah, that may be a sensible resolution. All things being equal I think\n\"warning\" is the right level, but out of caution and the historical\nprecedent, maybe downgrading it to \"info\" is justified.\n\nIt should be easy to work that into the patch I showed earlier.\n\n-Peff\n"},{"id":"461027","messageId":"YvQcNpizy9uOZiAz@coredump.intra.peff.net","threadId":"58273","inReplyTo":"YvQPQUuC+IxO0uo4@coredump.intra.peff.net","subject":"[PATCH 0/3] actually detect bad file modes in fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T20:59:34Z","receivedAt":"2022-08-10T21:01:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2022 at 04:04:17PM -0400, Jeff King wrote:\n\n> > Maybe downgrade to info or ignore by default then? It might still be\n> > an issue for people who wilfully upgraded the diagnostic to error\n> > hoping to catch the, but hopefully if they did that they'd rather get\n> > the notice later than never?\n> \n> Yeah, that may be a sensible resolution. All things being equal I think\n> \"warning\" is the right level, but out of caution and the historical\n> precedent, maybe downgrading it to \"info\" is justified.\n> \n> It should be easy to work that into the patch I showed earlier.\n\nOK, so here are cleaned-up patches to do that.\n\n  [1/3]: tree-walk: add a mechanism for getting non-canonicalized modes\n  [2/3]: fsck: actually detect bad file modes in trees\n  [3/3]: fsck: downgrade tree badFilemode to \"info\"\n\n fsck.c                          |  4 ++--\n fsck.h                          |  2 +-\n packfile.c                      |  2 +-\n t/t1450-fsck.sh                 | 14 ++++++++++++++\n t/t5504-fetch-receive-strict.sh | 17 +++++++++++++++++\n tree-walk.c                     | 14 +++++++++-----\n tree-walk.h                     |  8 +++++++-\n 7 files changed, 51 insertions(+), 10 deletions(-)\n\n-Peff\n"},{"id":"461028","messageId":"YvQcnW0yvwRwUVKg@coredump.intra.peff.net","threadId":"58273","inReplyTo":"YvQcNpizy9uOZiAz@coredump.intra.peff.net","subject":"[PATCH 1/3] tree-walk: add a mechanism for getting non-canonicalized modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T21:01:17Z","receivedAt":"2022-08-10T21:01:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When using init_tree_desc() and tree_entry() to iterate over a tree, we\nalways canonicalize the modes coming out of the tree. This is a good\nthing to prevent bugs or oddities in normal code paths, but it's\ncounter-productive for tools like fsck that want to see the exact\ncontents.\n\nWe can address this by adding an option to avoid the extra\ncanonicalization. A few notes on the implementation:\n\n  - I've attached the new option to the tree_desc struct itself. The\n    actual code change is in decode_tree_entry(), which is in turn\n    called by the public update_tree_entry(), tree_entry(), and\n    init_tree_desc() functions, plus their \"gently\" counterparts.\n\n    By letting it ride along in the struct, we can avoid changing the\n    signature of those functions, which are called many times. Plus it's\n    conceptually simpler: you really want a particular iteration of a\n    tree to be \"raw\" or not, rather than individual calls.\n\n  - We still have to set the new option somewhere. The struct is\n    initialized by init_tree_desc(). I added the new flags field only to\n    the \"gently\" version. That avoids disturbing the much more numerous\n    non-gentle callers, and it makes sense that anybody being careful\n    about looking at raw modes would also be careful about bogus trees\n    (i.e., the caller will be something like fsck in the first place).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fsck.c      |  4 ++--\n packfile.c  |  2 +-\n tree-walk.c | 14 +++++++++-----\n tree-walk.h |  8 +++++++-\n 4 files changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex dd4822ba1b..5acc982a7c 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -308,7 +308,7 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\treturn -1;\n \n \tname = fsck_get_object_name(options, &tree->object.oid);\n-\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n+\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size, 0))\n \t\treturn -1;\n \twhile (tree_entry_gently(&desc, &entry)) {\n \t\tstruct object *obj;\n@@ -578,7 +578,7 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tconst char *o_name;\n \tstruct name_stack df_dup_candidates = { NULL };\n \n-\tif (init_tree_desc_gently(&desc, buffer, size)) {\n+\tif (init_tree_desc_gently(&desc, buffer, size, 0)) {\n \t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t FSCK_MSG_BAD_TREE,\n \t\t\t\t \"cannot be parsed as a tree\");\ndiff --git a/packfile.c b/packfile.c\nindex 6b0eb9048e..5ae3ce8ea9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2231,7 +2231,7 @@ static int add_promisor_object(const struct object_id *oid,\n \t\tstruct tree *tree = (struct tree *)obj;\n \t\tstruct tree_desc desc;\n \t\tstruct name_entry entry;\n-\t\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n+\t\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size, 0))\n \t\t\t/*\n \t\t\t * Error messages are given when packs are\n \t\t\t * verified, so do not print any here.\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 506234b4b8..74f4d710e8 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -47,17 +47,20 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l\n \n \t/* Initialize the descriptor entry */\n \tdesc->entry.path = path;\n-\tdesc->entry.mode = canon_mode(mode);\n+\tdesc->entry.mode = (desc->flags & TREE_DESC_RAW_MODES) ? mode : canon_mode(mode);\n \tdesc->entry.pathlen = len - 1;\n \toidread(&desc->entry.oid, (const unsigned char *)path + len);\n \n \treturn 0;\n }\n \n-static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)\n+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer,\n+\t\t\t\t   unsigned long size, struct strbuf *err,\n+\t\t\t\t   enum tree_desc_flags flags)\n {\n \tdesc->buffer = buffer;\n \tdesc->size = size;\n+\tdesc->flags = flags;\n \tif (size)\n \t\treturn decode_tree_entry(desc, buffer, size, err);\n \treturn 0;\n@@ -66,15 +69,16 @@ static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, u\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+\tif (init_tree_desc_internal(desc, buffer, size, &err, 0))\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+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size,\n+\t\t\t  enum tree_desc_flags flags)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tint result = init_tree_desc_internal(desc, buffer, size, &err);\n+\tint result = init_tree_desc_internal(desc, buffer, size, &err, flags);\n \tif (result)\n \t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\ndiff --git a/tree-walk.h b/tree-walk.h\nindex a5058469e9..6305d53150 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -34,6 +34,11 @@ struct tree_desc {\n \n \t/* counts the number of bytes left in the `buffer`. */\n \tunsigned int size;\n+\n+\t/* option flags passed via init_tree_desc_gently() */\n+\tenum tree_desc_flags {\n+\t\tTREE_DESC_RAW_MODES = (1 << 0),\n+\t} flags;\n };\n \n /**\n@@ -79,7 +84,8 @@ int update_tree_entry_gently(struct tree_desc *);\n  */\n void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);\n \n-int init_tree_desc_gently(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+\t\t\t  enum tree_desc_flags flags);\n \n /*\n  * Visit the next entry in a tree. Returns 1 when there are more entries\n-- \n2.37.1.917.gf0c714241f\n\n"},{"id":"461029","messageId":"YvQc9egGzwFomQbw@coredump.intra.peff.net","threadId":"58273","inReplyTo":"YvQcNpizy9uOZiAz@coredump.intra.peff.net","subject":"[PATCH 2/3] fsck: actually detect bad file modes in trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T21:02:45Z","receivedAt":"2022-08-10T21:02:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use the normal tree_desc code to iterate over trees in fsck, meaning\nwe only see the canonicalized modes it returns. And hence we'd never see\nanything unexpected, since it will coerce literally any garbage into one\nof our normal and accepted modes.\n\nWe can use the new RAW_MODES flag to see the real modes, and then use\nthe existing code to actually analyze them. The existing code is written\nas allow-known-good, so there's not much point in testing a variety of\nbreakages. The one tested here should be S_IFREG but with nonsense\npermissions.\n\nDo note that the error-reporting here isn't great. We don't mention the\nspecific bad mode, but just that the tree has one or more broken modes.\nBut when you go to look at it with \"git ls-tree\", we'll report the\ncanonicalized mode! This isn't ideal, but given that this should come up\nrarely, and that any number of other tree corruptions might force you\ninto looking at the binary bytes via \"cat-file\", it's not the end of the\nworld. And it's something we can improve on top later if we choose.\n\nReported-by: Xavier Morel <xavier.morel@masklinn.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fsck.c          |  2 +-\n t/t1450-fsck.sh | 14 ++++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 5acc982a7c..b3da1d68c0 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -578,7 +578,7 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tconst char *o_name;\n \tstruct name_stack df_dup_candidates = { NULL };\n \n-\tif (init_tree_desc_gently(&desc, buffer, size, 0)) {\n+\tif (init_tree_desc_gently(&desc, buffer, size, TREE_DESC_RAW_MODES)) {\n \t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t FSCK_MSG_BAD_TREE,\n \t\t\t\t \"cannot be parsed as a tree\");\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex ab7f31f1dc..53c2aa10b7 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -364,6 +364,20 @@ test_expect_success 'tree entry with type mismatch' '\n \ttest_i18ngrep ! \"dangling blob\" out\n '\n \n+test_expect_success 'tree entry with bogus mode' '\n+\ttest_when_finished \"remove_object \\$blob\" &&\n+\ttest_when_finished \"remove_object \\$tree\" &&\n+\tblob=$(echo blob | git hash-object -w --stdin) &&\n+\tblob_oct=$(echo $blob | hex2oct) &&\n+\ttree=$(printf \"100000 foo\\0${blob_oct}\" |\n+\t       git hash-object -t tree --stdin -w --literally) &&\n+\tgit fsck 2>err &&\n+\tcat >expect <<-EOF &&\n+\twarning in tree $tree: badFilemode: contains bad file modes\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tbadoid=$(test_oid deadbeef) &&\n \tcat >invalid-tag <<-EOF &&\n-- \n2.37.1.917.gf0c714241f\n\n"},{"id":"461030","messageId":"YvQdR3sDqDMCIjIE@coredump.intra.peff.net","threadId":"58273","inReplyTo":"YvQcNpizy9uOZiAz@coredump.intra.peff.net","subject":"[PATCH 3/3] fsck: downgrade tree badFilemode to \"info\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T21:04:07Z","receivedAt":"2022-08-10T21:04:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit un-broke the \"badFileMode\" check; before then it was\nliterally testing nothing. And as far as I can tell, it has been so\nsince the very initial version of fsck.\n\nThe current severity of \"badFileMode\" is just \"warning\". But in the\n--strict mode used by transfer.fsckObjects, that is elevated to an\nerror. This will potentially cause hassle for users, because historical\nobjects with bad modes will suddenly start causing pushes to many server\noperators to be rejected.\n\nAt the same time, these bogus modes aren't actually a big risk. Because\nwe canonicalize them everywhere besides fsck, they can't cause too much\nmischief in the real world. The worst thing you can do is end up with\ntwo almost-identical trees that have different hashes but are\ninterpreted the same. That will generally cause things to be inefficient\nrather than wrong, and is a bug somebody working on a Git implementation\nwould want to fix, but probably not worth inconveniencing users by\nrefusing to push or fetch.\n\nSo let's downgrade this to \"info\" by default, which is our setting for\n\"mention this when fscking, but don't ever reject, even under strict\nmode\". If somebody really wants to be paranoid, they can still adjust\nthe level using config.\n\nSuggested-by: Xavier Morel <xavier.morel@masklinn.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fsck.h                          |  2 +-\n t/t5504-fetch-receive-strict.sh | 17 +++++++++++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.h b/fsck.h\nindex d07f7a2459..6f801e53b1 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -56,7 +56,6 @@ enum fsck_msg_type {\n \tFUNC(GITMODULES_PATH, ERROR) \\\n \tFUNC(GITMODULES_UPDATE, ERROR) \\\n \t/* warnings */ \\\n-\tFUNC(BAD_FILEMODE, WARN) \\\n \tFUNC(EMPTY_NAME, WARN) \\\n \tFUNC(FULL_PATHNAME, WARN) \\\n \tFUNC(HAS_DOT, WARN) \\\n@@ -66,6 +65,7 @@ enum fsck_msg_type {\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n+\tFUNC(BAD_FILEMODE, INFO) \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(GITIGNORE_SYMLINK, INFO) \\\n \tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex b0b795aca9..ac4099ca89 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -352,4 +352,21 @@ test_expect_success \\\n \tgrep \"Cannot demote unterminatedheader\" act\n '\n \n+test_expect_success 'badFilemode is not a strict error' '\n+\tgit init --bare badmode.git &&\n+\ttree=$(\n+\t\tcd badmode.git &&\n+\t\tblob=$(echo blob | git hash-object -w --stdin | hex2oct) &&\n+\t\tprintf \"123456 foo\\0${blob}\" |\n+\t\tgit hash-object -t tree --stdin -w --literally\n+\t) &&\n+\n+\trm -rf dst.git &&\n+\tgit init --bare dst.git &&\n+\tgit -C dst.git config transfer.fsckObjects true &&\n+\n+\tgit -C badmode.git push ../dst.git $tree:refs/tags/tree 2>err &&\n+\tgrep \"$tree: badFilemode\" err\n+'\n+\n test_done\n-- \n2.37.1.917.gf0c714241f\n"},{"id":"461037","messageId":"xmqq4jyjkcij.fsf@gitster.g","threadId":"58273","inReplyTo":"YvQc9egGzwFomQbw@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] fsck: actually detect bad file modes in trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T21:35:32Z","receivedAt":"2022-08-10T21:35:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We use the normal tree_desc code to iterate over trees in fsck, meaning\n> we only see the canonicalized modes it returns. And hence we'd never see\n> anything unexpected, since it will coerce literally any garbage into one\n> of our normal and accepted modes.\n\nWow.  I did know canon_mode() deliberately discarding the extra\npermission bits on trees and blobs, but it was that bad to mark\nwhatever it does not understand as a gitlink.  That is simply\nhorrible.\n\n> -\tif (init_tree_desc_gently(&desc, buffer, size, 0)) {\n> +\tif (init_tree_desc_gently(&desc, buffer, size, TREE_DESC_RAW_MODES)) {\n>  \t\tretval += report(options, tree_oid, OBJ_TREE,\n>  \t\t\t\t FSCK_MSG_BAD_TREE,\n>  \t\t\t\t \"cannot be parsed as a tree\");\n\nOK, so we'll let desc.entry.mode carry whatever bogus bit pattern we\ngot out of buffer and the downstream code already knows what to do\nwith them.  That's a clean and minimum way to do this.\n\nThanks.\n\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index ab7f31f1dc..53c2aa10b7 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -364,6 +364,20 @@ test_expect_success 'tree entry with type mismatch' '\n>  \ttest_i18ngrep ! \"dangling blob\" out\n>  '\n>  \n> +test_expect_success 'tree entry with bogus mode' '\n> +\ttest_when_finished \"remove_object \\$blob\" &&\n> +\ttest_when_finished \"remove_object \\$tree\" &&\n> +\tblob=$(echo blob | git hash-object -w --stdin) &&\n> +\tblob_oct=$(echo $blob | hex2oct) &&\n> +\ttree=$(printf \"100000 foo\\0${blob_oct}\" |\n> +\t       git hash-object -t tree --stdin -w --literally) &&\n> +\tgit fsck 2>err &&\n> +\tcat >expect <<-EOF &&\n> +\twarning in tree $tree: badFilemode: contains bad file modes\n> +\tEOF\n> +\ttest_cmp expect err\n> +'\n> +\n>  test_expect_success 'tag pointing to nonexistent' '\n>  \tbadoid=$(test_oid deadbeef) &&\n>  \tcat >invalid-tag <<-EOF &&\n"},{"id":"461039","messageId":"YvQnIuVzMrOaM589@coredump.intra.peff.net","threadId":"58273","inReplyTo":"xmqq4jyjkcij.fsf@gitster.g","subject":"Re: [PATCH 2/3] fsck: actually detect bad file modes in trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T21:46:10Z","receivedAt":"2022-08-10T21:46:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2022 at 02:35:32PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > We use the normal tree_desc code to iterate over trees in fsck, meaning\n> > we only see the canonicalized modes it returns. And hence we'd never see\n> > anything unexpected, since it will coerce literally any garbage into one\n> > of our normal and accepted modes.\n> \n> Wow.  I did know canon_mode() deliberately discarding the extra\n> permission bits on trees and blobs, but it was that bad to mark\n> whatever it does not understand as a gitlink.  That is simply\n> horrible.\n\nYeah, I noticed that, as well. It might actually be reasonable to put a\ndie() at the end of that canon_mode() function. It's rather unfriendly,\nbut then, treating nonsense as a gitlink is likely to just result in the\ncaller ending up aborting anyway.\n\nOutside the scope of my patch, though. :)\n\n-Peff\n"},{"id":"461040","messageId":"xmqqsfm3ix20.fsf@gitster.g","threadId":"58273","inReplyTo":"YvQnIuVzMrOaM589@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] fsck: actually detect bad file modes in trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T21:54:47Z","receivedAt":"2022-08-10T21:54:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Outside the scope of my patch, though. :)\n\nAbsolutely.\n"},{"id":"461041","messageId":"xmqqo7wriwfl.fsf@gitster.g","threadId":"58273","inReplyTo":"YvQdR3sDqDMCIjIE@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] fsck: downgrade tree badFilemode to \"info\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T22:08:14Z","receivedAt":"2022-08-10T22:08:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... That will generally cause things to be inefficient\n> rather than wrong, and is a bug somebody working on a Git implementation\n> would want to fix, but probably not worth inconveniencing users by\n> refusing to push or fetch.\n>\n> So let's downgrade this to \"info\" by default, which is our setting for\n> \"mention this when fscking, but don't ever reject, even under strict\n> mode\". If somebody really wants to be paranoid, they can still adjust\n> the level using config.\n\nIIRC, I heard that every reimplementation of Git got the tree entry\nwrong one way or another at least once.  I think this is prudent.\n\nI was almost sure that before we \"unified\" the codepath for normal\ntree reading and the one used for fsck in a mistaken way, which was\nfixed in this series, we were catching these anomalous mode bits,\nbut the suspected regression is too long ago that it does not make a\npractical difference if it was always broken or it was broken long\ntime ago.  The risk to start complaining on existing projects is the\nsame either way.\n\nThanks for working on this series.\n"},{"id":"461061","messageId":"YvS+x35sxXmO5H1h@coredump.intra.peff.net","threadId":"58273","inReplyTo":"xmqqo7wriwfl.fsf@gitster.g","subject":"Re: [PATCH 3/3] fsck: downgrade tree badFilemode to \"info\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-11T08:33:11Z","receivedAt":"2022-08-11T08:33:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2022 at 03:08:14PM -0700, Junio C Hamano wrote:\n\n> I was almost sure that before we \"unified\" the codepath for normal\n> tree reading and the one used for fsck in a mistaken way, which was\n> fixed in this series, we were catching these anomalous mode bits,\n> but the suspected regression is too long ago that it does not make a\n> practical difference if it was always broken or it was broken long\n> time ago.  The risk to start complaining on existing projects is the\n> same either way.\n\nI agree with the \"it was so long ago it does not matter\", but for the\nsake of posterity, here's what my digging found:\n\n  - we got the mode fsck checks in 64071805ed (git-fsck-cache: be\n    stricter about \"tree\" objects, 2005-07-27), though there is a proto\n    version that is even a little older. Back then we were using a\n    linked list to hold the parsed tree entries (!), but it was parsed\n    by a central spot in parse_tree_buffer().\n\n  - that linked list code went away in 15b5536ee4 (Remove last vestiges\n    of generic tree_entry_list, 2006-05-29). But...\n\n  - ...by then we already had 1b0c7174a1 (tree/diff header cleanup.,\n    2006-03-29), which had tree_entry_extract(). And that commit\n    introduced canon_mode, including the same \"set unexpected things to\n    a default\", though of course back then it waas S_IFDIR since\n    gitlinks didn't exist. ;)\n\n  - that canon_mode() was just a rename from DIFF_FILE_CANON_MODE(),\n    which ultimately came from 67574c403f ([PATCH] diff: mode bits\n    fixes, 2005-06-01)\n\nSo some form of canon_mode() does predate the fsck checks, but I _think_\nthe fsck code was using the old linked-list version until 15b5536ee4,\nand would not have been affected. So yes, there were probably 10 months\nin 2005-2006 where we would have detected these. :)\n\nAgain, probably not important, but it was interesting for me at least to\nsee the evolution of the tree code. Most of those changes predate my\ninvolvement with the code.\n\n-Peff\n"},{"id":"461066","messageId":"220811.86r11nqfi2.gmgdl@evledraar.gmail.com","threadId":"58273","inReplyTo":"YvQcNpizy9uOZiAz@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] actually detect bad file modes in fsck","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-11T09:39:42Z","receivedAt":"2022-08-11T09:46:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 10 2022, Jeff King wrote:\n\n> On Wed, Aug 10, 2022 at 04:04:17PM -0400, Jeff King wrote:\n>\n>> > Maybe downgrade to info or ignore by default then? It might still be\n>> > an issue for people who wilfully upgraded the diagnostic to error\n>> > hoping to catch the, but hopefully if they did that they'd rather get\n>> > the notice later than never?\n>> \n>> Yeah, that may be a sensible resolution. All things being equal I think\n>> \"warning\" is the right level, but out of caution and the historical\n>> precedent, maybe downgrading it to \"info\" is justified.\n>> \n>> It should be easy to work that into the patch I showed earlier.\n>\n> OK, so here are cleaned-up patches to do that.\n>\n>   [1/3]: tree-walk: add a mechanism for getting non-canonicalized modes\n>   [2/3]: fsck: actually detect bad file modes in trees\n>   [3/3]: fsck: downgrade tree badFilemode to \"info\"\n\nThis LGTM.\n\nI noticed/reported this issue more than a year ago, but the series I had\nfor fixing it ended up being dropped, here's the report/analysis at the\ntime:\nhttps://lore.kernel.org/git/20210308150650.18626-31-avarab@gmail.com/\n\nBasically I was taking a much longer way around to avoid...\n\n>  \t/* counts the number of bytes left in the `buffer`. */\n>  \tunsigned int size;\n> +\n> +\t/* option flags passed via init_tree_desc_gently() */\n> +\tenum tree_desc_flags {\n> +\t\tTREE_DESC_RAW_MODES = (1 << 0),\n> +\t} flags;\n>  };\n\n...this from 1/3 here, i.e. now we're paying the cost of an extra entry\nin every \"struct tree_desc\" user (which includes some hot codepaths),\nand just for this one fsck caller.\n\nI wonder if we couldn't introduce a init_tree_desc_gently_flags() for\nthis instead. You note in 1/3 that you're (rightly) avoiding the churn\nof existing callers, but they could just use a \"static inline\" wrapper\nfunction that would set those flags to 0, we'd pass the flags down, and\nnot put them into the \"tree_desc\" struct.\n\nArguably it doesn't belong there at all, since this new thing is really\nan \"opts\" struct, but \"tree_desc\" is for \"the state of the walk\".\n\nIt might/would make sense as a \"raw_mode\" in \"struct name_entry\"\nperhaps, but then we're gettin closer to the larger scope of my initial\nlarger series, oh well ... :)\n\n"},{"id":"461084","messageId":"xmqqh72ihhnw.fsf@gitster.g","threadId":"58273","inReplyTo":"YvS+x35sxXmO5H1h@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] fsck: downgrade tree badFilemode to \"info\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-11T16:24:51Z","receivedAt":"2022-08-11T16:51:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I agree with the \"it was so long ago it does not matter\", but for the\n> sake of posterity, here's what my digging found:\n> ...\n\nThat's indeed an ancient history.  Thanks for a funny find.  I\nforgot all about the linked list of tree entries.\n\n> Again, probably not important, but it was interesting for me at least to\n> see the evolution of the tree code. Most of those changes predate my\n> involvement with the code.\n\nYeah, your name didn't appear in the history leading to the version\nwith linked list of tree entries, but by the time it was converted\nto more modern \"update_tree_entry()\" interface, you had a handful of\ncommits already.\n\nThe main codepath never cared about zero-padded octal (we used to\nuse sscanf \"%o\", and these days get_mode() does it manually), but\nthere used to be Git implementations that zero-padded the mode\n(e.g. \"040000\") that we need to warn about.  Both in code before and\nafter 15b5536ee4, we peek into the bytes directly and check '0',\nindependent from parsing the octal string into 'mode'.  I found that\ninteresting (no, there is nothing to fix, just a fun read).\n\nThanks.\n\n\n"},{"id":"461187","messageId":"YvieR3Jy9d0JB9IB@coredump.intra.peff.net","threadId":"58273","inReplyTo":"220811.86r11nqfi2.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/3] actually detect bad file modes in fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-14T07:03:35Z","receivedAt":"2022-08-14T07:03:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 11, 2022 at 11:39:42AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > OK, so here are cleaned-up patches to do that.\n> >\n> >   [1/3]: tree-walk: add a mechanism for getting non-canonicalized modes\n> >   [2/3]: fsck: actually detect bad file modes in trees\n> >   [3/3]: fsck: downgrade tree badFilemode to \"info\"\n> \n> This LGTM.\n> \n> I noticed/reported this issue more than a year ago, but the series I had\n> for fixing it ended up being dropped, here's the report/analysis at the\n> time:\n> https://lore.kernel.org/git/20210308150650.18626-31-avarab@gmail.com/\n> \n> Basically I was taking a much longer way around to avoid...\n\nI took a look at that patch, but I'd really prefer _not_ to lose the\nauto-canonicalizing for most code paths. It's an easy thing to forget\nabout, and the current state protects most code from getting confused by\nmalicious or buggy modes.\n\n> >  \t/* counts the number of bytes left in the `buffer`. */\n> >  \tunsigned int size;\n> > +\n> > +\t/* option flags passed via init_tree_desc_gently() */\n> > +\tenum tree_desc_flags {\n> > +\t\tTREE_DESC_RAW_MODES = (1 << 0),\n> > +\t} flags;\n> >  };\n> \n> ...this from 1/3 here, i.e. now we're paying the cost of an extra entry\n> in every \"struct tree_desc\" user (which includes some hot codepaths),\n> and just for this one fsck caller.\n\nYes, but I don't think tree_desc itself is very hot. We allocate one per\niteration on the stack, not one per tree. So you'd have at most N at\nonce for a tree with depth N. And the rest of tree_desc is...already not\nvery lightweight.\n\nIn fact, I think this flag ends up in what is currently padding (I get\n72 bytes for the struct before and after). Though in the long run that\n\"unsigned int\" should almost certainly become a \"size_t\", which would\nfill out that padding. I still doubt it's measurable.\n\n> I wonder if we couldn't introduce a init_tree_desc_gently_flags() for\n> this instead. You note in 1/3 that you're (rightly) avoiding the churn\n> of existing callers, but they could just use a \"static inline\" wrapper\n> function that would set those flags to 0, we'd pass the flags down, and\n> not put them into the \"tree_desc\" struct.\n\nYou'd need not just that function, but wrappers for the iterator\nfunctions. I agree it could work. It just seemed less clean to me.\n\n> Arguably it doesn't belong there at all, since this new thing is really\n> an \"opts\" struct, but \"tree_desc\" is for \"the state of the walk\".\n\nI think that's a philosophical difference, and not one I really agree\nwith. I'd argue that options are part of the state (and we mingle them\nalready in other structs like rev_info).\n\n> It might/would make sense as a \"raw_mode\" in \"struct name_entry\"\n> perhaps, but then we're gettin closer to the larger scope of my initial\n> larger series, oh well ... :)\n\nYes, I'd be OK with that approach, too. But aren't we now similarly\nbloating things for a value that most callers won't care about? ;)\n\n-Peff\n"}]}