From 53602a937dc9eacd67b6afcd781f7b15bb02682f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 10 Aug 2022 17:02:45 -0400 Subject: fsck: actually detect bad file modes in trees We use the normal tree_desc code to iterate over trees in fsck, meaning we only see the canonicalized modes it returns. And hence we'd never see anything unexpected, since it will coerce literally any garbage into one of our normal and accepted modes. We can use the new RAW_MODES flag to see the real modes, and then use the existing code to actually analyze them. The existing code is written as allow-known-good, so there's not much point in testing a variety of breakages. The one tested here should be S_IFREG but with nonsense permissions. Do note that the error-reporting here isn't great. We don't mention the specific bad mode, but just that the tree has one or more broken modes. But when you go to look at it with "git ls-tree", we'll report the canonicalized mode! This isn't ideal, but given that this should come up rarely, and that any number of other tree corruptions might force you into looking at the binary bytes via "cat-file", it's not the end of the world. And it's something we can improve on top later if we choose. Reported-by: Xavier Morel Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1450-fsck.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 't') diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index ab7f31f1dc..53c2aa10b7 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -364,6 +364,20 @@ 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 && + cat >expect <<-EOF && + warning in tree $tree: badFilemode: contains bad file modes + EOF + test_cmp expect err +' + test_expect_success 'tag pointing to nonexistent' ' badoid=$(test_oid deadbeef) && cat >invalid-tag <<-EOF && -- cgit v1.2.3 From 4dd3b045f528b8d9cbbb4a50e371affb0543f37d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 10 Aug 2022 17:04:07 -0400 Subject: fsck: downgrade tree badFilemode to "info" The previous commit un-broke the "badFileMode" check; before then it was literally testing nothing. And as far as I can tell, it has been so since the very initial version of fsck. The current severity of "badFileMode" is just "warning". But in the --strict mode used by transfer.fsckObjects, that is elevated to an error. This will potentially cause hassle for users, because historical objects with bad modes will suddenly start causing pushes to many server operators to be rejected. At the same time, these bogus modes aren't actually a big risk. Because we canonicalize them everywhere besides fsck, they can't cause too much mischief in the real world. The worst thing you can do is end up with two almost-identical trees that have different hashes but are interpreted the same. That will generally cause things to be inefficient rather than wrong, and is a bug somebody working on a Git implementation would want to fix, but probably not worth inconveniencing users by refusing to push or fetch. So let's downgrade this to "info" by default, which is our setting for "mention this when fscking, but don't ever reject, even under strict mode". If somebody really wants to be paranoid, they can still adjust the level using config. Suggested-by: Xavier Morel Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5504-fetch-receive-strict.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 't') diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh index b0b795aca9..ac4099ca89 100755 --- a/t/t5504-fetch-receive-strict.sh +++ b/t/t5504-fetch-receive-strict.sh @@ -352,4 +352,21 @@ test_expect_success \ grep "Cannot demote unterminatedheader" act ' +test_expect_success 'badFilemode is not a strict error' ' + git init --bare badmode.git && + tree=$( + cd badmode.git && + blob=$(echo blob | git hash-object -w --stdin | hex2oct) && + printf "123456 foo\0${blob}" | + git hash-object -t tree --stdin -w --literally + ) && + + rm -rf dst.git && + git init --bare dst.git && + git -C dst.git config transfer.fsckObjects true && + + git -C badmode.git push ../dst.git $tree:refs/tags/tree 2>err && + grep "$tree: badFilemode" err +' + test_done -- cgit v1.2.3