{"thread":{"id":"59115","subject":"[RFC/PATCH 0/6] hash-object: use fsck to check objects","startedAt":"2023-01-18T20:36:05Z","lastAt":"2023-02-01T20:42:07Z","messageCount":28,"participants":["Jeff King","Junio C Hamano","Taylor Blau","René Scharfe","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"470658","messageId":"Y8hX+pIZUKXsyYj5@coredump.intra.peff.net","threadId":"59115","inReplyTo":null,"subject":"[RFC/PATCH 0/6] hash-object: use fsck to check objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-18T20:35:06Z","receivedAt":"2023-01-18T20:36:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Right now \"git hash-object\" will do some basic sanity checks of the\ninput using the usual parser code. This series teaches it to use the\nfsck code instead, which should catch more things. See patch 6 for some\ndiscussion of the implications.\n\nThe reason this is marked as an RFC is that at the end, compiling with\nSANITIZE=address will provoke a failure in t3800. The issue is that\nfsck_tag_standalone(), when fed a buffer/size combo, will look for a NUL\nat the end of the headers, which might be buffer[size]. This is usually\nOK for objects we've loaded from the odb, because we intentionally stick\nan extra NUL at the end for safety. But here index_mem() may get an\narbitrary buffer.\n\nI'm not sure yet of the right path forward. It's not too hard to add an\nextra NUL in most cases, but one code path will mmap a file on disk. And\nsticking a NUL there is hard (we already went down that road trying to\navoid REG_STARTEND for grep, and there wasn't a good solution).\n\nThe other option is having the fsck code avoid looking past the size it\nwas given. I think the intent is that this should work, from commits\nlike 4d0d89755e (Make sure fsck_commit_buffer() does not run out of the\nbuffer, 2014-09-11). We do use skip_prefix() and parse_oid_hex(), which\nwon't respect the size, but I think[1] that's OK because we'll have\nparsed up to the end-of-header beforehand (and those functions would\nnever match past there).\n\nWhich would mean that 9a1a3a4d4c (mktag: allow omitting the header/body\n\\n separator, 2021-01-05) and acf9de4c94 (mktag: use fsck instead of\ncustom verify_tag(), 2021-01-05) were buggy, and we can just fix them.\n\n[1] But I said \"I think\" above because it can get pretty subtle. There's\n some more discussion in this thread:\n\n https://lore.kernel.org/git/20150625155128.C3E9738005C@gemini.denx.de/\n\n but I haven't yet convinced myself it's safe. This is exactly the\n kind of analysis I wish I had the power to nerd-snipe René into.\n\nAnyway, here are the patches in the meantime. I do think this is a good\ndirection overall, modulo addressing the NUL-terminator question.\n\n [1/6]: t1007: modernize malformed object tests\n [2/6]: t1006: stop using 0-padded timestamps\n [3/6]: t7030: stop using invalid tag name\n [4/6]: t: use hash-object --literally when created malformed objects\n [5/6]: fsck: provide a function to fsck buffer without object struct\n [6/6]: hash-object: use fsck for object checks\n\n fsck.c | 29 ++++++++++-------\n fsck.h | 8 +++++\n object-file.c | 55 +++++++++++++-------------------\n t/t1006-cat-file.sh | 6 ++--\n t/t1007-hash-object.sh | 29 +++++++++++------\n t/t1450-fsck.sh | 28 ++++++++--------\n t/t4054-diff-bogus-tree.sh | 2 +-\n t/t4058-diff-duplicates.sh | 2 +-\n t/t4212-log-corrupt.sh | 4 +--\n t/t5302-pack-index.sh | 2 +-\n t/t5504-fetch-receive-strict.sh | 2 +-\n t/t5702-protocol-v2.sh | 2 +-\n t/t6300-for-each-ref.sh | 2 +-\n t/t7030-verify-tag.sh | 2 +-\n t/t7031-verify-tag-signed-ssh.sh | 2 +-\n t/t7509-commit-authorship.sh | 2 +-\n t/t7510-signed-commit.sh | 2 +-\n t/t7528-signed-commit-ssh.sh | 2 +-\n t/t8003-blame-corner-cases.sh | 2 +-\n t/t9350-fast-export.sh | 2 +-\n 20 files changed, 101 insertions(+), 84 deletions(-)\n\n-Peff\n"},{"id":"470659","messageId":"Y8hYEgMze3bY44/0@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hX+pIZUKXsyYj5@coredump.intra.peff.net","subject":"[PATCH 1/6] t1007: modernize malformed object tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-18T20:35:30Z","receivedAt":"2023-01-18T20:36:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The tests in t1007 for detecting malformed objects have two\nanachronisms:\n\n - they use \"sha1\" instead of \"oid\" in variable names, even though the\n script as a whole has been adapted to handle sha256\n\n - they use test_i18ngrep, which is no longer necessary\n\nSince we'll be adding a new similar test, let's clean these up so they\nare all consistently using the modern style.\n\nSigned-off-by: Jeff King \n---\n t/t1007-hash-object.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex ac5ad8c740..2d2148d8fa 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -203,23 +203,23 @@ done\n test_expect_success 'too-short tree' '\n \techo abc >malformed-tree &&\n \ttest_must_fail git hash-object -t tree malformed-tree 2>err &&\n-\ttest_i18ngrep \"too-short tree object\" err\n+\tgrep \"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 | hex2oct) &&\n-\tprintf \"9100644 \\0$bin_sha1\" >tree-with-malformed-mode &&\n+\thex_oid=$(echo foo | git hash-object --stdin -w) &&\n+\tbin_oid=$(echo $hex_oid | hex2oct) &&\n+\tprintf \"9100644 \\0$bin_oid\" >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+\tgrep \"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+\thex_oid=$(echo foo | git hash-object --stdin -w) &&\n+\tbin_oid=$(echo $hex_oid | hex2oct) &&\n+\tprintf \"100644 \\0$bin_oid\" >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+\tgrep \"empty filename in tree entry\" err\n '\n \n test_expect_success 'corrupt commit' '\n-- \n2.39.1.616.gd06fca9e99\n\n"},{"id":"470660","messageId":"Y8hYKD07HulByUcB@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hX+pIZUKXsyYj5@coredump.intra.peff.net","subject":"[PATCH 2/6] t1006: stop using 0-padded timestamps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-18T20:35:52Z","receivedAt":"2023-01-18T20:36:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The fake objects in t1006 use dummy timestamps like \"0000000000 +0000\".\nWhile this does make them look more like normal timestamps (which,\nunless it is 1970, have many digits), it actually violates our fsck\nchecks, which complain about zero-padded timestamps.\n\nThis doesn't currently break anything, but let's future-proof our tests\nagainst a version of hash-object which is a little more careful about\nits input. We don't actually care about the exact values here (and in\nfact, the helper functions in this script end up removing the timestamps\nanyway, so we don't even have to adjust other parts of the tests).\n\nSigned-off-by: Jeff King \n---\n t/t1006-cat-file.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 23b8942edb..2d875b17d8 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -292,8 +292,8 @@ commit_message=\"Initial commit\"\n commit_sha1=$(echo_without_newline \"$commit_message\" | git commit-tree $tree_sha1)\n commit_size=$(($(test_oid hexsz) + 137))\n commit_content=\"tree $tree_sha1\n-author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> 0000000000 +0000\n-committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 0000000000 +0000\n+author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> 0 +0000\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 0 +0000\n \n $commit_message\"\n \n@@ -304,7 +304,7 @@ type blob\n tag hellotag\n tagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n tag_description=\"This is a tag\"\n-tag_content=\"$tag_header_without_timestamp 0000000000 +0000\n+tag_content=\"$tag_header_without_timestamp 0 +0000\n \n $tag_description\"\n \n-- \n2.39.1.616.gd06fca9e99\n\n"},{"id":"470661","messageId":"Y8hYRn5+/zswl9Zx@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hX+pIZUKXsyYj5@coredump.intra.peff.net","subject":"[PATCH 3/6] t7030: stop using invalid tag name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-18T20:36:22Z","receivedAt":"2023-01-18T20:36:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We intentionally invalidate the signature of a tag by switching its tag\nname from \"seventh\" to \"7th forged\". However, the latter is not a valid\ntag name because it contains a space. This doesn't currently affect the\ntest, but we're better off using something syntactically valid. That\nreduces the number of possible failure modes in the test, and\nfuture-proofs us if git hash-object gets more picky about its input.\n\nThe t7031 script, which was mostly copied from t7030, has the same\nproblem, so we'll fix it, too.\n\nSigned-off-by: Jeff King \n---\n t/t7030-verify-tag.sh | 2 +-\n t/t7031-verify-tag-signed-ssh.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex 10faa64515..6f526c37c2 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -115,7 +115,7 @@ test_expect_success GPGSM 'verify and show signatures x509 with high minTrustLev\n \n test_expect_success GPG 'detect fudged signature' '\n \tgit cat-file tag seventh-signed >raw &&\n-\tsed -e \"/^tag / s/seventh/7th forged/\" raw >forged1 &&\n+\tsed -e \"/^tag / s/seventh/7th-forged/\" raw >forged1 &&\n \tgit hash-object -w -t tag forged1 >forged1.tag &&\n \ttest_must_fail git verify-tag $(cat forged1.tag) 2>actual1 &&\n \tgrep \"BAD signature from\" actual1 &&\ndiff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh\nindex 1cb36b9ab8..36eb86a4b1 100755\n--- a/t/t7031-verify-tag-signed-ssh.sh\n+++ b/t/t7031-verify-tag-signed-ssh.sh\n@@ -125,7 +125,7 @@ test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag failes with tag date ou\n test_expect_success GPGSSH 'detect fudged ssh signature' '\n \ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \tgit cat-file tag seventh-signed >raw &&\n-\tsed -e \"/^tag / s/seventh/7th forged/\" raw >forged1 &&\n+\tsed -e \"/^tag / s/seventh/7th-forged/\" raw >forged1 &&\n \tgit hash-object -w -t tag forged1 >forged1.tag &&\n \ttest_must_fail git verify-tag $(cat forged1.tag) 2>actual1 &&\n \tgrep \"${GPGSSH_BAD_SIGNATURE}\" actual1 &&\n-- \n2.39.1.616.gd06fca9e99\n\n"},{"id":"470662","messageId":"Y8hZlN9rRg1msc0L@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hX+pIZUKXsyYj5@coredump.intra.peff.net","subject":"[PATCH 4/6] t: use hash-object --literally when created malformed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-18T20:41:56Z","receivedAt":"2023-01-18T20:42:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many test scripts use hash-object to create malformed objects to see how\nwe handle the results in various commands. In some cases we already have\nto use \"hash-object --literally\", because it does some rudimentary\nquality checks. But let's use \"--literally\" more consistently to\nfuture-proof these tests against hash-object learning to be more\ncareful.\n\nSigned-off-by: Jeff King \n---\nThis patch is worth looking at because it shows the kinds of things the\nnew hash-object from patch 6 will reject.\n\nMost of these are obviously terrible things that we'd want to complain\nabout, like broken emails, embedded NULs, and so on. The most\ncontentious one is probably a tag without a tagger line, which were\ngenerated by early versions of Git (e.g., see Git's v0.99 tag). This is\nan \"info\" in fsck (which is semantically like a warning, except\ntransfer.fsckObjects treats warnings as errors due to hysterical\nraisins). But the hash-object change in patch 6 will reject it, because\nit operates in strict mode.\n\nThat seems reasonable to me, since we're helping users avoid doing bad\nthings, and not dealing with existing objects.\n\n t/t1450-fsck.sh | 28 ++++++++++++++--------------\n t/t4054-diff-bogus-tree.sh | 2 +-\n t/t4058-diff-duplicates.sh | 2 +-\n t/t4212-log-corrupt.sh | 4 ++--\n t/t5302-pack-index.sh | 2 +-\n t/t5504-fetch-receive-strict.sh | 2 +-\n t/t5702-protocol-v2.sh | 2 +-\n t/t6300-for-each-ref.sh | 2 +-\n t/t7509-commit-authorship.sh | 2 +-\n t/t7510-signed-commit.sh | 2 +-\n t/t7528-signed-commit-ssh.sh | 2 +-\n t/t8003-blame-corner-cases.sh | 2 +-\n t/t9350-fast-export.sh | 2 +-\n 13 files changed, 27 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex de0f6d5e7f..fdb886dfe4 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -212,7 +212,7 @@ test_expect_success 'email without @ is okay' '\n test_expect_success 'email with embedded > is not okay' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/@[a-z]/&>/\" basis >bad-email &&\n-\tnew=$(git hash-object -t commit -w --stdin is not okay' '\n test_expect_success 'missing < email delimiter is reported nicely' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/bad-email-2 &&\n-\tnew=$(git hash-object -t commit -w --stdin basis &&\n \tsed \"s/[a-z]* <[^>]*>//\" basis >bad-email-3 &&\n-\tnew=$(git hash-object -t commit -w --stdin in name is reported' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/ bad-email-4 &&\n-\tnew=$(git hash-object -t commit -w --stdin basis &&\n \tsed \"s/^\\\\(author .*>\\\\) [0-9]*/\\\\1 18446744073709551617/\" \\\n \t\tbad-timestamp &&\n-\tnew=$(git hash-object -t commit -w --stdin basis &&\n \tsed \"s/author ./author Q/\" commit-NUL-header &&\n-\tnew=$(git hash-object -t commit -w --stdin out &&\n \ttest_i18ngrep \"error in tree .*contains duplicate file entries\" out\n@@ -426,7 +426,7 @@ test_expect_success 'tag with incorrect tag name & missing tagger' '\n \tThis is an invalid tag.\n \tEOF\n \n-\ttag=$(git hash-object -t tag -w --stdin .git/refs/tags/wrong &&\n \ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n@@ -558,7 +558,7 @@ test_expect_success 'rev-list --verify-objects with commit graph (parent)' '\n test_expect_success 'force fsck to ignore double author' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/^author .*/&,&/\" multiple-authors &&\n-\tnew=$(git hash-object -t commit -w --stdin out &&\n \t test_i18ngrep \"warning.*null sha1\" out\n \t)\n@@ -583,7 +583,7 @@ test_expect_success 'fsck notices submodule entry pointing to null sha1' '\n \t(git init null-commit &&\n \t cd null-commit &&\n \t sha=$(printf \"160000 submodule$_bz$_bzoid\" |\n-\t git hash-object -w --stdin -t tree) &&\n+\t git hash-object --literally -w --stdin -t tree) &&\n \t git fsck 2>out &&\n \t test_i18ngrep \"warning.*null sha1\" out\n \t)\n@@ -648,7 +648,7 @@ test_expect_success 'NUL in commit' '\n \t\tgit commit --allow-empty -m \"initial commitQNUL after message\" &&\n \t\tgit cat-file commit HEAD >original &&\n \t\tq_to_nul munged &&\n-\t\tgit hash-object -w -t commit --stdin name &&\n+\t\tgit hash-object --literally -w -t commit --stdin name &&\n \t\tgit branch bad $(cat name) &&\n \n \t\ttest_must_fail git -c fsck.nulInCommit=error fsck 2>warn.1 &&\n@@ -794,8 +794,8 @@ test_expect_success 'fsck errors in packed objects' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/one &&\n \tsed \"s/two &&\n-\tone=$(git hash-object -t commit -w one) &&\n-\ttwo=$(git hash-object -t commit -w two) &&\n+\tone=$(git hash-object --literally -t commit -w one) &&\n+\ttwo=$(git hash-object --literally -t commit -w two) &&\n \tpack=$(\n \t\t{\n \t\t\techo $one &&\ndiff --git a/t/t4054-diff-bogus-tree.sh b/t/t4054-diff-bogus-tree.sh\nindex 294fb55313..05c88f8cdf 100755\n--- a/t/t4054-diff-bogus-tree.sh\n+++ b/t/t4054-diff-bogus-tree.sh\n@@ -10,7 +10,7 @@ test_expect_success 'create bogus tree' '\n \tbogus_tree=$(\n \t\tprintf \"100644 fooQ$name\" |\n \t\tq_to_nul |\n-\t\tgit hash-object -w --stdin -t tree\n+\t\tgit hash-object --literally -w --stdin -t tree\n \t)\n '\n \ndiff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh\nindex 54614b814d..2501c89c1c 100755\n--- a/t/t4058-diff-duplicates.sh\n+++ b/t/t4058-diff-duplicates.sh\n@@ -29,7 +29,7 @@ make_tree () {\n \t\tmake_tree_entry \"$1\" \"$2\" \"$3\"\n \t\tshift; shift; shift\n \tdone |\n-\tgit hash-object -w -t tree --stdin\n+\tgit hash-object --literally -w -t tree --stdin\n }\n \n # this is kind of a convoluted setup, but matches\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex 30a219894b..e89e1f54b6 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -10,7 +10,7 @@ test_expect_success 'setup' '\n \n \tgit cat-file commit HEAD |\n \tsed \"/^author /s/>/>-<>/\" >broken_email.commit &&\n-\tgit hash-object -w -t commit broken_email.commit >broken_email.hash &&\n+\tgit hash-object --literally -w -t commit broken_email.commit >broken_email.hash &&\n \tgit update-ref refs/heads/broken_email $(cat broken_email.hash)\n '\n \n@@ -46,7 +46,7 @@ test_expect_success 'git log --format with broken author email' '\n munge_author_date () {\n \tgit cat-file commit \"$1\" >commit.orig &&\n \tsed \"s/^\\(author .*>\\) [0-9]*/\\1 $2/\" commit.munge &&\n-\tgit hash-object -w -t commit commit.munge\n+\tgit hash-object --literally -w -t commit commit.munge\n }\n \n test_expect_success 'unparsable dates produce sentinel value' '\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex b0095ab41d..59e9e77223 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -263,7 +263,7 @@ tag guten tag\n This is an invalid tag.\n EOF\n \n-\ttag=$(git hash-object -t tag -w --stdin \"$P/my-blob\" &&\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 2ae1fc721b..c466fd989f 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -606,7 +606,7 @@ test_expect_success 'create tag without tagger' '\n \tgit tag -a -m \"Broken tag\" taggerless &&\n \tgit tag -f taggerless $(git cat-file tag taggerless |\n \t\tsed -e \"/^tagger /d\" |\n-\t\tgit hash-object --stdin -w -t tag)\n+\t\tgit hash-object --literally --stdin -w -t tag)\n '\n \n test_atom refs/tags/taggerless type 'commit'\ndiff --git a/t/t7509-commit-authorship.sh b/t/t7509-commit-authorship.sh\nindex 21c668f75e..5d890949f7 100755\n--- a/t/t7509-commit-authorship.sh\n+++ b/t/t7509-commit-authorship.sh\n@@ -105,7 +105,7 @@ test_expect_success '--amend option with empty author' '\n test_expect_success '--amend option with missing author' '\n \tgit cat-file commit Initial >tmp &&\n \tsed \"s/author [^<]* malformed &&\n-\tsha=$(git hash-object -t commit -w malformed) &&\n+\tsha=$(git hash-object --literally -t commit -w malformed) &&\n \ttest_when_finished \"remove_object $sha\" &&\n \tgit checkout $sha &&\n \ttest_when_finished \"git checkout Initial\" &&\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 8593b7e3cb..bc7a31ba3e 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -202,7 +202,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n \tgit cat-file commit seventh-signed >raw &&\n \tcat raw >forged2 &&\n \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n-\tgit hash-object -w -t commit forged2 >forged2.commit &&\n+\tgit hash-object --literally -w -t commit forged2 >forged2.commit &&\n \ttest_must_fail git verify-commit $(cat forged2.commit) &&\n \tgit show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&\n \tgrep \"BAD signature from\" actual2 &&\ndiff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh\nindex f47e995179..065f780636 100755\n--- a/t/t7528-signed-commit-ssh.sh\n+++ b/t/t7528-signed-commit-ssh.sh\n@@ -270,7 +270,7 @@ test_expect_success GPGSSH 'detect fudged signature with NUL' '\n \tgit cat-file commit seventh-signed >raw &&\n \tcat raw >forged2 &&\n \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n-\tgit hash-object -w -t commit forged2 >forged2.commit &&\n+\tgit hash-object --literally -w -t commit forged2 >forged2.commit &&\n \ttest_must_fail git verify-commit $(cat forged2.commit) &&\n \tgit show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&\n \tgrep \"${GPGSSH_BAD_SIGNATURE}\" actual2 &&\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex d751d48b7d..8bcd39e81b 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -201,7 +201,7 @@ committer David Reiss 1234567890 +0000\n \n some message\n EOF\n- COMMIT=$(git hash-object -t commit -w badcommit) &&\n+ COMMIT=$(git hash-object --literally -t commit -w badcommit) &&\n git --no-pager blame $COMMIT -- uno >/dev/null\n '\n \ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex ff21a12ee6..26c25c0eb2 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -373,7 +373,7 @@ EOF\n \n test_expect_success 'cope with tagger-less tags' '\n \n-\tTAG=$(git hash-object -t tag -w tag-content) &&\n+\tTAG=$(git hash-object --literally -t tag -w tag-content) &&\n \tgit update-ref refs/tags/sonnenschein $TAG &&\n \tgit fast-export -C -C --signed-tags=strip --all > output &&\n \ttest $(grep -c \"^tag \" output) = 4 &&\n-- \n2.39.1.616.gd06fca9e99\n\n"},{"id":"470664","messageId":"Y8haCbAQIV9s/95l@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hX+pIZUKXsyYj5@coredump.intra.peff.net","subject":"[PATCH 5/6] fsck: provide a function to fsck buffer without object struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-18T20:43:53Z","receivedAt":"2023-01-18T20:44:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The fsck code has been slowly moving away from requiring an object\nstruct in commits like 103fb6d43b (fsck: accept an oid instead of a\n\"struct tag\" for fsck_tag(), 2019-10-18), c5b4269b57 (fsck: accept an\noid instead of a \"struct commit\" for fsck_commit(), 2019-10-18), etc.\n\nHowever, the only external interface that fsck.c provides is\nfsck_object(), which requires an object struct, then promptly discards\neverything except its oid and type. Let's factor out the post-discard\npart of that function as fsck_buffer(), leaving fsck_object() as a thin\nwrapper around it. That will provide more flexibility for callers which\nmay not have a struct.\n\nSigned-off-by: Jeff King \n---\nThis is obviously preparation for the next patch. But I suspect it could\nbe used elsewhere, too. Regular fsck wants object structs anyway to hold\nflags, I think, but index-pack could probably save some memory and\neffort by avoiding them. I didn't look too closely, as it's all out of\nscope for this series.\n\n fsck.c | 29 ++++++++++++++++++-----------\n fsck.h | 8 ++++++++\n 2 files changed, 26 insertions(+), 11 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 47eaeedd70..c2c8facd2d 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1237,19 +1237,26 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \tif (!obj)\n \t\treturn report(options, NULL, OBJ_NONE, FSCK_MSG_BAD_OBJECT_SHA1, \"no valid object to fsck\");\n \n-\tif (obj->type == OBJ_BLOB)\n-\t\treturn fsck_blob(&obj->oid, data, size, options);\n-\tif (obj->type == OBJ_TREE)\n-\t\treturn fsck_tree(&obj->oid, data, size, options);\n-\tif (obj->type == OBJ_COMMIT)\n-\t\treturn fsck_commit(&obj->oid, data, size, options);\n-\tif (obj->type == OBJ_TAG)\n-\t\treturn fsck_tag(&obj->oid, data, size, options);\n-\n-\treturn report(options, &obj->oid, obj->type,\n+\treturn fsck_buffer(&obj->oid, obj->type, data, size, options);\n+}\n+\n+int fsck_buffer(const struct object_id *oid, enum object_type type,\n+\t\tvoid *data, unsigned long size,\n+\t\tstruct fsck_options *options)\n+{\n+\tif (type == OBJ_BLOB)\n+\t\treturn fsck_blob(oid, data, size, options);\n+\tif (type == OBJ_TREE)\n+\t\treturn fsck_tree(oid, data, size, options);\n+\tif (type == OBJ_COMMIT)\n+\t\treturn fsck_commit(oid, data, size, options);\n+\tif (type == OBJ_TAG)\n+\t\treturn fsck_tag(oid, data, size, options);\n+\n+\treturn report(options, oid, type,\n \t\t FSCK_MSG_UNKNOWN_TYPE,\n \t\t \"unknown type '%d' (internal fsck error)\",\n-\t\t obj->type);\n+\t\t type);\n }\n \n int fsck_error_function(struct fsck_options *o,\ndiff --git a/fsck.h b/fsck.h\nindex fcecf4101c..668330880e 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -183,6 +183,14 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options);\n int fsck_object(struct object *obj, void *data, unsigned long size,\n \tstruct fsck_options *options);\n \n+/*\n+ * Same as fsck_object(), but for when the caller doesn't have an object\n+ * struct.\n+ */\n+int fsck_buffer(const struct object_id *oid, enum object_type,\n+\t\tvoid *data, unsigned long size,\n+\t\tstruct fsck_options *options);\n+\n /*\n * fsck a tag, and pass info about it back to the caller. This is\n * exposed fsck_object() internals for git-mktag(1).\n-- \n2.39.1.616.gd06fca9e99\n\n"},{"id":"470665","messageId":"Y8haHL9xIWntSm0/@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hX+pIZUKXsyYj5@coredump.intra.peff.net","subject":"[PATCH 6/6] hash-object: use fsck for object checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-18T20:44:12Z","receivedAt":"2023-01-18T20:44:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since c879daa237 (Make hash-object more robust against malformed\nobjects, 2011-02-05), we've done some rudimentary checks against objects\nwe're about to write by running them through our usual parsers for\ntrees, commits, and tags.\n\nThese parsers catch some problems, but they are not nearly as careful as\nthe fsck functions (which make sense; the parsers are designed to be\nfast and forgiving, bailing only when the input is unintelligible). We\nare better off doing the more thorough fsck checks when writing objects.\nDoing so at write time is much better than writing garbage only to find\nout later (after building more history atop it!) that fsck complains\nabout it, or hosts with transfer.fsckObjects reject it.\n\nThis is obviously going to be a user-visible behavior change, and the\ntest changes earlier in this series show the scope of the impact. But\nI'd argue that this is OK:\n\n - the documentation for hash-object is already vague about which\n checks we might do, saying that --literally will allow \"any\n garbage[...] which might not otherwise pass standard object parsing\n or git-fsck checks\". So we are already covered under the documented\n behavior.\n\n - users don't generally run hash-object anyway. There are a lot of\n spots in the tests that needed to be updated because creating\n garbage objects is something that Git's tests disproportionately do.\n\n - it's hard to imagine anyone thinking the new behavior is worse. Any\n object we reject would be a potential problem down the road for the\n user. And if they really want to create garbage, --literally is\n already the escape hatch they need.\n\nNote that the change here is actually in index_mem(), which handles the\nHASH_FORMAT_CHECK flag passed by hash-object. That flag is also used by\n\"git-replace --edit\" to sanity-check the result. Covering that with more\nthorough checks likewise seems like a good thing.\n\nBesides being more thorough, there are a few other bonuses:\n\n - we get rid of some questionable stack allocations of object structs.\n These don't seem to currently cause any problems in practice, but\n they subtly violate some of the assumptions made by the rest of the\n code (e.g., the \"struct commit\" we put on the stack and\n zero-initialize will not have a proper index from\n alloc_comit_index().\n\n - likewise, those parsed object structs are the source of some small\n memory leaks\n\n - the resulting messages are much better. For example:\n\n [before]\n $ echo 'tree 123' | git hash-object -t commit --stdin\n error: bogus commit object 0000000000000000000000000000000000000000\n fatal: corrupt commit\n\n [after]\n $ echo 'tree 123' | git.compile hash-object -t commit --stdin\n error: object fails fsck: badTreeSha1: invalid 'tree' line format - bad sha1\n fatal: refusing to create malformed object\n\nSigned-off-by: Jeff King \n---\n object-file.c | 55 ++++++++++++++++++------------------------\n t/t1007-hash-object.sh | 11 +++++++++\n 2 files changed, 34 insertions(+), 32 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 80a0cd3b35..5c96384803 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -33,6 +33,7 @@\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"submodule.h\"\n+#include \"fsck.h\"\n \n /* The maximum size for an object header. */\n #define MAX_HEADER_LEN 32\n@@ -2298,32 +2299,21 @@ int repo_has_object_file(struct repository *r,\n \treturn repo_has_object_file_with_flags(r, oid, 0);\n }\n \n-static void check_tree(const void *buf, size_t size)\n-{\n-\tstruct tree_desc desc;\n-\tstruct name_entry entry;\n-\n-\tinit_tree_desc(&desc, buf, size);\n-\twhile (tree_entry(&desc, &entry))\n-\t\t/* do nothing\n-\t\t * tree_entry() will die() on malformed entries */\n-\t\t;\n-}\n-\n-static void check_commit(const void *buf, size_t size)\n-{\n-\tstruct commit c;\n-\tmemset(&c, 0, sizeof(c));\n-\tif (parse_commit_buffer(the_repository, &c, buf, size, 0))\n-\t\tdie(_(\"corrupt commit\"));\n-}\n-\n-static void check_tag(const void *buf, size_t size)\n-{\n-\tstruct tag t;\n-\tmemset(&t, 0, sizeof(t));\n-\tif (parse_tag_buffer(the_repository, &t, buf, size))\n-\t\tdie(_(\"corrupt tag\"));\n+/*\n+ * We can't use the normal fsck_error_function() for index_mem(),\n+ * because we don't yet have a valid oid for it to report. Instead,\n+ * report the minimal fsck error here, and rely on the caller to\n+ * give more context.\n+ */\n+static int hash_format_check_report(struct fsck_options *opts,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t enum object_type object_type,\n+\t\t\t\t enum fsck_msg_type msg_type,\n+\t\t\t\t enum fsck_msg_id msg_id,\n+\t\t\t\t const char *message)\n+{\n+\terror(_(\"object fails fsck: %s\"), message);\n+\treturn 1;\n }\n \n static int index_mem(struct index_state *istate,\n@@ -2350,12 +2340,13 @@ static int index_mem(struct index_state *istate,\n \t\t}\n \t}\n \tif (flags & HASH_FORMAT_CHECK) {\n-\t\tif (type == OBJ_TREE)\n-\t\t\tcheck_tree(buf, size);\n-\t\tif (type == OBJ_COMMIT)\n-\t\t\tcheck_commit(buf, size);\n-\t\tif (type == OBJ_TAG)\n-\t\t\tcheck_tag(buf, size);\n+\t\tstruct fsck_options opts = FSCK_OPTIONS_DEFAULT;\n+\n+\t\topts.strict = 1;\n+\t\topts.error_func = hash_format_check_report;\n+\t\tif (fsck_buffer(null_oid(), type, buf, size, &opts))\n+\t\t\tdie(_(\"refusing to create malformed object\"));\n+\t\tfsck_finish(&opts);\n \t}\n \n \tif (write_object)\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex 2d2148d8fa..ac3d173767 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -222,6 +222,17 @@ test_expect_success 'empty filename in tree' '\n \tgrep \"empty filename in tree entry\" err\n '\n \n+test_expect_success 'duplicate filename in tree' '\n+\thex_oid=$(echo foo | git hash-object --stdin -w) &&\n+\tbin_oid=$(echo $hex_oid | hex2oct) &&\n+\t{\n+\t\tprintf \"100644 file\\0$bin_oid\" &&\n+\t\tprintf \"100644 file\\0$bin_oid\"\n+\t} >tree-with-duplicate-filename &&\n+\ttest_must_fail git hash-object -t tree tree-with-duplicate-filename 2>err &&\n+\tgrep \"duplicateEntries\" err\n+'\n+\n test_expect_success 'corrupt commit' '\n \ttest_must_fail git hash-object -t commit --stdin The other option is having the fsck code avoid looking past the size it\n> was given. I think the intent is that this should work, from commits\n> like 4d0d89755e (Make sure fsck_commit_buffer() does not run out of the\n> buffer, 2014-09-11). We do use skip_prefix() and parse_oid_hex(), which\n> won't respect the size, but I think[1] that's OK because we'll have\n> parsed up to the end-of-header beforehand (and those functions would\n> never match past there).\n> \n> Which would mean that 9a1a3a4d4c (mktag: allow omitting the header/body\n> \\n separator, 2021-01-05) and acf9de4c94 (mktag: use fsck instead of\n> custom verify_tag(), 2021-01-05) were buggy, and we can just fix them.\n\nThat would look something like this:\n\ndiff --git a/fsck.c b/fsck.c\nindex c2c8facd2d..d220276bcb 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -898,6 +898,7 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n {\n \tint ret = 0;\n \tchar *eol;\n+\tconst char *eob = buffer + size;\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *p;\n \n@@ -960,10 +961,8 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \t}\n \telse\n \t\tret = fsck_ident(&buffer, oid, OBJ_TAG, options);\n-\tif (!*buffer)\n-\t\tgoto done;\n \n-\tif (!starts_with(buffer, \"\\n\")) {\n+\tif (buffer != eob && *buffer != '\\n') {\n \t\t/*\n \t\t * The verify_headers() check will allow\n \t\t * e.g. \"[...]tagger \\nsome\n\nChanging the starts_with() is not strictly necessary, but I think it\nmakes it more clear that we are only going to look at the one character\nwe confirmed is still valid inside the buffer.\n\nThis is enough to have the whole test suite pass with ASan/UBSan after\nmy series. But as I said earlier, I'd want to look carefully at the rest\nof the fsck code to make sure there aren't any other possible inputs\nthat could look past the end of the buffer.\n\n-Peff\n"},{"id":"470668","messageId":"xmqqmt6f4l03.fsf@gitster.g","threadId":"59115","inReplyTo":"Y8hX+pIZUKXsyYj5@coredump.intra.peff.net","subject":"Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-18T20:59:24Z","receivedAt":"2023-01-18T20:59:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> [1/6]: t1007: modernize malformed object tests\n\nObviously good.\n\n> [2/6]: t1006: stop using 0-padded timestamps\n> [3/6]: t7030: stop using invalid tag name\n\nThese two are pleasant to see and revealed what are \"accepted\" by\nmistake, quite surprisingly.\n\n> [4/6]: t: use hash-object --literally when created malformed objects\n\nThe --literally option was invented initially primarily to allow a\nbogus type of object (e.g. \"hash-object -t xyzzy --literally\") but I\nam happy to see that we are finding different uses. I wonder if\nthese objects of known types but with syntactically bad contents can\nbe \"repack\"ed from loose into packed?\n\n> [5/6]: fsck: provide a function to fsck buffer without object struct\n\nObvious, clean and very nice.\n\n> [6/6]: hash-object: use fsck for object checks\n"},{"id":"470669","messageId":"Y8hhB2U0KD5vXJjZ@nand.local","threadId":"59115","inReplyTo":"Y8hYEgMze3bY44/0@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] t1007: modernize malformed object tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-01-18T21:13:43Z","receivedAt":"2023-01-18T21:15:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 18, 2023 at 03:35:30PM -0500, Jeff King wrote:\n> The tests in t1007 for detecting malformed objects have two\n> anachronisms:\n>\n> - they use \"sha1\" instead of \"oid\" in variable names, even though the\n> script as a whole has been adapted to handle sha256\n\nI appreciate you saying that we should s/sha1/oid here. But more\nimportantly, thanks for drawing attention to the fact that this script\nalready handles sha256, and that the update is purely cosmetic.\n\n> ---\n> t/t1007-hash-object.sh | 18 +++++++++---------\n> 1 file changed, 9 insertions(+), 9 deletions(-)\n\nThese look obviously correct.\n\nThanks,\nTaylor\n"},{"id":"470670","messageId":"Y8hiWGOArYqzen58@nand.local","threadId":"59115","inReplyTo":"Y8hZlN9rRg1msc0L@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] t: use hash-object --literally when created malformed objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-01-18T21:19:20Z","receivedAt":"2023-01-18T21:19:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 18, 2023 at 03:41:56PM -0500, Jeff King wrote:\n> Many test scripts use hash-object to create malformed objects to see how\n> we handle the results in various commands. In some cases we already have\n> to use \"hash-object --literally\", because it does some rudimentary\n> quality checks. But let's use \"--literally\" more consistently to\n> future-proof these tests against hash-object learning to be more\n> careful.\n\nHeh, I suppose this is a good illustration of how loose our checks our\neven without `--literally` ;-).\n\n> ---\n> This patch is worth looking at because it shows the kinds of things the\n> new hash-object from patch 6 will reject.\n\nObviously we could avoid this patch entirely by making the new behavior\nof fscking the incoming objects hidden behind a `--fsck` flag or\nsomething. But I think the decision not to is a good one.\n\nWe already have `--literally`, and it makes sense that passing that\nshould let us write anything, and that not passing it should perform\nsome validity checks. But I think exactly *what* those checks are is\nambiguous enough that the absence of `--literally` implying fsck checks\nisn't out of the question.\n\nYou address this in the last patch more thoroughly, but I figure that it\nis worth stating some of this here during review to indicate that I\nthink the direction you pursued here is a good one.\n\n> t/t1450-fsck.sh | 28 ++++++++++++++--------------\n> t/t4054-diff-bogus-tree.sh | 2 +-\n> t/t4058-diff-duplicates.sh | 2 +-\n> t/t4212-log-corrupt.sh | 4 ++--\n> t/t5302-pack-index.sh | 2 +-\n> t/t5504-fetch-receive-strict.sh | 2 +-\n> t/t5702-protocol-v2.sh | 2 +-\n> t/t6300-for-each-ref.sh | 2 +-\n> t/t7509-commit-authorship.sh | 2 +-\n> t/t7510-signed-commit.sh | 2 +-\n> t/t7528-signed-commit-ssh.sh | 2 +-\n> t/t8003-blame-corner-cases.sh | 2 +-\n> t/t9350-fast-export.sh | 2 +-\n> 13 files changed, 27 insertions(+), 27 deletions(-)\n\nAnd these all look good, too. Each of the spots you touch here is\nlimited to replacing \"git hash-object\" with \"git hash-object --literally\".\n\nThanks,\nTaylor\n"},{"id":"470671","messageId":"Y8hjiWUAIuiWfJoD@nand.local","threadId":"59115","inReplyTo":"Y8haCbAQIV9s/95l@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] fsck: provide a function to fsck buffer without object struct","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-01-18T21:24:25Z","receivedAt":"2023-01-18T21:24:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 18, 2023 at 03:43:53PM -0500, Jeff King wrote:\n> However, the only external interface that fsck.c provides is\n> fsck_object(), which requires an object struct, then promptly discards\n> everything except its oid and type. Let's factor out the post-discard\n> part of that function as fsck_buffer(), leaving fsck_object() as a thin\n> wrapper around it. That will provide more flexibility for callers which\n> may not have a struct.\n\nIt's really nice that the only thing we care about having an object\nstruct around for is basically just knowing its type. IOW it seems to\nhave made the refactoring here pretty straightforward, which is nice\n;-).\n\nThanks,\nTaylor\n"},{"id":"470672","messageId":"Y8hlyr0o6gs9omI5@nand.local","threadId":"59115","inReplyTo":"Y8haHL9xIWntSm0/@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] hash-object: use fsck for object checks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-01-18T21:34:02Z","receivedAt":"2023-01-18T21:34:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 18, 2023 at 03:44:12PM -0500, Jeff King wrote:\n> This is obviously going to be a user-visible behavior change, and the\n> test changes earlier in this series show the scope of the impact. But\n> I'd argue that this is OK:\n>\n> - the documentation for hash-object is already vague about which\n> checks we might do, saying that --literally will allow \"any\n> garbage[...] which might not otherwise pass standard object parsing\n> or git-fsck checks\". So we are already covered under the documented\n> behavior.\n>\n> - users don't generally run hash-object anyway. There are a lot of\n> spots in the tests that needed to be updated because creating\n> garbage objects is something that Git's tests disproportionately do.\n>\n> - it's hard to imagine anyone thinking the new behavior is worse. Any\n> object we reject would be a potential problem down the road for the\n> user. And if they really want to create garbage, --literally is\n> already the escape hatch they need.\n\nThis is the discussion I was pointing out earlier in the series as\nevidence for making this behavior the new default without \"--literally\".\n\nThat being said, let me play devil's advocate for a second. Do the new\nfsck checks slow anything in hash-object down significantly? If so, then\nit's plausible to imagine a hash-object caller who (a) doesn't use\n`--literally`, but (b) does care about throughput if they're writing a\nlarge number of objects at once.\n\nI don't know if such a situation exists, or if these new fsck checks\neven slow hash-object down enough to care. But I didn't catch a\ndiscussion of this case in your series, so I figured I'd bring it up\nhere just in case.\n\n> - the resulting messages are much better. For example:\n>\n> [before]\n> $ echo 'tree 123' | git hash-object -t commit --stdin\n> error: bogus commit object 0000000000000000000000000000000000000000\n> fatal: corrupt commit\n>\n> [after]\n> $ echo 'tree 123' | git.compile hash-object -t commit --stdin\n> error: object fails fsck: badTreeSha1: invalid 'tree' line format - bad sha1\n> fatal: refusing to create malformed object\n\nMuch nicer, well done.\n\n> Signed-off-by: Jeff King \n> ---\n> object-file.c | 55 ++++++++++++++++++------------------------\n> t/t1007-hash-object.sh | 11 +++++++++\n> 2 files changed, 34 insertions(+), 32 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 80a0cd3b35..5c96384803 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -33,6 +33,7 @@\n> #include \"object-store.h\"\n> #include \"promisor-remote.h\"\n> #include \"submodule.h\"\n> +#include \"fsck.h\"\n>\n> /* The maximum size for an object header. */\n> #define MAX_HEADER_LEN 32\n> @@ -2298,32 +2299,21 @@ int repo_has_object_file(struct repository *r,\n> \treturn repo_has_object_file_with_flags(r, oid, 0);\n> }\n>\n> -static void check_tree(const void *buf, size_t size)\n> -{\n> -\tstruct tree_desc desc;\n> -\tstruct name_entry entry;\n> -\n> -\tinit_tree_desc(&desc, buf, size);\n> -\twhile (tree_entry(&desc, &entry))\n> -\t\t/* do nothing\n> -\t\t * tree_entry() will die() on malformed entries */\n> -\t\t;\n> -}\n> -\n> -static void check_commit(const void *buf, size_t size)\n> -{\n> -\tstruct commit c;\n> -\tmemset(&c, 0, sizeof(c));\n> -\tif (parse_commit_buffer(the_repository, &c, buf, size, 0))\n> -\t\tdie(_(\"corrupt commit\"));\n> -}\n> -\n> -static void check_tag(const void *buf, size_t size)\n> -{\n> -\tstruct tag t;\n> -\tmemset(&t, 0, sizeof(t));\n> -\tif (parse_tag_buffer(the_repository, &t, buf, size))\n> -\t\tdie(_(\"corrupt tag\"));\n\nOK, here we're getting rid of all of the lightweight checks that\nhash-object used to implement on its own.\n\n> +/*\n> + * We can't use the normal fsck_error_function() for index_mem(),\n> + * because we don't yet have a valid oid for it to report. Instead,\n> + * report the minimal fsck error here, and rely on the caller to\n> + * give more context.\n> + */\n> +static int hash_format_check_report(struct fsck_options *opts,\n> +\t\t\t\t const struct object_id *oid,\n> +\t\t\t\t enum object_type object_type,\n> +\t\t\t\t enum fsck_msg_type msg_type,\n> +\t\t\t\t enum fsck_msg_id msg_id,\n> +\t\t\t\t const char *message)\n> +{\n> +\terror(_(\"object fails fsck: %s\"), message);\n> +\treturn 1;\n> }\n>\n> static int index_mem(struct index_state *istate,\n> @@ -2350,12 +2340,13 @@ static int index_mem(struct index_state *istate,\n> \t\t}\n> \t}\n> \tif (flags & HASH_FORMAT_CHECK) {\n> -\t\tif (type == OBJ_TREE)\n> -\t\t\tcheck_tree(buf, size);\n> -\t\tif (type == OBJ_COMMIT)\n> -\t\t\tcheck_commit(buf, size);\n> -\t\tif (type == OBJ_TAG)\n> -\t\t\tcheck_tag(buf, size);\n> +\t\tstruct fsck_options opts = FSCK_OPTIONS_DEFAULT;\n> +\n> +\t\topts.strict = 1;\n> +\t\topts.error_func = hash_format_check_report;\n> +\t\tif (fsck_buffer(null_oid(), type, buf, size, &opts))\n> +\t\t\tdie(_(\"refusing to create malformed object\"));\n> +\t\tfsck_finish(&opts);\n> \t}\n\nAnd here's the main part of the change, which is delightfully simple and\nappears correct to me.\n\n> diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\n> index 2d2148d8fa..ac3d173767 100755\n> --- a/t/t1007-hash-object.sh\n> +++ b/t/t1007-hash-object.sh\n> @@ -222,6 +222,17 @@ test_expect_success 'empty filename in tree' '\n> \tgrep \"empty filename in tree entry\" err\n> '\n>\n> +test_expect_success 'duplicate filename in tree' '\n> +\thex_oid=$(echo foo | git hash-object --stdin -w) &&\n> +\tbin_oid=$(echo $hex_oid | hex2oct) &&\n> +\t{\n> +\t\tprintf \"100644 file\\0$bin_oid\" &&\n> +\t\tprintf \"100644 file\\0$bin_oid\"\n> +\t} >tree-with-duplicate-filename &&\n> +\ttest_must_fail git hash-object -t tree tree-with-duplicate-filename 2>err &&\n> +\tgrep \"duplicateEntries\" err\n> +'\n> +\n\nFor what it's worth, I think that this is sufficient coverage for the\nnew fsck checks.\n\nThanks,\nTaylor\n"},{"id":"470673","messageId":"Y8hm4KKVwguGGrdn@nand.local","threadId":"59115","inReplyTo":"xmqqmt6f4l03.fsf@gitster.g","subject":"Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-01-18T21:38:40Z","receivedAt":"2023-01-18T21:39:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 18, 2023 at 12:59:24PM -0800, Junio C Hamano wrote:\n> The --literally option was invented initially primarily to allow a\n> bogus type of object (e.g. \"hash-object -t xyzzy --literally\") but I\n> am happy to see that we are finding different uses. I wonder if\n> these objects of known types but with syntactically bad contents can\n> be \"repack\"ed from loose into packed?\n>\n> > [5/6]: fsck: provide a function to fsck buffer without object struct\n\nIt is indeed possible:\n\n--- >8 ---\nInitialized empty Git repository in /home/ttaylorr/src/git/t/trash directory.t9999-test/.git/\nexpecting success of 9999.1 'repacking corrupt loose object into packed':\n\tname=$(echo $ZERO_OID | sed -e \"s/00/Q/g\") &&\n\tprintf \"100644 fooQ$name\" | q_to_nul |\n\t\tgit hash-object -w --stdin -t tree >in &&\n\n\tgit pack-objects .git/objects/pack/pack The other option is having the fsck code avoid looking past the size it\n> was given. I think the intent is that this should work, from commits\n> like 4d0d89755e (Make sure fsck_commit_buffer() does not run out of the\n> buffer, 2014-09-11). We do use skip_prefix() and parse_oid_hex(), which\n> won't respect the size, but I think[1] that's OK because we'll have\n> parsed up to the end-of-header beforehand (and those functions would\n> never match past there).\n> \n> Which would mean that 9a1a3a4d4c (mktag: allow omitting the header/body\n> \\n separator, 2021-01-05) and acf9de4c94 (mktag: use fsck instead of\n> custom verify_tag(), 2021-01-05) were buggy, and we can just fix them.\n> \n> [1] But I said \"I think\" above because it can get pretty subtle. There's\n> some more discussion in this thread:\n> \n> https://lore.kernel.org/git/20150625155128.C3E9738005C@gemini.denx.de/\n> \n> but I haven't yet convinced myself it's safe. This is exactly the\n> kind of analysis I wish I had the power to nerd-snipe René into.\n\nI poked at this a bit more, and it definitely isn't safe. I think the\nuse of skip_prefix(), etc, is OK, because they'd always stop at an\nunexpected newline. But verify_headers() is only confirming that we have\na series of complete lines, and we might end with no \"\\n\\n\" (and hence\nno commit/tag message). And so the obvious case that fools us is one\nwhere the data simply ends at a newline, but we are missing one or more\nheaders. So a truncated commit like:\n\n tree 1234567890123456789012345678901234567890\n\n(with the newline at the end of the \"tree\" line, but nothing else) will\ncause fsck_commit() to look past the \"size\" we pass it. With all of the\ncurrent callers, that means it sees a NUL and bails. So it's not\ncurrently a bug, but it becomes one if we can feed it arbitrary buffers.\n\nFixing it isn't _too_ bad, and could look something like this:\n\ndiff --git a/fsck.c b/fsck.c\nindex c2c8facd2d..3f0bb3e350 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -834,6 +834,7 @@ static int fsck_commit(const struct object_id *oid,\n \tunsigned author_count;\n \tint err;\n \tconst char *buffer_begin = buffer;\n+\tconst char *buffer_end = buffer + size;\n \tconst char *p;\n \n \tif (verify_headers(buffer, size, oid, OBJ_COMMIT, options))\n@@ -847,7 +848,7 @@ static int fsck_commit(const struct object_id *oid,\n \t\t\treturn err;\n \t}\n \tbuffer = p + 1;\n-\twhile (skip_prefix(buffer, \"parent \", &buffer)) {\n+\twhile (buffer < buffer_end && skip_prefix(buffer, \"parent \", &buffer)) {\n \t\tif (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n \t\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n \t\t\tif (err)\n@@ -856,7 +857,7 @@ static int fsck_commit(const struct object_id *oid,\n \t\tbuffer = p + 1;\n \t}\n \tauthor_count = 0;\n-\twhile (skip_prefix(buffer, \"author \", &buffer)) {\n+\twhile (buffer < buffer_end && skip_prefix(buffer, \"author \", &buffer)) {\n \t\tauthor_count++;\n \t\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n \t\tif (err)\n@@ -868,7 +869,7 @@ static int fsck_commit(const struct object_id *oid,\n \t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n \tif (err)\n \t\treturn err;\n-\tif (!skip_prefix(buffer, \"committer \", &buffer))\n+\tif (buffer >= buffer_end || !skip_prefix(buffer, \"committer \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n \terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n \tif (err)\n\nAnd then the tag side would need something similar. I'd probably also\nsprinkle some comments in verify_headers() and its callers documenting\nour assumptions and what's OK to do (string-like parsing functions work\nas long as they stop when they hit a newline). That, plus a few tests\ncovering the problematic cases to avoid regressions, would probably be\nOK.\n\nI think fsck_tree() is mostly fine, as the tree-iterating code detects\ntruncation. Though I do find the use of strlen() in decode_tree_entry()\na little suspicious (and that would be true of the current code, as\nwell, since it powers hash-object's existing parsing check).\n\n-Peff\n"},{"id":"470695","messageId":"Y8ik/3gPJHaAm8Jw@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hm4KKVwguGGrdn@nand.local","subject":"Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-19T02:03:43Z","receivedAt":"2023-01-19T02:04:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2023 at 04:38:40PM -0500, Taylor Blau wrote:\n\n> On Wed, Jan 18, 2023 at 12:59:24PM -0800, Junio C Hamano wrote:\n> > The --literally option was invented initially primarily to allow a\n> > bogus type of object (e.g. \"hash-object -t xyzzy --literally\") but I\n> > am happy to see that we are finding different uses. I wonder if\n> > these objects of known types but with syntactically bad contents can\n> > be \"repack\"ed from loose into packed?\n> >\n> > > [5/6]: fsck: provide a function to fsck buffer without object struct\n> \n> It is indeed possible:\n> \n> --- >8 ---\n> Initialized empty Git repository in /home/ttaylorr/src/git/t/trash directory.t9999-test/.git/\n> expecting success of 9999.1 'repacking corrupt loose object into packed':\n> \tname=$(echo $ZERO_OID | sed -e \"s/00/Q/g\") &&\n> \tprintf \"100644 fooQ$name\" | q_to_nul |\n> \t\tgit hash-object -w --stdin -t tree >in &&\n> \n> \tgit pack-objects .git/objects/pack/pack \n> Enumerating objects: 1, done.\n> Counting objects: 100% (1/1), done.\n> 06146c77fd19c096858d6459d602be0fdf10891b\n> Writing objects: 100% (1/1), done.\n> Total 1 (delta 0), reused 0 (delta 0), pack-reused 0\n> ok 1 - repacking corrupt loose object into packed\n> --- 8< ---\n\nRight, we don't do any fsck-ing when packing objects. Nor should we, I\nthink. We should be checking objects when they come into the repository\n(via index-pack/unpack-objects) or when they're created (hash-object),\nbut there's little need to do so when they migrate between storage\nformats.\n\nThe fact that \"--literally\" manually writes a loose object is mostly an\nimplementation detail. I think if we are not writing an object with an\nesoteric type, that it could even just hit the regular index_fd() code\npath (and drop the HASH_FORMAT_CHECK flag).\n\nIf you do write one with \"-t xyzzy\", I think pack-objects would barf,\nbut not because of fsck checks. It just couldn't represent that type\n(which really makes such objects pretty pointless; you cannot ever fetch\nor push them!).\n\n-Peff\n"},{"id":"470696","messageId":"Y8ilsLkR7R2uu542@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hiWGOArYqzen58@nand.local","subject":"Re: [PATCH 4/6] t: use hash-object --literally when created malformed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-19T02:06:40Z","receivedAt":"2023-01-19T02:06:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2023 at 04:19:20PM -0500, Taylor Blau wrote:\n\n> > This patch is worth looking at because it shows the kinds of things the\n> > new hash-object from patch 6 will reject.\n> \n> Obviously we could avoid this patch entirely by making the new behavior\n> of fscking the incoming objects hidden behind a `--fsck` flag or\n> something. But I think the decision not to is a good one.\n> \n> We already have `--literally`, and it makes sense that passing that\n> should let us write anything, and that not passing it should perform\n> some validity checks. But I think exactly *what* those checks are is\n> ambiguous enough that the absence of `--literally` implying fsck checks\n> isn't out of the question.\n> \n> You address this in the last patch more thoroughly, but I figure that it\n> is worth stating some of this here during review to indicate that I\n> think the direction you pursued here is a good one.\n\nThanks for raising this, I think it's a good thing to consider. I didn't\neven really think about making it a new option, since we already do\nquality checks (and loosen them via --literally). This just seemed like\nmore of the same.\n\nSo yeah, if there were people who really wanted to distinguish between\nthe severity of the old checks and the new ones, we can add --fsck (or\neven default to having it on, and disable it with --no-fsck to get the\nold checks). But I just see little point in that.\n\nOne thing we _could_ support that my patch doesn't (I think; I didn't\ntest very deeply here) is respecting individual fsck.msgType config\nvariables. Again, I don't really see much point there. If you know you\nare producing garbage, then just say --literally. The type-specific ones\nare useful when you have to hold your nose and accept somebody else's\nhistorical garbage, and you want to limit the damage as much as\npossible.\n\n-Peff\n"},{"id":"470697","messageId":"Y8il34JHhEqLkdnC@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hjiWUAIuiWfJoD@nand.local","subject":"Re: [PATCH 5/6] fsck: provide a function to fsck buffer without object struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-19T02:07:27Z","receivedAt":"2023-01-19T02:07:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2023 at 04:24:25PM -0500, Taylor Blau wrote:\n\n> On Wed, Jan 18, 2023 at 03:43:53PM -0500, Jeff King wrote:\n> > However, the only external interface that fsck.c provides is\n> > fsck_object(), which requires an object struct, then promptly discards\n> > everything except its oid and type. Let's factor out the post-discard\n> > part of that function as fsck_buffer(), leaving fsck_object() as a thin\n> > wrapper around it. That will provide more flexibility for callers which\n> > may not have a struct.\n> \n> It's really nice that the only thing we care about having an object\n> struct around for is basically just knowing its type. IOW it seems to\n> have made the refactoring here pretty straightforward, which is nice\n> ;-).\n\nYeah, it was always in the back of my mind while doing other fsck\nrefactors. But I have to admit that I was surprised that we were so\nclose to the finish line. :)\n\n-Peff\n"},{"id":"470698","messageId":"Y8iriP4T2FQPtBfF@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8hlyr0o6gs9omI5@nand.local","subject":"Re: [PATCH 6/6] hash-object: use fsck for object checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-19T02:31:36Z","receivedAt":"2023-01-19T02:31:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2023 at 04:34:02PM -0500, Taylor Blau wrote:\n\n> That being said, let me play devil's advocate for a second. Do the new\n> fsck checks slow anything in hash-object down significantly? If so, then\n> it's plausible to imagine a hash-object caller who (a) doesn't use\n> `--literally`, but (b) does care about throughput if they're writing a\n> large number of objects at once.\n> \n> I don't know if such a situation exists, or if these new fsck checks\n> even slow hash-object down enough to care. But I didn't catch a\n> discussion of this case in your series, so I figured I'd bring it up\n> here just in case.\n\nThat's a really good point to bring up.\n\nPrior to timing anything, here were my guesses:\n\n - it won't make a big difference either way because the time is\n dominated by computing sha1 anyway\n\n - we might actually be a little faster for commits and tags in the new\n code, because they aren't allocating structs for the pointed-to\n objects (trees, parents, etc). Nor stuffing them into obj_hash, so\n our total memory usage would be lower.\n\n - trees may be a little slower, because we're doing a more analysis on\n the filenames (sort order, various filesystem specific checks for\n .git, etc)\n\nAnd here's what I timed, using linux.git. First I pulled out the raw\nobject data like so:\n\n mkdir -p commit tag tree\n\n git cat-file --batch-all-objects --unordered --batch-check='%(objecttype) %(objectname)' |\n perl -alne 'print $F[1] unless $F[0] eq \"blob\"' |\n git cat-file --batch |\n perl -ne '\n /(\\S+) (\\S+) (\\d+)/ or die \"confusing: $_\";\n my $dir = \"$2/\" . substr($1, 0, 2);\n my $fn = \"$dir/\" . substr($1, 2);\n mkdir($dir);\n open(my $fh, \">\", $fn) or die \"open($fn): $!\";\n read(STDIN, my $buf, $3) or die \"read($3): $!\";\n print $fh $buf;\n read(STDIN, $buf, 1); # trailing newline\n '\n\nAnd then I timed it like this:\n\n find commit -type f | sort >input\n hyperfine -L v old,new './git.{v} hash-object --stdin-paths -t commit input\n hyperfine -L v old,new './git.{v} hash-object --stdin-paths -t tree On Wed, Jan 18, 2023 at 03:35:06PM -0500, Jeff King wrote:\n> \n> > The other option is having the fsck code avoid looking past the size it\n> > was given. I think the intent is that this should work, from commits\n> > like 4d0d89755e (Make sure fsck_commit_buffer() does not run out of the\n> > buffer, 2014-09-11). We do use skip_prefix() and parse_oid_hex(), which\n> > won't respect the size, but I think[1] that's OK because we'll have\n> > parsed up to the end-of-header beforehand (and those functions would\n> > never match past there).\n> > \n> > Which would mean that 9a1a3a4d4c (mktag: allow omitting the header/body\n> > \\n separator, 2021-01-05) and acf9de4c94 (mktag: use fsck instead of\n> > custom verify_tag(), 2021-01-05) were buggy, and we can just fix them.\n> > \n> > [1] But I said \"I think\" above because it can get pretty subtle. There's\n> > some more discussion in this thread:\n> > \n> > https://lore.kernel.org/git/20150625155128.C3E9738005C@gemini.denx.de/\n> > \n> > but I haven't yet convinced myself it's safe. This is exactly the\n> > kind of analysis I wish I had the power to nerd-snipe René into.\n> \n> I poked at this a bit more, and it definitely isn't safe.\n\nSo here's the result of my digging on this. The good news is that this\none commit on top of the rest of the series should make everything safe.\nI'm sorry the explanation is a bit long, but I wanted to capture a bit\nof the history, the subtle assumptions, and how I approached analyzing\nand fixing it.\n\nThere are a few paths forward here:\n\n - apply this on top of the earlier 6 patches. This is the simplest\n thing, and my preference. It does mean that t3800 temporarily has a\n read-one-char-past-buffer bug that is detected by ASan after patch 6\n but before this patch is applied.\n\n - put this fix first. Unfortunately the tests rely on having patch 6\n in order to be able to feed a non-NUL-terminated buffer to fsck.\n Options there are:\n\n - split this patch into two: code fix goes at the beginning of the\n series, and then the tests come at the end. The downside here is\n that it's very hard to run the tests on the pre-fixed code to\n verify that they are finding problems (you'd have to revert the\n fix, or re-order patches to get the broken state)\n\n - introduce a test-helper that lets you feed a buffer to\n fsck_buffer(). That can demonstrate the problem and fix\n independently of any hash-object changes. But it ends up being a\n fair bit of boilerplate, and ultimately we want to test\n hash-object anyway.\n\n - decide the whole \"make fsck work with arbitrary buffers\" thing is\n too subtle and error-prone. I don't think this, or else I wouldn't\n have made this patch. But I think it's an argument that can be made\n (and is roughly the approach we decided to take way back in the 2015\n thread linked above). The solution there is to make sure we\n NUL-terminate everything. As I said before, this is tricky because\n of mmap. But we could probably just skip using mmap in index_core()\n for non-blobs (which don't tend to be very big), and then assume\n fsck on individual blobs is safe (it is, because they won't have\n been marked as gitmodules, etc for more detailed scanning).\n\n I think it could work. I kind of prefer just making the fsck\n functions safe. Even though the way they do left-to-right scanning\n is error-prone, at least the ugliness is contained inside them,\n rather than this \"sure, I take a ptr/len combo, but make sure you\n allocate an extra NUL byte!\" assumption that currently exists.\n\nAnyway, here's the patch. I'm happy to repost the whole 7-patch series,\ntoo, but since the earlier ones didn't change in my preferred path\nforward, this seemed easier for now. ;)\n\n-- >8 --\nSubject: [PATCH] fsck: do not assume NUL-termination of buffers\n\nThe fsck code operates on an object buffer represented as a pointer/len\ncombination. However, the parsing of commits and tags is a little bit\nloose; we mostly scan left-to-right through the buffer, without checking\nwhether we've gone past the length we were given.\n\nThis has traditionally been OK because the buffers we feed to fsck\nalways have an extra NUL after the end of the object content, which ends\nany left-to-right scan. That has always been true for objects we read\nfrom the odb, and we made it true for incoming index-pack/unpack-objects\nchecks in a1e920a0a7 (index-pack: terminate object buffers with NUL,\n2014-12-08).\n\nHowever, we recently added an exception: hash-object asks index_fd() to\ndo fsck checks. That _may_ have an extra NUL (if we read from a pipe\ninto a strbuf), but it might not (if we read the contents from the\nfile). Nor can we just teach it to always add a NUL. We may mmap the\non-disk file, which will not have any extra bytes (if it's a multiple of\nthe page size). Not to mention that this is a rather subtle assumption\nfor the fsck code to make.\n\nInstead, let's make sure that the fsck parsers don't ever look past the\nsize of the buffer they've been given. This _almost_ works already,\nthanks to earlier work in 4d0d89755e (Make sure fsck_commit_buffer()\ndoes not run out of the buffer, 2014-09-11). The theory there is that we\ncheck up front whether we have the end of header double-newline\nseparator. And then any left-to-right scanning we do is OK as long as it\nstops when it hits that boundary.\n\nHowever, we later softened that in 84d18c0bcf (fsck: it is OK for a tag\nand a commit to lack the body, 2015-06-28), which allows the\ndouble-newline header to be missing, but does require that the header\nends in a newline. That was OK back then, because of the NUL-termination\nguarantees (including the one from a1e920a0a7 mentioned above).\n\nBecause 84d18c0bcf guarantees that any header line does end in a\nnewline, we are still OK with most of the left-to-right scanning. We\nonly need to take care after completing a line, to check that there is\nanother line (and we didn't run out of buffer).\n\nMost of these checks are just need to check \"buffer < buffer_end\" (where\nbuffer is advanced as we parse) before scanning for the next header\nline. But here are a few notes:\n\n - we don't technically need to check for remaining buffer before\n parsing the very first line (\"tree\" for a commit, or \"object\" for a\n tag), because verify_headers() rejects a totally empty buffer. But\n we'll do so in the name of consistency and defensiveness.\n\n - there are some calls to strchr('\\n'). These are actually OK by the\n \"the final header line must end in a newline\" guarantee from\n verify_headers(). They will always find that rather than run off the\n end of the buffer. Curiously, they do check for a NULL return and\n complain, but I believe that condition can never be reached.\n\n However, I converted them to use memchr() with a proper size and\n retained the NULL checks. Using memchr() is not much longer and\n makes it more obvious what is going on. Likewise, retaining the NULL\n checks serves as a defensive measure in case my analysis is wrong.\n\n - commit 9a1a3a4d4c (mktag: allow omitting the header/body \\n\n separator, 2021-01-05), does check for the end-of-buffer condition,\n but does so with \"!*buffer\", relying explicitly on the NUL\n termination. We can accomplish the same thing with a pointer\n comparison. I also folded it into the follow-on conditional that\n checks the contents of the buffer, for consistency with the other\n checks.\n\n - fsck_ident() uses parse_timestamp(), which is based on strtoumax().\n That function will happily skip past leading whitespace, including\n newlines, which makes it a risk. We can fix this by scanning to the\n first digit ourselves, and then using parse_timestamp() to do the\n actual numeric conversion.\n\n Note that as a side effect this fixes the fact that we missed\n zero-padded timestamps like \" 0123\" (whereas we would\n complain about \" 0123\"). I doubt anybody cares, but I\n mention it here for completeness.\n\n - fsck_tree() does not need any modifications. It relies on\n decode_tree_entry() to do the actual parsing, and that function\n checks both that there are enough bytes in the buffer to represent\n an entry, and that there is a NUL at the appropriate spot (one\n hash-length from the end; this may not be the NUL for the entry we\n are parsing, but we know that in the worst case, everything from our\n current position to that NUL is a filename, so we won't run out of\n bytes).\n\nIn addition to fixing the code itself, we'd like to make sure our rather\nsubtle assumptions are not violated in the future. So this patch does\ntwo more things:\n\n - add comments around verify_headers() documenting the link between\n what it checks and the memory safety of the callers. I don't expect\n this code to be modified frequently, but this may help somebody from\n accidentally breaking things.\n\n - add a thorough set of tests covering truncations at various key\n spots (e.g., for a \"tree $oid\" line, in the middle of the word\n \"tree\", right after it, after the space, in the middle of the $oid,\n and right at the end of the line. Most of these are fine already (it\n is only truncating right at the end of the line that is currently\n broken). And some of them are not even possible with the current\n code (we parse \"tree \" as a unit, so truncating before the space is\n equivalent). But I aimed here to consider the code a black box and\n look for any truncations that would be a problem for a left-to-right\n parser.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 67 ++++++++++++++++----\n t/t1451-fsck-buffer.sh | 140 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 194 insertions(+), 13 deletions(-)\n create mode 100755 t/t1451-fsck-buffer.sh\n\ndiff --git a/fsck.c b/fsck.c\nindex c2c8facd2d..2b18717ee8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -748,6 +748,23 @@ static int fsck_tree(const struct object_id *tree_oid,\n \treturn retval;\n }\n \n+/*\n+ * Confirm that the headers of a commit or tag object end in a reasonable way,\n+ * either with the usual \"\\n\\n\" separator, or at least with a trailing newline\n+ * on the final header line.\n+ *\n+ * This property is important for the memory safety of our callers. It allows\n+ * them to scan the buffer linewise without constantly checking the remaining\n+ * size as long as:\n+ *\n+ * - they check that there are bytes left in the buffer at the start of any\n+ * line (i.e., that the last newline they saw was not the final one we\n+ * found here)\n+ *\n+ * - any intra-line scanning they do will stop at a newline, which will worst\n+ * case hit the newline we found here as the end-of-header. This makes it\n+ * OK for them to use helpers like parse_oid_hex(), or even skip_prefix().\n+ */\n static int verify_headers(const void *data, unsigned long size,\n \t\t\t const struct object_id *oid, enum object_type type,\n \t\t\t struct fsck_options *options)\n@@ -808,6 +825,20 @@ static int fsck_ident(const char **ident,\n \tif (*p != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n \tp++;\n+\t/*\n+\t * Our timestamp parser is based on the C strto*() functions, which\n+\t * will happily eat whitespace, including the newline that is supposed\n+\t * to prevent us walking past the end of the buffer. So do our own\n+\t * scan, skipping linear whitespace but not newlines, and then\n+\t * confirming we found a digit. We _could_ be even more strict here,\n+\t * as we really expect only a single space, but since we have\n+\t * traditionally allowed extra whitespace, we'll continue to do so.\n+\t */\n+\twhile (*p == ' ' || *p == '\\t')\n+\t\tp++;\n+\tif (!isdigit(*p))\n+\t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE,\n+\t\t\t \"invalid author/committer line - bad date\");\n \tif (*p == '0' && p[1] != ' ')\n \t\treturn report(options, oid, type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n \tif (date_overflows(parse_timestamp(p, &end, 10)))\n@@ -834,20 +865,26 @@ static int fsck_commit(const struct object_id *oid,\n \tunsigned author_count;\n \tint err;\n \tconst char *buffer_begin = buffer;\n+\tconst char *buffer_end = buffer + size;\n \tconst char *p;\n \n+\t/*\n+\t * We _must_ stop parsing immediately if this reports failure, as the\n+\t * memory safety of the rest of the function depends on it. See the\n+\t * comment above the definition of verify_headers() for more details.\n+\t */\n \tif (verify_headers(buffer, size, oid, OBJ_COMMIT, options))\n \t\treturn -1;\n \n-\tif (!skip_prefix(buffer, \"tree \", &buffer))\n+\tif (buffer >= buffer_end || !skip_prefix(buffer, \"tree \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n \tif (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n') {\n \t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n \t\tif (err)\n \t\t\treturn err;\n \t}\n \tbuffer = p + 1;\n-\twhile (skip_prefix(buffer, \"parent \", &buffer)) {\n+\twhile (buffer < buffer_end && skip_prefix(buffer, \"parent \", &buffer)) {\n \t\tif (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n \t\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n \t\t\tif (err)\n@@ -856,7 +893,7 @@ static int fsck_commit(const struct object_id *oid,\n \t\tbuffer = p + 1;\n \t}\n \tauthor_count = 0;\n-\twhile (skip_prefix(buffer, \"author \", &buffer)) {\n+\twhile (buffer < buffer_end && skip_prefix(buffer, \"author \", &buffer)) {\n \t\tauthor_count++;\n \t\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n \t\tif (err)\n@@ -868,7 +905,7 @@ static int fsck_commit(const struct object_id *oid,\n \t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n \tif (err)\n \t\treturn err;\n-\tif (!skip_prefix(buffer, \"committer \", &buffer))\n+\tif (buffer >= buffer_end || !skip_prefix(buffer, \"committer \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n \terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n \tif (err)\n@@ -899,13 +936,19 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \tint ret = 0;\n \tchar *eol;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *buffer_end = buffer + size;\n \tconst char *p;\n \n+\t/*\n+\t * We _must_ stop parsing immediately if this reports failure, as the\n+\t * memory safety of the rest of the function depends on it. See the\n+\t * comment above the definition of verify_headers() for more details.\n+\t */\n \tret = verify_headers(buffer, size, oid, OBJ_TAG, options);\n \tif (ret)\n \t\tgoto done;\n \n-\tif (!skip_prefix(buffer, \"object \", &buffer)) {\n+\tif (buffer >= buffer_end || !skip_prefix(buffer, \"object \", &buffer)) {\n \t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_OBJECT, \"invalid format - expected 'object' line\");\n \t\tgoto done;\n \t}\n@@ -916,11 +959,11 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \t}\n \tbuffer = p + 1;\n \n-\tif (!skip_prefix(buffer, \"type \", &buffer)) {\n+\tif (buffer >= buffer_end || !skip_prefix(buffer, \"type \", &buffer)) {\n \t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TYPE_ENTRY, \"invalid format - expected 'type' line\");\n \t\tgoto done;\n \t}\n-\teol = strchr(buffer, '\\n');\n+\teol = memchr(buffer, '\\n', buffer_end - buffer);\n \tif (!eol) {\n \t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TYPE, \"invalid format - unexpected end after 'type' line\");\n \t\tgoto done;\n@@ -932,11 +975,11 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \t\tgoto done;\n \tbuffer = eol + 1;\n \n-\tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n+\tif (buffer >= buffer_end || !skip_prefix(buffer, \"tag \", &buffer)) {\n \t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAG_ENTRY, \"invalid format - expected 'tag' line\");\n \t\tgoto done;\n \t}\n-\teol = strchr(buffer, '\\n');\n+\teol = memchr(buffer, '\\n', buffer_end - buffer);\n \tif (!eol) {\n \t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAG, \"invalid format - unexpected end after 'type' line\");\n \t\tgoto done;\n@@ -952,18 +995,16 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \t}\n \tbuffer = eol + 1;\n \n-\tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n+\tif (buffer >= buffer_end || !skip_prefix(buffer, \"tagger \", &buffer)) {\n \t\t/* early tags do not contain 'tagger' lines; warn only */\n \t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAGGER_ENTRY, \"invalid format - expected 'tagger' line\");\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n \telse\n \t\tret = fsck_ident(&buffer, oid, OBJ_TAG, options);\n-\tif (!*buffer)\n-\t\tgoto done;\n \n-\tif (!starts_with(buffer, \"\\n\")) {\n+\tif (buffer < buffer_end && !starts_with(buffer, \"\\n\")) {\n \t\t/*\n \t\t * The verify_headers() check will allow\n \t\t * e.g. \"[...]tagger \\nsome\ndiff --git a/t/t1451-fsck-buffer.sh b/t/t1451-fsck-buffer.sh\nnew file mode 100755\nindex 0000000000..9ac270abab\n--- /dev/null\n+++ b/t/t1451-fsck-buffer.sh\n@@ -0,0 +1,140 @@\n+#!/bin/sh\n+\n+test_description='fsck on buffers without NUL termination\n+\n+The goal here is to make sure that the various fsck parsers never look\n+past the end of the buffer they are given, even when encountering broken\n+or truncated objects.\n+\n+We have to use \"hash-object\" for this because most code paths that read objects\n+append an extra NUL for safety after the buffer. But hash-object, since it is\n+reading straight from a file (and possibly even mmap-ing it) cannot always do\n+so.\n+\n+These tests _might_ catch such overruns in normal use, but should be run with\n+ASan or valgrind for more confidence.\n+'\n+. ./test-lib.sh\n+\n+# the general idea for tags and commits is to build up the \"base\" file\n+# progressively, and then test new truncations on top of it.\n+reset () {\n+\ttest_expect_success 'reset input to empty' '\n+\t\t>base\n+\t'\n+}\n+\n+add () {\n+\tcontent=\"$1\"\n+\ttype=${content%% *}\n+\ttest_expect_success \"add $type line\" '\n+\t\techo \"$content\" >>base\n+\t'\n+}\n+\n+check () {\n+\ttype=$1\n+\tfsck=$2\n+\tcontent=$3\n+\ttest_expect_success \"truncated $type ($fsck, \\\"$content\\\")\" '\n+\t\t# do not pipe into hash-object here; we want to increase\n+\t\t# the chance that it uses a fixed-size buffer or mmap,\n+\t\t# and a pipe would be read into a strbuf.\n+\t\t{\n+\t\t\tcat base &&\n+\t\t\techo \"$content\"\n+\t\t} >input &&\n+\t\ttest_must_fail git hash-object -t \"$type\" input 2>err &&\n+\t\tgrep \"$fsck\" err\n+\t'\n+}\n+\n+test_expect_success 'create valid objects' '\n+\tgit commit --allow-empty -m foo &&\n+\tcommit=$(git rev-parse --verify HEAD) &&\n+\ttree=$(git rev-parse --verify HEAD^{tree})\n+'\n+\n+reset\n+check commit missingTree \"\"\n+check commit missingTree \"tr\"\n+check commit missingTree \"tree\"\n+check commit badTreeSha1 \"tree \"\n+check commit badTreeSha1 \"tree 1234\"\n+add \"tree $tree\"\n+\n+# these expect missingAuthor because \"parent\" is optional\n+check commit missingAuthor \"\"\n+check commit missingAuthor \"par\"\n+check commit missingAuthor \"parent\"\n+check commit badParentSha1 \"parent \"\n+check commit badParentSha1 \"parent 1234\"\n+add \"parent $commit\"\n+\n+check commit missingAuthor \"\"\n+check commit missingAuthor \"au\"\n+check commit missingAuthor \"author\"\n+ident_checks () {\n+\tcheck $1 missingEmail \"$2 \"\n+\tcheck $1 missingEmail \"$2 name\"\n+\tcheck $1 badEmail \"$2 name <\"\n+\tcheck $1 badEmail \"$2 name \"\n+\tcheck $1 badDate \"$2 name \"\n+\tcheck $1 badDate \"$2 name 1234\"\n+\tcheck $1 badTimezone \"$2 name 1234 \"\n+\tcheck $1 badTimezone \"$2 name 1234 +\"\n+}\n+ident_checks commit author\n+add \"author name 1234 +0000\"\n+\n+check commit missingCommitter \"\"\n+check commit missingCommitter \"co\"\n+check commit missingCommitter \"committer\"\n+ident_checks commit committer\n+add \"committer name 1234 +0000\"\n+\n+reset\n+check tag missingObject \"\"\n+check tag missingObject \"obj\"\n+check tag missingObject \"object\"\n+check tag badObjectSha1 \"object \"\n+check tag badObjectSha1 \"object 1234\"\n+add \"object $commit\"\n+\n+check tag missingType \"\"\n+check tag missingType \"ty\"\n+check tag missingType \"type\"\n+check tag badType \"type \"\n+check tag badType \"type com\"\n+add \"type commit\"\n+\n+check tag missingTagEntry \"\"\n+check tag missingTagEntry \"ta\"\n+check tag missingTagEntry \"tag\"\n+check tag badTagName \"tag \"\n+add \"tag foo\"\n+\n+check tag missingTagger \"\"\n+check tag missingTagger \"ta\"\n+check tag missingTagger \"tagger\"\n+ident_checks tag tagger\n+\n+# trees are a binary format and can't use our earlier helpers\n+test_expect_success 'truncated tree (short hash)' '\n+\tprintf \"100644 foo\\0\\1\\1\\1\\1\" >input &&\n+\ttest_must_fail git hash-object -t tree input 2>err &&\n+\tgrep badTree err\n+'\n+\n+test_expect_success 'truncated tree (missing nul)' '\n+\t# these two things are indistinguishable to the parser. The important\n+\t# thing about this is example is that there are enough bytes to\n+\t# make up a hash, and that there is no NUL (and we confirm that the\n+\t# parser does not walk past the end of the buffer).\n+\tprintf \"100644 a long filename, or a hash with missing nul?\" >input &&\n+\ttest_must_fail git hash-object -t tree input 2>err &&\n+\tgrep badTree err\n+'\n+\n+test_done\n-- \n2.39.1.616.gd06fca9e99\n\n"},{"id":"470755","messageId":"xmqq358613h2.fsf@gitster.g","threadId":"59115","inReplyTo":"Y8nOmZHv7T843uBn@coredump.intra.peff.net","subject":"Re: [PATCH 7/6] fsck: do not assume NUL-termination of buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-19T23:58:33Z","receivedAt":"2023-01-19T23:58:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So here's the result of my digging on this. The good news is that this\n> one commit on top of the rest of the series should make everything safe.\n> I'm sorry the explanation is a bit long, but I wanted to capture a bit\n> of the history, the subtle assumptions, and how I approached analyzing\n> and fixing it.\n>\n> There are a few paths forward here:\n>\n> - apply this on top of the earlier 6 patches. This is the simplest\n> thing, and my preference. It does mean that t3800 temporarily has a\n> read-one-char-past-buffer bug that is detected by ASan after patch 6\n> but before this patch is applied.\n\nThat sounds reasonable, even though purist among us may find it\nslightly disturbing that it breaks \"bisectability\".\n\n> Anyway, here's the patch. I'm happy to repost the whole 7-patch series,\n> too, but since the earlier ones didn't change in my preferred path\n> forward, this seemed easier for now. ;)\n\nThanks. Will queue.\n\n"},{"id":"470835","messageId":"97faa323-a5b9-e459-70d7-3f6318446898@web.de","threadId":"59115","inReplyTo":"Y8ifa7hyqxSbL92U@coredump.intra.peff.net","subject":"Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-21T09:36:08Z","receivedAt":"2023-01-21T09:36:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.01.23 um 02:39 schrieb Jeff King:\n>\n> Though I do find the use of strlen() in decode_tree_entry()\n> a little suspicious (and that would be true of the current code, as\n> well, since it powers hash-object's existing parsing check).\n\nstrlen() won't overrun the buffer because the first check in\ndecode_tree_entry() makes sure there is a NUL in the buffer ahead.\nIf get_mode() crosses it then we exit early.\n\nStoring the result in an unsigned int can overflow on platforms where\nsize_t is bigger. That would result in pathlen values being too short\nfor really long paths, but no out-of-bounds access. They are then\nstored as signed int in struct name_entry and used as such in many\nplaces -- that seems like a bad idea, but I didn't actually check them\nthoroughly.\n\nget_mode() can overflow \"mode\" if there are too many octal digits. Do\nwe need to accept more than two handfuls in the first place? I'll send\na patch for at least rejecting overflow.\n\nHmm, what would be the performance impact of trees with mode fields\nzero-padded to silly lengths?\n\nRené\n"},{"id":"470872","messageId":"Y8zqZH+X6fOoCOYV@coredump.intra.peff.net","threadId":"59115","inReplyTo":"97faa323-a5b9-e459-70d7-3f6318446898@web.de","subject":"Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-22T07:48:52Z","receivedAt":"2023-01-22T07:48:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 21, 2023 at 10:36:08AM +0100, René Scharfe wrote:\n\n> Am 19.01.23 um 02:39 schrieb Jeff King:\n> >\n> > Though I do find the use of strlen() in decode_tree_entry()\n> > a little suspicious (and that would be true of the current code, as\n> > well, since it powers hash-object's existing parsing check).\n> \n> strlen() won't overrun the buffer because the first check in\n> decode_tree_entry() makes sure there is a NUL in the buffer ahead.\n> If get_mode() crosses it then we exit early.\n\nYeah, that was what I found after digging deeper (see my patch 7).\n\n> Storing the result in an unsigned int can overflow on platforms where\n> size_t is bigger. That would result in pathlen values being too short\n> for really long paths, but no out-of-bounds access. They are then\n> stored as signed int in struct name_entry and used as such in many\n> places -- that seems like a bad idea, but I didn't actually check them\n> thoroughly.\n\nYeah, I agree that the use of a signed int there looks questionable. I\ndo think it's orthogonal to my series here, as that tree-decoding is\nused by the existing hash-object checks.\n\nBut it probably bears further examination, especially because we use it\nfor the fsck checks on incoming objects via receive-pack, etc, which are\nmeant to be the first line of defense for hosters who might receive\nmalicious garbage from users.\n\nWe probably ought to reject trees with enormous names via fsck anyway. I\nactually have a patch to do that, but of course it depends on\ndecode_tree_entry() to get the length, so there's a bit of\nchicken-and-egg. We probably also should outright reject gigantic trees,\nwhich closes out a whole class of integer truncation problems. I know\nGitHub has rejected trees over 100MB for years for this reason.\n\n> get_mode() can overflow \"mode\" if there are too many octal digits. Do\n> we need to accept more than two handfuls in the first place? I'll send\n> a patch for at least rejecting overflow.\n\nSeems reasonable. I doubt there's an interesting attack here, just\nbecause the mode isn't used to index anything. If you feed a garbage\nmode that happens to overflow to something useful, you could just as\neasily have sent the useful mode in the first place.\n\n> Hmm, what would be the performance impact of trees with mode fields\n> zero-padded to silly lengths?\n\nCertainly it would waste some time parsing the tree, but you could do\nthat already with a long pathname. Or just having a lot of paths in a\ntree. Or a lot of trees.\n\n-Peff\n"},{"id":"470876","messageId":"d225dddc-973c-f710-9d24-cb53b26b973f@web.de","threadId":"59115","inReplyTo":"Y8zqZH+X6fOoCOYV@coredump.intra.peff.net","subject":"Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-22T11:39:26Z","receivedAt":"2023-01-22T11:39:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.01.23 um 08:48 schrieb Jeff King:\n> On Sat, Jan 21, 2023 at 10:36:08AM +0100, René Scharfe wrote:\n>\n>> Am 19.01.23 um 02:39 schrieb Jeff King:\n>>>\n>>> Though I do find the use of strlen() in decode_tree_entry()\n>>> a little suspicious (and that would be true of the current code, as\n>>> well, since it powers hash-object's existing parsing check).\n>>\n>> strlen() won't overrun the buffer because the first check in\n>> decode_tree_entry() makes sure there is a NUL in the buffer ahead.\n>> If get_mode() crosses it then we exit early.\n>\n> Yeah, that was what I found after digging deeper (see my patch 7).\n>\n>> Storing the result in an unsigned int can overflow on platforms where\n>> size_t is bigger. That would result in pathlen values being too short\n>> for really long paths, but no out-of-bounds access. They are then\n>> stored as signed int in struct name_entry and used as such in many\n>> places -- that seems like a bad idea, but I didn't actually check them\n>> thoroughly.\n>\n> Yeah, I agree that the use of a signed int there looks questionable. I\n> do think it's orthogonal to my series here, as that tree-decoding is\n> used by the existing hash-object checks.\n\nSure.\n\n> But it probably bears further examination, especially because we use it\n> for the fsck checks on incoming objects via receive-pack, etc, which are\n> meant to be the first line of defense for hosters who might receive\n> malicious garbage from users.\n>\n> We probably ought to reject trees with enormous names via fsck anyway. I\n> actually have a patch to do that, but of course it depends on\n> decode_tree_entry() to get the length, so there's a bit of\n> chicken-and-egg.\n\nSolvable by limiting the search for the end of the string in\ndecode_tree_entry() by using strnlen(3) or memchr(3) instead of\nstrlen(3). You just need to define some (configurable?) limit.\n\n> We probably also should outright reject gigantic trees,\n> which closes out a whole class of integer truncation problems. I know\n> GitHub has rejected trees over 100MB for years for this reason.\n\nMakes sense.\n\n>> get_mode() can overflow \"mode\" if there are too many octal digits. Do\n>> we need to accept more than two handfuls in the first place? I'll send\n>> a patch for at least rejecting overflow.\n>\n> Seems reasonable. I doubt there's an interesting attack here, just\n> because the mode isn't used to index anything. If you feed a garbage\n> mode that happens to overflow to something useful, you could just as\n> easily have sent the useful mode in the first place.\n>\n>> Hmm, what would be the performance impact of trees with mode fields\n>> zero-padded to silly lengths?\n>\n> Certainly it would waste some time parsing the tree, but you could do\n> that already with a long pathname. Or just having a lot of paths in a\n> tree. Or a lot of trees.\n\nThat's a cup half full/empty thing, perhaps. What's the harm in leading\nzeros? vs. Why allow leading zeros?\n\nRené\n"},{"id":"471239","messageId":"Y9pgG10dAoQABGXG@coredump.intra.peff.net","threadId":"59115","inReplyTo":"Y8haHL9xIWntSm0/@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] hash-object: use fsck for object checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-01T12:50:35Z","receivedAt":"2023-02-01T12:50:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2023 at 03:44:12PM -0500, Jeff King wrote:\n\n> @@ -2350,12 +2340,13 @@ static int index_mem(struct index_state *istate,\n> \t\t}\n> \t}\n> \tif (flags & HASH_FORMAT_CHECK) {\n> -\t\tif (type == OBJ_TREE)\n> -\t\t\tcheck_tree(buf, size);\n> -\t\tif (type == OBJ_COMMIT)\n> -\t\t\tcheck_commit(buf, size);\n> -\t\tif (type == OBJ_TAG)\n> -\t\t\tcheck_tag(buf, size);\n> +\t\tstruct fsck_options opts = FSCK_OPTIONS_DEFAULT;\n> +\n> +\t\topts.strict = 1;\n> +\t\topts.error_func = hash_format_check_report;\n> +\t\tif (fsck_buffer(null_oid(), type, buf, size, &opts))\n> +\t\t\tdie(_(\"refusing to create malformed object\"));\n> +\t\tfsck_finish(&opts);\n> \t}\n\nBy the way, I wanted to call out one thing here that nobody mentioned\nduring review: we are not checking the return value of fsck_finish().\n\nThat is a bit of a weird function. We must call it because it cleans up\nany resources allocated during the fsck_buffer() call. But it also is\nthe last chance to fsck any special blobs (like those that are found as\n.gitmodules, etc). We only find out the filenames while looking at the\nenclosing trees, so we queue them and then check the blobs later.\n\nSo if we are hashing a blob, that is mostly fine. We will not have the\nblob's name queued as anything special, and so the fsck is a noop.\n\nBut if we fsck a tree, and it has a .gitmodules entry pointing to blob\nX, then we would also pull X from the odb and fsck it during this\n\"finish\" phase.\n\nWhich leads me to two diverging lines of thought:\n\n 1. One of my goals with this series is that one could add objects to\n the repository via \"git hash-object -w\" and feel confident that no\n fsck rules were violated, because fsck implements some security\n checks. In the past when GitHub rolled out security checks this was\n a major pain, because objects enter repositories not just from\n pushes, but also from web-client activity (e.g., editing a blob on\n the website). And since Git had no way to say \"fsck just this\n object\", we ended up implementing the fsck checks multiple times,\n in libgit2 and in some of its calling code.\n\n So I was hoping that just passing the objects to \"hash-object\"\n would be a viable solution. I'm not sure if it is or not. If you\n just hash a blob, then we'll have no clue it's a .gitmodules file.\n OTOH, you have to get the matching tree which binds the blob to the\n .gitmodules path somehow. So if that tree is fsck'd, and then\n checks the blob during fsck_finish(), that should be enough.\n Assuming that fsck complains when the pointed-to blob cannot be\n accessed, which I think it should (because really, incremental\n pushes face the same problem).\n\n In which case we really ought to be checking the result of\n fsck_finish() here and complaining.\n\n 2. We're not checking fsck connectivity here, and that's intentional.\n So you can \"hash-object\" a tree that points to blobs that we don't\n actually have. But if you hash a tree that points a .gitmodules\n entry at a blob that doesn't exist, then that will fail the fsck\n (during the finish step). And respecting the fsck_finish() exit\n code would break that.\n\n As an addendum, in a regular fsck, many trees might mention the\n same blob as .gitmodules, and we'll queue that blob to be checked\n once. But here, we are potentially running a bunch of individual\n fscks, one per object we hash. So if you had, say, 1000 trees that\n all mentioned the same blob (because other entries were changing),\n and you tried to hash them all with \"hash-object --stdin-paths\" or\n similar, then we'd fsck that blob 1000 times.\n\n Which isn't wrong, per se, but seems inefficient. Solving it would\n require keeping track of what has been checked between calls to\n index_mem(). Which is kind of awkward, seeing as how low-level it\n is. It would be a lot more natural if all this checking happened in\n hash-object itself.\n\nSo I dunno. The code above is doing (2), albeit with the inefficiency of\nchecking blobs that we might not care about. I kind of think (1) is the\nright thing, though, and anybody who really wants to make trees that\npoint to bogus .gitmodules can use --literally.\n\n-Peff\n"},{"id":"471242","messageId":"230201.86h6w5r07w.gmgdl@evledraar.gmail.com","threadId":"59115","inReplyTo":"Y9pgG10dAoQABGXG@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] hash-object: use fsck for object checks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-01T13:08:39Z","receivedAt":"2023-02-01T13:20:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 01 2023, Jeff King wrote:\n\n> On Wed, Jan 18, 2023 at 03:44:12PM -0500, Jeff King wrote:\n>\n>> @@ -2350,12 +2340,13 @@ static int index_mem(struct index_state *istate,\n>> \t\t}\n>> \t}\n>> \tif (flags & HASH_FORMAT_CHECK) {\n>> -\t\tif (type == OBJ_TREE)\n>> -\t\t\tcheck_tree(buf, size);\n>> -\t\tif (type == OBJ_COMMIT)\n>> -\t\t\tcheck_commit(buf, size);\n>> -\t\tif (type == OBJ_TAG)\n>> -\t\t\tcheck_tag(buf, size);\n>> +\t\tstruct fsck_options opts = FSCK_OPTIONS_DEFAULT;\n>> +\n>> +\t\topts.strict = 1;\n>> +\t\topts.error_func = hash_format_check_report;\n>> +\t\tif (fsck_buffer(null_oid(), type, buf, size, &opts))\n>> +\t\t\tdie(_(\"refusing to create malformed object\"));\n>> +\t\tfsck_finish(&opts);\n>> \t}\n>\n> By the way, I wanted to call out one thing here that nobody mentioned\n> during review: we are not checking the return value of fsck_finish().\n>\n> That is a bit of a weird function. We must call it because it cleans up\n> any resources allocated during the fsck_buffer() call. But it also is\n> the last chance to fsck any special blobs (like those that are found as\n> .gitmodules, etc). We only find out the filenames while looking at the\n> enclosing trees, so we queue them and then check the blobs later.\n>\n> So if we are hashing a blob, that is mostly fine. We will not have the\n> blob's name queued as anything special, and so the fsck is a noop.\n>\n> But if we fsck a tree, and it has a .gitmodules entry pointing to blob\n> X, then we would also pull X from the odb and fsck it during this\n> \"finish\" phase.\n>\n> Which leads me to two diverging lines of thought:\n>\n> 1. One of my goals with this series is that one could add objects to\n> the repository via \"git hash-object -w\" and feel confident that no\n> fsck rules were violated, because fsck implements some security\n> checks. In the past when GitHub rolled out security checks this was\n> a major pain, because objects enter repositories not just from\n> pushes, but also from web-client activity (e.g., editing a blob on\n> the website). And since Git had no way to say \"fsck just this\n> object\", we ended up implementing the fsck checks multiple times,\n> in libgit2 and in some of its calling code.\n>\n> So I was hoping that just passing the objects to \"hash-object\"\n> would be a viable solution. I'm not sure if it is or not. If you\n> just hash a blob, then we'll have no clue it's a .gitmodules file.\n> OTOH, you have to get the matching tree which binds the blob to the\n> .gitmodules path somehow. So if that tree is fsck'd, and then\n> checks the blob during fsck_finish(), that should be enough.\n> Assuming that fsck complains when the pointed-to blob cannot be\n> accessed, which I think it should (because really, incremental\n> pushes face the same problem).\n>\n> In which case we really ought to be checking the result of\n> fsck_finish() here and complaining.\n>\n> 2. We're not checking fsck connectivity here, and that's intentional.\n> So you can \"hash-object\" a tree that points to blobs that we don't\n> actually have. But if you hash a tree that points a .gitmodules\n> entry at a blob that doesn't exist, then that will fail the fsck\n> (during the finish step). And respecting the fsck_finish() exit\n> code would break that.\n>\n> As an addendum, in a regular fsck, many trees might mention the\n> same blob as .gitmodules, and we'll queue that blob to be checked\n> once. But here, we are potentially running a bunch of individual\n> fscks, one per object we hash. So if you had, say, 1000 trees that\n> all mentioned the same blob (because other entries were changing),\n> and you tried to hash them all with \"hash-object --stdin-paths\" or\n> similar, then we'd fsck that blob 1000 times.\n>\n> Which isn't wrong, per se, but seems inefficient. Solving it would\n> require keeping track of what has been checked between calls to\n> index_mem(). Which is kind of awkward, seeing as how low-level it\n> is. It would be a lot more natural if all this checking happened in\n> hash-object itself.\n>\n> So I dunno. The code above is doing (2), albeit with the inefficiency of\n> checking blobs that we might not care about. I kind of think (1) is the\n> right thing, though, and anybody who really wants to make trees that\n> point to bogus .gitmodules can use --literally.\n\nAside from the other things you bring up here, it seems wrong to me to\nconflate --literally with some sort of \"no fsck\" or \"don't fsck this\ncollection yet\" mode.\n\nCan't we have a \"--no-fsck\" or similar, which won't do any sort of full\nfsck, but also won't accept bogus object types & the like?\n\nCurrently I believe (and I haven't had time to carefully review what you\nhave here) we only need --literally to produce objects that are truly\ncorrupt when viewed in isolation. E.g. a tag that refers to a bogus\nobject type etc.\n\nBut we have long supported a narrow view of what the fsck checks mean in\nthat context. E.g. now with \"mktag\" we'll use the fsck machinery, but\nonly skin-deep, so you can be referring to a tree which would in turn\nfail our checks.\n\nI tend to think that we should be keeping it like that, but documenting\nthat if you're creating such objects you either need to do it really\ncarefully, or follow it up with an operation that's guaranteed to fsck\nthe sum of the objects you've added recursively.\n\nSo, rather than teach e.g. \"hash-object\" to be smart about that we\nshould e.g. encourage a manual creation of trees/blobs/commits to be\nfollowed-up with a \"git push\" to a new ref that refers to them, even if\nthat \"git push\" is to the repository located in the $PWD.\n\nBy doing that we offload the \"what's new?\" question to the\npack-over-the-wire machinery, which is well tested.\n\nAnything else seems ultimately to be madness, after all if I feed a\nnewly crafted commit to \"hash-object\" how do we know where to stop,\nother than essentially faking up a push negotiation with ourselves?\n\nIt's also worth noting that much of the complexity around .gitmodules in\nparticular is to support packfile-uri's odd notion of applying the\n\"last\" part of the PACK before the \"first\" part, which nothing else\ndoes.\n\nWhich, if we just blindly applied both, and then fsck'd the resulting\ncombination we'd get rid of that tricky special-case. But I haven't\nbenchmarked that. It should be a bit slower, particularly on a large\nrepository that won't fit in memory. But my hunch is that it won't be\ntoo bad, and the resulting simplification may be worth it (particularly\nnow that we have bundle-uri, which doesn't share that edge-case).\n\n"},{"id":"471244","messageId":"230201.868rhhqxp1.gmgdl@evledraar.gmail.com","threadId":"59115","inReplyTo":"d225dddc-973c-f710-9d24-cb53b26b973f@web.de","subject":"Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-01T14:06:39Z","receivedAt":"2023-02-01T14:15:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jan 22 2023, René Scharfe wrote:\n\n> Am 22.01.23 um 08:48 schrieb Jeff King:\n>> We probably also should outright reject gigantic trees,\n>> which closes out a whole class of integer truncation problems. I know\n>> GitHub has rejected trees over 100MB for years for this reason.\n>\n> Makes sense.\n\nI really don't think it does, let's not forever encode arbitrary limits\nin the formats because of transitory implementation details.\n\nThose sort of arbitrary limits are understandable for hosting providers,\nand a sensible trade-off on that front.\n\nBut for git as a general tool? I'd like to be able to throw whatever\ngarbage I've got locally at it, and not have it complain.\n\nWe already have a deluge of int v.s. unsigned int v.s. size_t warnings\nthat we could address, we're just choosing to suppress them\ncurrently. Instead we have hacks like cast_size_t_to_int().\n\nThose sorts of hacks are understandable as band-aid fixes, but let's\nwork on fixing the real causes.\n"},{"id":"471265","messageId":"xmqqlelhyv81.fsf@gitster.g","threadId":"59115","inReplyTo":"Y9pgG10dAoQABGXG@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] hash-object: use fsck for object checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-01T20:41:18Z","receivedAt":"2023-02-01T20:42:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... So if that tree is fsck'd, and then\n> checks the blob during fsck_finish(), that should be enough.\n> Assuming that fsck complains when the pointed-to blob cannot be\n> accessed, which I think it should (because really, incremental\n> pushes face the same problem).\n\nYes.\n\n> 2. We're not checking fsck connectivity here, and that's intentional.\n> So you can \"hash-object\" a tree that points to blobs that we don't\n> actually have. But if you hash a tree that points a .gitmodules\n> entry at a blob that doesn't exist, then that will fail the fsck\n> (during the finish step). And respecting the fsck_finish() exit\n> code would break that.\n\nThat's tricky. An attack vector to sneak a bad .gitmodules file\ninto history then becomes (1) hash a tree with a .gitmodules entry\nthat points at a missing blob and then (2) after that fact is\nforgotten, hash a bad blob pointed to by the tree? We cannot afford\nto remember all trees with .gitmodules we didn't find the blob for\nforever, so one approach to solve it is to reject trees with missing\nblobs. Legitimate use cases should be able to build up trees bottle\nup to hash blobs before their containing trees.\n\nIf you hash a commit object, we would want to fsck its tree? Do we\nwant to fsck its parent commit and its tree? Ideally we can stop\nwhen our \"traversal\" reaches objects that are known to be good, but\nhow do we decide which objects are \"known to be good\"? Being\nreachable from our refs, as usual?\n\n> So I dunno. The code above is doing (2), albeit with the inefficiency of\n> checking blobs that we might not care about. I kind of think (1) is the\n> right thing, though, and anybody who really wants to make trees that\n> point to bogus .gitmodules can use --literally.\n\nTrue.\n"}]}