git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: fsck: BAD_FILEMODE diagnostic broken / never triggers

From
Jeff King <peff@peff.net>
Date
Aug 9, 2022, 23:28 UTC
Message-ID
<YvLttyyaZWm4pitu@coredump.intra.peff.net>
In-Reply-To
<YvLkU3X9lBsG8wXp@coredump.intra.peff.net>
On Tue, Aug 09, 2022 at 06:48:51PM -0400, Jeff King wrote:
Show 15 quoted lines
> On Sat, Aug 06, 2022 at 03:43:56PM +0200, Xavier Morel wrote:
> 
> > However looking at fsck_tree, it does have a fair amount of code to
> > validate entry modes and a dedicated message id for this (BAD_FILEMODE
> > / badFilemode), it even has a code path for legacy entries with
> > S_IWGRP set (extensively documented under `git fsck --strict`).
> > 
> > I guess, over time mode canonicalisation has slowly creeped earlier
> > the tree-parsing code, and (seemingly for several years) it has been
> > occurring before "git fsck" gets tree entry information, so git fsck
> > simply can not see invalid entry modes?
> 
> Yes, I think you're right. I didn't bisect, but I suspect this goes back
> to 7146e66f08 (tree-walk: finally switch over tree descriptors to
> contain a pre-parsed entry, 2014-02-06).

Hmm, actually, I think this may have been broken since the beginning of fsck. At least I could not find a version for which the test below behaves as expected.

> We probably need to provide version of decode_tree_entry() which gives
> the non-canonical mode, and to call it from fsck.
Perhaps something like the patch below.

Note that these are treated with a severity of "warning", so fsck won't give a non-zero exit. I think it still enough for transfer.fsckObjects to mark them. I kind of wonder if fixing this at this point might create more problems than it solves though (e.g., if people have broken modes in historical objects that servers may now reject).

---
diff --git a/fsck.c b/fsck.c
index dd4822ba1b..b3da1d68c0 100644
--- a/fsck.c
+++ b/fsck.c
@@ -308,7 +308,7 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
 		return -1;
 
 	name = fsck_get_object_name(options, &tree->object.oid);
-	if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
+	if (init_tree_desc_gently(&desc, tree->buffer, tree->size, 0))
 		return -1;
 	while (tree_entry_gently(&desc, &entry)) {
 		struct object *obj;
@@ -578,7 +578,7 @@ static int fsck_tree(const struct object_id *tree_oid,
 	const char *o_name;
 	struct name_stack df_dup_candidates = { NULL };
 
-	if (init_tree_desc_gently(&desc, buffer, size)) {
+	if (init_tree_desc_gently(&desc, buffer, size, TREE_DESC_RAW_MODES)) {
 		retval += report(options, tree_oid, OBJ_TREE,
 				 FSCK_MSG_BAD_TREE,
 				 "cannot be parsed as a tree");
diff --git a/packfile.c b/packfile.c
index 6b0eb9048e..5ae3ce8ea9 100644
--- a/packfile.c
+++ b/packfile.c
@@ -2231,7 +2231,7 @@ static int add_promisor_object(const struct object_id *oid,
 		struct tree *tree = (struct tree *)obj;
 		struct tree_desc desc;
 		struct name_entry entry;
-		if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
+		if (init_tree_desc_gently(&desc, tree->buffer, tree->size, 0))
 			/*
 			 * Error messages are given when packs are
 			 * verified, so do not print any here.
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index ab7f31f1dc..d2051e0fda 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -364,6 +364,17 @@ test_expect_success 'tree entry with type mismatch' '
 	test_i18ngrep ! "dangling blob" out
 '
 
+test_expect_success 'tree entry with bogus mode' '
+	test_when_finished "remove_object \$blob" &&
+	test_when_finished "remove_object \$tree" &&
+	blob=$(echo blob | git hash-object -w --stdin) &&
+	blob_oct=$(echo $blob | hex2oct) &&
+	tree=$(printf "100000 foo\0${blob_oct}" |
+	       git hash-object -t tree --stdin -w --literally) &&
+	git fsck 2>err &&
+	grep "$tree: badFilemode" err
+'
+
 test_expect_success 'tag pointing to nonexistent' '
 	badoid=$(test_oid deadbeef) &&
 	cat >invalid-tag <<-EOF &&
diff --git a/tree-walk.c b/tree-walk.c
index 506234b4b8..74f4d710e8 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -47,17 +47,20 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
 
 	/* Initialize the descriptor entry */
 	desc->entry.path = path;
-	desc->entry.mode = canon_mode(mode);
+	desc->entry.mode = (desc->flags & TREE_DESC_RAW_MODES) ? mode : canon_mode(mode);
 	desc->entry.pathlen = len - 1;
 	oidread(&desc->entry.oid, (const unsigned char *)path + len);
 
 	return 0;
 }
 
-static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)
+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer,
+				   unsigned long size, struct strbuf *err,
+				   enum tree_desc_flags flags)
 {
 	desc->buffer = buffer;
 	desc->size = size;
+	desc->flags = flags;
 	if (size)
 		return decode_tree_entry(desc, buffer, size, err);
 	return 0;
@@ -66,15 +69,16 @@ static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, u
 void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
 {
 	struct strbuf err = STRBUF_INIT;
-	if (init_tree_desc_internal(desc, buffer, size, &err))
+	if (init_tree_desc_internal(desc, buffer, size, &err, 0))
 		die("%s", err.buf);
 	strbuf_release(&err);
 }
 
-int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)
+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size,
+			  enum tree_desc_flags flags)
 {
 	struct strbuf err = STRBUF_INIT;
-	int result = init_tree_desc_internal(desc, buffer, size, &err);
+	int result = init_tree_desc_internal(desc, buffer, size, &err, flags);
 	if (result)
 		error("%s", err.buf);
 	strbuf_release(&err);
diff --git a/tree-walk.h b/tree-walk.h
index a5058469e9..6305d53150 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -34,6 +34,11 @@ struct tree_desc {
 
 	/* counts the number of bytes left in the `buffer`. */
 	unsigned int size;
+
+	/* option flags passed via init_tree_desc_gently() */
+	enum tree_desc_flags {
+		TREE_DESC_RAW_MODES = (1 << 0),
+	} flags;
 };
 
 /**
@@ -79,7 +84,8 @@ int update_tree_entry_gently(struct tree_desc *);
  */
 void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);
 
-int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);
+int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size,
+			  enum tree_desc_flags flags);
 
 /*
  * Visit the next entry in a tree. Returns 1 when there are more entries
Previous: Jeff KingNext: Xavier Morel
Message 3 of 17 in “fsck: BAD_FILEMODE diagnostic broken / never triggers”
  1. Xavier MorelAug 6, 2022
  2. Jeff KingAug 9, 2022
  3. Jeff KingAug 9, 2022
  4. Xavier MorelAug 10, 2022
  5. Jeff KingAug 10, 2022
  6. 0/3 actually detect bad file modes in fsckJeff King, Aug 10, 2022
  7. 1/3 tree-walk: add a mechanism for getting non-canonicalized modesJeff King, Aug 10, 2022
  8. 2/3 fsck: actually detect bad file modes in treesJeff King, Aug 10, 2022
  9. Junio C HamanoAug 10, 2022
  10. Jeff KingAug 10, 2022
  11. Junio C HamanoAug 10, 2022
  12. 3/3 fsck: downgrade tree badFilemode to "info"Jeff King, Aug 10, 2022
  13. Junio C HamanoAug 10, 2022
  14. Jeff KingAug 11, 2022
  15. Junio C HamanoAug 11, 2022
  16. Ævar Arnfjörð BjarmasonAug 11, 2022
  17. Jeff KingAug 14, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.