{"thread":{"id":"65051","subject":"[PATCH 0/4] pack-check: fix verification of large objects","startedAt":"2026-02-23T09:50:48Z","lastAt":"2026-02-24T06:26:52Z","messageCount":26,"participants":["Patrick Steinhardt","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"536728","messageId":"20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im","threadId":"65051","inReplyTo":null,"subject":"[PATCH 0/4] pack-check: fix verification of large objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T09:50:39Z","receivedAt":"2026-02-23T09:50:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series addresses the bug reported by brian in [1].\nThanks!\n\nPatrick\n\n[1]: <20260222183710.2963424-1-sandals@crustytoothpaste.net>\n\n---\nPatrick Steinhardt (4):\n t/helper: improve \"genrandom\" test helper\n object-file: adapt `stream_object_signature()` to take a stream\n packfile: expose function to read object stream for an offset\n pack-check: fix verification of large objects\n\n object-file.c | 10 +++-------\n object-file.h | 4 +++-\n object.c | 15 ++++++++++++---\n pack-check.c | 12 +++++++++---\n packfile.c | 36 +++++++++++++++++++----------------\n packfile.h | 4 ++++\n t/helper/test-genrandom.c | 5 ++++-\n t/t1006-cat-file.sh | 2 +-\n t/t1050-large.sh | 6 +++---\n t/t1450-fsck.sh | 17 ++++++++++++++++-\n t/t5301-sliding-window.sh | 2 +-\n t/t5310-pack-bitmaps.sh | 2 +-\n t/t5710-promisor-remote-capability.sh | 4 ++--\n t/t7700-repack.sh | 6 +++---\n 14 files changed, 82 insertions(+), 43 deletions(-)\n\n\n---\nbase-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4\nchange-id: 20260223-pks-fsck-fix-aa8a18a223c8\n\n"},{"id":"536729","messageId":"20260223-pks-fsck-fix-v1-1-c29036832b6e@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im","subject":"[PATCH 1/4] t/helper: improve \"genrandom\" test helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T09:50:40Z","receivedAt":"2026-02-23T09:50:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `test-tool genrandom` test helper can be used to generate random\ndata, either as an infinite stream or with a specified number of bytes.\nThe way we handle parsing the number of bytes is lacking though:\n\n - We don't have good error handling, so if the caller for example uses\n `test-tool genrandom 200xyz` then we'll end up generating 200 bytes\n of random data successfully.\n\n - Many callers want to generate e.g. 1 kilobyte or megabyte of data,\n but they have to either use unwieldy numbers like 1048576, or they\n have to precompute them.\n\nFix both of these issues by using `git_parse_ulong()` to parse the\nargumemnt. This function has better error handling, and it knows to\nhandle unit suffixes.\n\nAdapt a couple of our tests to use suffixes instead of manual\ncomputations.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/helper/test-genrandom.c | 5 ++++-\n t/t1006-cat-file.sh | 2 +-\n t/t1050-large.sh | 6 +++---\n t/t1450-fsck.sh | 2 +-\n t/t5301-sliding-window.sh | 2 +-\n t/t5310-pack-bitmaps.sh | 2 +-\n t/t5710-promisor-remote-capability.sh | 4 ++--\n t/t7700-repack.sh | 6 +++---\n 8 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/t/helper/test-genrandom.c b/t/helper/test-genrandom.c\nindex 51b67f2f87..77dc31a315 100644\n--- a/t/helper/test-genrandom.c\n+++ b/t/helper/test-genrandom.c\n@@ -6,6 +6,7 @@\n \n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n+#include \"parse.h\"\n \n int cmd__genrandom(int argc, const char **argv)\n {\n@@ -22,7 +23,9 @@ int cmd__genrandom(int argc, const char **argv)\n \t\tnext = next * 11 + *c;\n \t} while (*c++);\n \n-\tcount = (argc == 3) ? strtoul(argv[2], NULL, 0) : ULONG_MAX;\n+\tcount = ULONG_MAX;\n+\tif (argc == 3 && git_parse_ulong(argv[2], &count) < 0)\n+\t\treturn error_errno(\"cannot parse argument '%s'\", argv[2]);\n \n \twhile (count--) {\n \t\tnext = next * 1103515245 + 12345;\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 0eee3bb878..5499be8dc9 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -643,7 +643,7 @@ test_expect_success 'object reference via commit text search' '\n '\n \n test_expect_success 'setup blobs which are likely to delta' '\n-\ttest-tool genrandom foo 10240 >foo &&\n+\ttest-tool genrandom foo 10k >foo &&\n \t{ cat foo && echo plus; } >foo-plus &&\n \tgit add foo foo-plus &&\n \tgit commit -m foo &&\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex 5be273611a..7d40d08521 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -104,9 +104,9 @@ test_expect_success 'packsize limit' '\n \t\t# mid1 and mid2 will fit within 256k limit but\n \t\t# appending mid3 will bust the limit and will\n \t\t# result in a separate packfile.\n-\t\ttest-tool genrandom \"a\" $(( 66 * 1024 )) >mid1 &&\n-\t\ttest-tool genrandom \"b\" $(( 80 * 1024 )) >mid2 &&\n-\t\ttest-tool genrandom \"c\" $(( 128 * 1024 )) >mid3 &&\n+\t\ttest-tool genrandom \"a\" 66k >mid1 &&\n+\t\ttest-tool genrandom \"b\" 80k >mid2 &&\n+\t\ttest-tool genrandom \"c\" 128k >mid3 &&\n \t\tgit add mid1 mid2 mid3 &&\n \n \t\tcount=0 &&\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 3fae05f9d9..8fb79b3e5d 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -918,7 +918,7 @@ test_expect_success 'fsck detects trailing loose garbage (large blob)' '\n test_expect_success 'fsck detects truncated loose object' '\n \t# make it big enough that we know we will truncate in the data\n \t# portion, not the header\n-\ttest-tool genrandom truncate 4096 >file &&\n+\ttest-tool genrandom truncate 4k >file &&\n \tblob=$(git hash-object -w file) &&\n \tfile=$(sha1_file $blob) &&\n \ttest_when_finished \"remove_object $blob\" &&\ndiff --git a/t/t5301-sliding-window.sh b/t/t5301-sliding-window.sh\nindex ff6b5159a3..3c3666b278 100755\n--- a/t/t5301-sliding-window.sh\n+++ b/t/t5301-sliding-window.sh\n@@ -12,7 +12,7 @@ test_expect_success 'setup' '\n \tfor i in a b c\n \tdo\n \techo $i >$i &&\n-\ttest-tool genrandom \"$i\" 32768 >>$i &&\n+\ttest-tool genrandom \"$i\" 32k >>$i &&\n \tgit update-index --add $i || return 1\n \tdone &&\n \techo d >d && cat c >>d && git update-index --add d &&\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 6718fb98c0..3e3366f57d 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -242,7 +242,7 @@ test_bitmap_cases () {\n \t'\n \n \ttest_expect_success 'splitting packs does not generate bogus bitmaps' '\n-\t\ttest-tool genrandom foo $((1024 * 1024)) >rand &&\n+\t\ttest-tool genrandom foo 1m >rand &&\n \t\tgit add rand &&\n \t\tgit commit -m \"commit with big file\" &&\n \t\tgit -c pack.packSizeLimit=500k repack -adb &&\ndiff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh\nindex 023735d6a8..66af84cd56 100755\n--- a/t/t5710-promisor-remote-capability.sh\n+++ b/t/t5710-promisor-remote-capability.sh\n@@ -20,7 +20,7 @@ test_expect_success 'setup: create \"template\" repository' '\n \ttest_commit -C template 1 &&\n \ttest_commit -C template 2 &&\n \ttest_commit -C template 3 &&\n-\ttest-tool genrandom foo 10240 >template/foo &&\n+\ttest-tool genrandom foo 10k >template/foo &&\n \tgit -C template add foo &&\n \tgit -C template commit -m foo\n '\n@@ -376,7 +376,7 @@ test_expect_success \"clone with promisor.advertise set to 'true' but don't delet\n \n test_expect_success \"setup for subsequent fetches\" '\n \t# Generate new commit with large blob\n-\ttest-tool genrandom bar 10240 >template/bar &&\n+\ttest-tool genrandom bar 10k >template/bar &&\n \tgit -C template add bar &&\n \tgit -C template commit -m bar &&\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 73b78bdd88..439ab24d23 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -319,7 +319,7 @@ test_expect_success 'no bitmaps created if .keep files present' '\n \n test_expect_success 'auto-bitmaps do not complain if unavailable' '\n \ttest_config -C bare.git pack.packSizeLimit 1M &&\n-\tblob=$(test-tool genrandom big $((1024*1024)) |\n+\tblob=$(test-tool genrandom big 1m |\n \t git -C bare.git hash-object -w --stdin) &&\n \tgit -C bare.git update-ref refs/tags/big $blob &&\n \n@@ -495,9 +495,9 @@ test_expect_success '--filter works with --max-pack-size' '\n \t\tcd max-pack-size &&\n \t\ttest_commit base &&\n \t\t# two blobs which exceed the maximum pack size\n-\t\ttest-tool genrandom foo 1048576 >foo &&\n+\t\ttest-tool genrandom foo 1m >foo &&\n \t\tgit hash-object -w foo &&\n-\t\ttest-tool genrandom bar 1048576 >bar &&\n+\t\ttest-tool genrandom bar 1m >bar &&\n \t\tgit hash-object -w bar &&\n \t\tgit add foo bar &&\n \t\tgit commit -m \"adding foo and bar\"\n\n-- \n2.53.0.414.gf7e9f6c205.dirty\n\n"},{"id":"536730","messageId":"20260223-pks-fsck-fix-v1-2-c29036832b6e@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im","subject":"[PATCH 2/4] object-file: adapt `stream_object_signature()` to take a stream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T09:50:41Z","receivedAt":"2026-02-23T09:50:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `stream_object_signature()` is responsible for verifying\nwhether the given object ID matches the actual hash of the object's\ncontents. In contrast to `check_object_signature()` it does so in a\nstreaming fashion so that we don't have to load the full object into\nmemory.\n\nIn a subsequent commit we'll want to adapt one of its callsites to pass\na preconstructed stream. Prepare for this by accepting a stream as input\nthat the caller needs to assemble.\n\nSigned-off-by: Patrick Steinhardt \n---\n object-file.c | 10 +++-------\n object-file.h | 4 +++-\n object.c | 15 ++++++++++++---\n pack-check.c | 12 +++++++++---\n 4 files changed, 27 insertions(+), 14 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 1b62996ef0..ca2c4dddf3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -129,18 +129,15 @@ int check_object_signature(struct repository *r, const struct object_id *oid,\n \treturn !oideq(oid, &real_oid) ? -1 : 0;\n }\n \n-int stream_object_signature(struct repository *r, const struct object_id *oid)\n+int stream_object_signature(struct repository *r,\n+\t\t\t struct odb_read_stream *st,\n+\t\t\t const struct object_id *oid)\n {\n \tstruct object_id real_oid;\n-\tstruct odb_read_stream *st;\n \tstruct git_hash_ctx c;\n \tchar hdr[MAX_HEADER_LEN];\n \tint hdrlen;\n \n-\tst = odb_read_stream_open(r->objects, oid, NULL);\n-\tif (!st)\n-\t\treturn -1;\n-\n \t/* Generate the header */\n \thdrlen = format_object_header(hdr, sizeof(hdr), st->type, st->size);\n \n@@ -160,7 +157,6 @@ int stream_object_signature(struct repository *r, const struct object_id *oid)\n \t\tgit_hash_update(&c, buf, readlen);\n \t}\n \tgit_hash_final_oid(&real_oid, &c);\n-\todb_read_stream_close(st);\n \treturn !oideq(oid, &real_oid) ? -1 : 0;\n }\n \ndiff --git a/object-file.h b/object-file.h\nindex a62d0de394..733d232309 100644\n--- a/object-file.h\n+++ b/object-file.h\n@@ -164,7 +164,9 @@ int check_object_signature(struct repository *r, const struct object_id *oid,\n * Try reading the object named with \"oid\" using\n * the streaming interface and rehash it to do the same.\n */\n-int stream_object_signature(struct repository *r, const struct object_id *oid);\n+int stream_object_signature(struct repository *r,\n+\t\t\t struct odb_read_stream *stream,\n+\t\t\t const struct object_id *oid);\n \n enum finalize_object_file_flags {\n \tFOF_SKIP_COLLISION_CHECK = 1,\ndiff --git a/object.c b/object.c\nindex 4669b8d65e..56d79d77b4 100644\n--- a/object.c\n+++ b/object.c\n@@ -6,6 +6,7 @@\n #include \"object.h\"\n #include \"replace-object.h\"\n #include \"object-file.h\"\n+#include \"odb/streaming.h\"\n #include \"blob.h\"\n #include \"statinfo.h\"\n #include \"tree.h\"\n@@ -330,9 +331,17 @@ struct object *parse_object_with_flags(struct repository *r,\n \n \tif ((!obj || obj->type == OBJ_NONE || obj->type == OBJ_BLOB) &&\n \t odb_read_object_info(r->objects, oid, NULL) == OBJ_BLOB) {\n-\t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n-\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n-\t\t\treturn NULL;\n+\t\tif (!skip_hash) {\n+\t\t\tstruct odb_read_stream *stream = odb_read_stream_open(r->objects, oid, NULL);\n+\t\t\tif (!stream || stream_object_signature(r, stream, repl) < 0) {\n+\t\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n+\t\t\t\tif (stream)\n+\t\t\t\t\todb_read_stream_close(stream);\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\n+\t\t\tif (stream)\n+\t\t\t\todb_read_stream_close(stream);\n \t\t}\n \t\tparse_blob_buffer(lookup_blob(r, oid));\n \t\treturn lookup_object(r, oid);\ndiff --git a/pack-check.c b/pack-check.c\nindex 67cb2cf72f..46782a29d5 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -9,6 +9,7 @@\n #include \"packfile.h\"\n #include \"object-file.h\"\n #include \"odb.h\"\n+#include \"odb/streaming.h\"\n \n struct idx_entry {\n \toff_t offset;\n@@ -104,6 +105,7 @@ static int verify_packfile(struct repository *r,\n \tQSORT(entries, nr_objects, compare_entries);\n \n \tfor (i = 0; i < nr_objects; i++) {\n+\t\tstruct odb_read_stream *stream = NULL;\n \t\tvoid *data;\n \t\tstruct object_id oid;\n \t\tenum object_type type;\n@@ -152,7 +154,9 @@ static int verify_packfile(struct repository *r,\n \t\t\t\t\t\t\ttype) < 0)\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\n-\t\telse if (!data && stream_object_signature(r, &oid) < 0)\n+\t\telse if (!data &&\n+\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n+\t\t\t stream_object_signature(r, stream, &oid) < 0))\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\n \t\telse if (fn) {\n@@ -163,12 +167,14 @@ static int verify_packfile(struct repository *r,\n \t\t}\n \t\tif (((base_count + i) & 1023) == 0)\n \t\t\tdisplay_progress(progress, base_count + i);\n-\t\tfree(data);\n \n+\t\tif (stream)\n+\t\t\todb_read_stream_close(stream);\n+\t\tfree(data);\n \t}\n+\n \tdisplay_progress(progress, base_count + i);\n \tfree(entries);\n-\n \treturn err;\n }\n \n\n-- \n2.53.0.414.gf7e9f6c205.dirty\n\n"},{"id":"536731","messageId":"20260223-pks-fsck-fix-v1-3-c29036832b6e@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im","subject":"[PATCH 3/4] packfile: expose function to read object stream for an offset","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T09:50:42Z","receivedAt":"2026-02-23T09:50:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `packfile_store_read_object_stream()` takes as input an\nobject ID and then constructs a `struct odb_read_stream` from it. In a\nsubsequent commit we'll want to create an object stream for a given\ncombination of packfile and offset though, which is not something that\ncan currently be done.\n\nExtract a new function `packfile_read_object_stream()` that makes this\nfunctionality available.\n\nSigned-off-by: Patrick Steinhardt \n---\n packfile.c | 36 ++++++++++++++++++++----------------\n packfile.h | 4 ++++\n 2 files changed, 24 insertions(+), 16 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 402c3b5dc7..9d795a671f 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2553,29 +2553,21 @@ static int close_istream_pack_non_delta(struct odb_read_stream *_st)\n \treturn 0;\n }\n \n-int packfile_store_read_object_stream(struct odb_read_stream **out,\n-\t\t\t\t struct packfile_store *store,\n-\t\t\t\t const struct object_id *oid)\n+int packfile_read_object_stream(struct odb_read_stream **out,\n+\t\t\t\tstruct packed_git *pack,\n+\t\t\t\toff_t offset)\n {\n \tstruct odb_packed_read_stream *stream;\n \tstruct pack_window *window = NULL;\n-\tstruct object_info oi = OBJECT_INFO_INIT;\n \tenum object_type in_pack_type;\n \tunsigned long size;\n \n-\toi.sizep = &size;\n+\tin_pack_type = unpack_object_header(pack, &window, &offset, &size);\n+\tunuse_pack(&window);\n \n-\tif (packfile_store_read_object_info(store, oid, &oi, 0) ||\n-\t oi.u.packed.type == PACKED_OBJECT_TYPE_REF_DELTA ||\n-\t oi.u.packed.type == PACKED_OBJECT_TYPE_OFS_DELTA ||\n-\t repo_settings_get_big_file_threshold(store->source->odb->repo) >= size)\n+\tif (repo_settings_get_big_file_threshold(pack->repo) >= size)\n \t\treturn -1;\n \n-\tin_pack_type = unpack_object_header(oi.u.packed.pack,\n-\t\t\t\t\t &window,\n-\t\t\t\t\t &oi.u.packed.offset,\n-\t\t\t\t\t &size);\n-\tunuse_pack(&window);\n \tswitch (in_pack_type) {\n \tdefault:\n \t\treturn -1; /* we do not do deltas for now */\n@@ -2592,10 +2584,22 @@ int packfile_store_read_object_stream(struct odb_read_stream **out,\n \tstream->base.type = in_pack_type;\n \tstream->base.size = size;\n \tstream->z_state = ODB_PACKED_READ_STREAM_UNINITIALIZED;\n-\tstream->pack = oi.u.packed.pack;\n-\tstream->pos = oi.u.packed.offset;\n+\tstream->pack = pack;\n+\tstream->pos = offset;\n \n \t*out = &stream->base;\n \n \treturn 0;\n }\n+\n+int packfile_store_read_object_stream(struct odb_read_stream **out,\n+\t\t\t\t struct packfile_store *store,\n+\t\t\t\t const struct object_id *oid)\n+{\n+\tstruct pack_entry e;\n+\n+\tif (!find_pack_entry(store, oid, &e))\n+\t\treturn -1;\n+\n+\treturn packfile_read_object_stream(out, e.p, e.offset);\n+}\ndiff --git a/packfile.h b/packfile.h\nindex acc5c55ad5..67d5750140 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -436,6 +436,10 @@ off_t get_delta_base(struct packed_git *p, struct pack_window **w_curs,\n \t\t off_t *curpos, enum object_type type,\n \t\t off_t delta_obj_offset);\n \n+int packfile_read_object_stream(struct odb_read_stream **out,\n+\t\t\t\tstruct packed_git *pack,\n+\t\t\t\toff_t offset);\n+\n void release_pack_memory(size_t);\n \n /* global flag to enable extra checks when accessing packed objects */\n\n-- \n2.53.0.414.gf7e9f6c205.dirty\n\n"},{"id":"536732","messageId":"20260223-pks-fsck-fix-v1-4-c29036832b6e@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im","subject":"[PATCH 4/4] pack-check: fix verification of large objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T09:50:43Z","receivedAt":"2026-02-23T09:50:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported [1] that git-fsck(1) may sometimes run into an infinite\nloop when processing packfiles. This bug was bisected to c31bad4f7d\n(packfile: track packs via the MRU list exclusively, 2025-10-30), which\nrefactored our lsit of packfiles to only be tracked via an MRU list,\nexclusively. This isn't entirely surprising: any caller that iterates\nthrough the list of packfiles and then hits `find_pack_entry()`, for\nexample because they read an object from it, may cause the MRU list to\nbe updated. And if the caller is unlucky, this may cause the mentioned\ninfinite loop.\n\nWhile this mechanism is somewhat fragile, it is still surprising that we\nencounter it when verifying the packfile. We iterate through objects in\na given pack one by one and then read them via their offset, and doing\nthis shouldn't ever end up in `find_pack_entry()`.\n\nBut there is an edge case here: when the object in question is a blob\nbigger than \"core.largeFileThreshold\", then we will be careful to not\nread it into memory. Instead, we read it via an object stream by calling\n`odb_read_object_stream()`, and that function will perform an object\nlookup via `odb_read_object_info()`. So in the case where there are at\nleast two blobs in two different packfiles, and both of these blobs\nexceed \"core.largeFileThreshold\", then we'll run into an infinite loop\nbecause we'll always update the MRU.\n\nWe could fix this by improving `repo_for_each_pack()` to not update the\nMRU, and this would address the issue. But the fun part is that using\n`odb_read_object_stream()` is the wrong thing to do in the first place:\nit may open _any_ instance of this object, so we ultimately cannot be\nsure that we even verified the object in our given packfile.\n\nFix this bug by creating the object stream for the packed object\ndirectly via `packfile_read_object_stream()`. Add a test that would have\ncaused the infinite loop.\n\n[1]: <20260222183710.2963424-1-sandals@crustytoothpaste.net>\n\nReported-by: brian m. carlson \nSigned-off-by: Patrick Steinhardt \n---\n pack-check.c | 2 +-\n t/t1450-fsck.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex 46782a29d5..6149567060 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -155,7 +155,7 @@ static int verify_packfile(struct repository *r,\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\n \t\telse if (!data &&\n-\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n+\t\t\t (packfile_read_object_stream(&stream, p, entries[i].offset) < 0 ||\n \t\t\t stream_object_signature(r, stream, &oid) < 0))\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8fb79b3e5d..ec68397ea3 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -852,6 +852,21 @@ test_expect_success 'fsck errors in packed objects' '\n \t! grep corrupt out\n '\n \n+test_expect_success 'fsck handles multiple packfiles with big blobs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tblob_one=$(test-tool genrandom one 200k | git hash-object -t blob -w --stdin) &&\n+\t\tblob_two=$(test-tool genrandom two 200k | git hash-object -t blob -w --stdin) &&\n+\t\tprintf \"%s\\n\" \"$blob_one\" | git pack-objects .git/objects/pack/pack &&\n+\t\tprintf \"%s\\n\" \"$blob_two\" | git pack-objects .git/objects/pack/pack &&\n+\t\tremove_object \"$blob_one\" &&\n+\t\tremove_object \"$blob_two\" &&\n+\t\tgit -c core.bigFileThreshold=100k fsck\n+\t)\n+'\n+\n test_expect_success 'fsck fails on corrupt packfile' '\n \thsh=$(git commit-tree -m mycommit HEAD^{tree}) &&\n \tpack=$(echo $hsh | git pack-objects .git/objects/pack/pack) &&\n\n-- \n2.53.0.414.gf7e9f6c205.dirty\n\n"},{"id":"536739","messageId":"20260223104915.GA215364@coredump.intra.peff.net","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-2-c29036832b6e@pks.im","subject":"Re: [PATCH 2/4] object-file: adapt `stream_object_signature()` to take a stream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T10:49:15Z","receivedAt":"2026-02-23T10:49:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 10:50:41AM +0100, Patrick Steinhardt wrote:\n\n> The function `stream_object_signature()` is responsible for verifying\n> whether the given object ID matches the actual hash of the object's\n> contents. In contrast to `check_object_signature()` it does so in a\n> streaming fashion so that we don't have to load the full object into\n> memory.\n\nMakes sense, and is the first step I expected to see.\n\nThe code looks OK, though a few small comments below.\n\n> -int stream_object_signature(struct repository *r, const struct object_id *oid)\n> +int stream_object_signature(struct repository *r,\n> +\t\t\t struct odb_read_stream *st,\n> +\t\t\t const struct object_id *oid)\n> {\n> \tstruct object_id real_oid;\n> -\tstruct odb_read_stream *st;\n> \tstruct git_hash_ctx c;\n> \tchar hdr[MAX_HEADER_LEN];\n> \tint hdrlen;\n> \n> -\tst = odb_read_stream_open(r->objects, oid, NULL);\n> -\tif (!st)\n> -\t\treturn -1;\n> -\n> \t/* Generate the header */\n> \thdrlen = format_object_header(hdr, sizeof(hdr), st->type, st->size);\n> \n> @@ -160,7 +157,6 @@ int stream_object_signature(struct repository *r, const struct object_id *oid)\n> \t\tgit_hash_update(&c, buf, readlen);\n> \t}\n> \tgit_hash_final_oid(&real_oid, &c);\n> -\todb_read_stream_close(st);\n> \treturn !oideq(oid, &real_oid) ? -1 : 0;\n> }\n\nThe minimal change for callers would be to give a wrapper like\nstream_object_signature_from_oid() or similar, that included the\nopen/close. But it's not too many lines, and of the two callers, one is\nthe caller we are trying to split apart anyway. So inlining these bits\nin the callers makes sense.\n\n> @@ -330,9 +331,17 @@ struct object *parse_object_with_flags(struct repository *r,\n> \n> \tif ((!obj || obj->type == OBJ_NONE || obj->type == OBJ_BLOB) &&\n> \t odb_read_object_info(r->objects, oid, NULL) == OBJ_BLOB) {\n> -\t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n> -\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> -\t\t\treturn NULL;\n> +\t\tif (!skip_hash) {\n> +\t\t\tstruct odb_read_stream *stream = odb_read_stream_open(r->objects, oid, NULL);\n> +\t\t\tif (!stream || stream_object_signature(r, stream, repl) < 0) {\n> +\t\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> +\t\t\t\tif (stream)\n> +\t\t\t\t\todb_read_stream_close(stream);\n> +\t\t\t\treturn NULL;\n> +\t\t\t}\n> +\n> +\t\t\tif (stream)\n> +\t\t\t\todb_read_stream_close(stream);\n> \t\t}\n\nThis final \"if (stream)\" is a noop; we'd have exited the function\nalready if \"!stream\".\n\nIn the earlier conditional:\n\n if (!stream || stream_object_signature(r, stream, repl) < 0)\n\nwe're combining two error checks: opening the stream and actually\nhashing it. That matches the existing code (since it all happened in a\nsingle function), but should we take this opportunity to give more\naccurate error messages? I.e., to do:\n\n if (!stream) {\n\terror(_(\"unable to open object stream for %s\"), oid_to_hex(oid));\n\treturn NULL;\n }\n if (stream_object_signature(r, stream, repl) < 0) {\n\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n\todb_read_stream_close(stream);\n\treturn NULL;\n }\n odb_read_stream_close(stream);\n\nI dunno. It should be quite uncommon to see either of these messages,\nbut that is sometimes the moment when details are most important.\n\nAlso, as an aside, I found it curious that we still need to pass the\nrepository struct to stream_object_signature(). That's because it needs\nto know the correct hash_algo. I wondered if the stream struct itself\nmight know about that, but it doesn't seem to (it doesn't know anything\nabout where it came from). So it's unavoidable that we'd need to retain\nit.\n\n> @@ -104,6 +105,7 @@ static int verify_packfile(struct repository *r,\n> \tQSORT(entries, nr_objects, compare_entries);\n> \n> \tfor (i = 0; i < nr_objects; i++) {\n> +\t\tstruct odb_read_stream *stream = NULL;\n> \t\tvoid *data;\n> \t\tstruct object_id oid;\n> \t\tenum object_type type;\n> @@ -152,7 +154,9 @@ static int verify_packfile(struct repository *r,\n> \t\t\t\t\t\t\ttype) < 0)\n> \t\t\terr = error(\"packed %s from %s is corrupt\",\n> \t\t\t\t oid_to_hex(&oid), p->pack_name);\n> -\t\telse if (!data && stream_object_signature(r, &oid) < 0)\n> +\t\telse if (!data &&\n> +\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n> +\t\t\t stream_object_signature(r, stream, &oid) < 0))\n> \t\t\terr = error(\"packed %s from %s is corrupt\",\n> \t\t\t\t oid_to_hex(&oid), p->pack_name);\n> \t\telse if (fn) {\n> @@ -163,12 +167,14 @@ static int verify_packfile(struct repository *r,\n> \t\t}\n> \t\tif (((base_count + i) & 1023) == 0)\n> \t\t\tdisplay_progress(progress, base_count + i);\n> -\t\tfree(data);\n> \n> +\t\tif (stream)\n> +\t\t\todb_read_stream_close(stream);\n> +\t\tfree(data);\n> \t}\n\nOK, and in this case the final \"if\" is important, because we just set\n\"err\" and continue through the function. This would be much simpler with\na wrapper, but of course this is the very caller we're planning to fix.\n\n-Peff\n"},{"id":"536742","messageId":"20260223110722.GB215364@coredump.intra.peff.net","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-3-c29036832b6e@pks.im","subject":"Re: [PATCH 3/4] packfile: expose function to read object stream for an offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T11:07:22Z","receivedAt":"2026-02-23T11:07:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 10:50:42AM +0100, Patrick Steinhardt wrote:\n\n> The function `packfile_store_read_object_stream()` takes as input an\n> object ID and then constructs a `struct odb_read_stream` from it. In a\n> subsequent commit we'll want to create an object stream for a given\n> combination of packfile and offset though, which is not something that\n> can currently be done.\n> \n> Extract a new function `packfile_read_object_stream()` that makes this\n> functionality available.\n\nYup, makes sense. There's one part that puzzled me at first (but I\nfigured out), and one part I'm not quite sure of.\n\n> -int packfile_store_read_object_stream(struct odb_read_stream **out,\n> -\t\t\t\t struct packfile_store *store,\n> -\t\t\t\t const struct object_id *oid)\n> +int packfile_read_object_stream(struct odb_read_stream **out,\n> +\t\t\t\tstruct packed_git *pack,\n> +\t\t\t\toff_t offset)\n> {\n> \tstruct odb_packed_read_stream *stream;\n> \tstruct pack_window *window = NULL;\n> -\tstruct object_info oi = OBJECT_INFO_INIT;\n> \tenum object_type in_pack_type;\n> \tunsigned long size;\n> \n> -\toi.sizep = &size;\n> +\tin_pack_type = unpack_object_header(pack, &window, &offset, &size);\n> +\tunuse_pack(&window);\n> \n> -\tif (packfile_store_read_object_info(store, oid, &oi, 0) ||\n> -\t oi.u.packed.type == PACKED_OBJECT_TYPE_REF_DELTA ||\n> -\t oi.u.packed.type == PACKED_OBJECT_TYPE_OFS_DELTA ||\n> -\t repo_settings_get_big_file_threshold(store->source->odb->repo) >= size)\n> +\tif (repo_settings_get_big_file_threshold(pack->repo) >= size)\n> \t\treturn -1;\n> \n> -\tin_pack_type = unpack_object_header(oi.u.packed.pack,\n> -\t\t\t\t\t &window,\n> -\t\t\t\t\t &oi.u.packed.offset,\n> -\t\t\t\t\t &size);\n> -\tunuse_pack(&window);\n\nBefore we were checking big_file_threshold up front, and now we must\ncall unpack_object_header() first. But that's because we got the size\nfor \"free\" as part of the object_info call that found our pack entry.\n\nNow our caller is responsible for finding the entry. Our wrapper _could_\ncontinue to provide us with the size, but I don't think there is any\nefficiency to be gained. Once we have the pack/offset pair, both code\npaths will call unpack_object_header() to find it cheaply. And the new\ncode is even a little more efficient.\n\nBut what about the checks for deltas? We've dropped them completely. I\nthink that's OK, though, because later we have:\n\n> \tswitch (in_pack_type) {\n> \tdefault:\n> \t\treturn -1; /* we do not do deltas for now */\n\nSo they were somewhat redundant in the first place, and just avoided\ncalling unpack_object_header() for cases where we knew we could not use\nthe result (which again, was already filled by packed_object_info() in\nthe same way).\n\nGood.\n\n> +int packfile_store_read_object_stream(struct odb_read_stream **out,\n> +\t\t\t\t struct packfile_store *store,\n> +\t\t\t\t const struct object_id *oid)\n> +{\n> +\tstruct pack_entry e;\n> +\n> +\tif (!find_pack_entry(store, oid, &e))\n> +\t\treturn -1;\n> +\n> +\treturn packfile_read_object_stream(out, e.p, e.offset);\n> +}\n\nOK. The original read via packfile_store_read_object_info(), which does\na bit more work. It called packed_object_info() and if necessary would\ntrigger mark_bad_packed_object(). But now that we are leaving it to\npackfile_read_object_stream() to look at the header, we don't need to\nload any object info, and we have no error code to check.\n\nIt does make me wonder, though, if we are missing out on marking bad\nobjects here. The idea is that we'd usually do something like:\n\n 1. some code wants to access $OID\n\n 2. we find $OID in pack $P\n\n 3. that turns out to be broken for some reason, so we mark it as bad\n\n 4. we try again, skipping $P and finding it in some other pack\n\nBut now I wonder if code that tries to stream will skip step 3, and then\nin step 4 we'll find the same broken $P over and over.\n\nBut I suspect if that is possible, it was already true. We were only\nasking for the type and size, so any content-level corruption wouldn't\nbe caught here and we'd have the same issue. I think the right thing is\nprobably for the streaming code to know about the pack/oid pair it's\ntrying to read, and to mark it as bad if it hits an error.\n\nSo your patch here might be making the problem a tiny bit worse, but not\nin a material way. I think we can ignore it for now.\n\n-Peff\n"},{"id":"536743","messageId":"20260223111120.GC215364@coredump.intra.peff.net","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-4-c29036832b6e@pks.im","subject":"Re: [PATCH 4/4] pack-check: fix verification of large objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T11:11:20Z","receivedAt":"2026-02-23T11:11:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 10:50:43AM +0100, Patrick Steinhardt wrote:\n\n> diff --git a/pack-check.c b/pack-check.c\n> index 46782a29d5..6149567060 100644\n> --- a/pack-check.c\n> +++ b/pack-check.c\n> @@ -155,7 +155,7 @@ static int verify_packfile(struct repository *r,\n> \t\t\terr = error(\"packed %s from %s is corrupt\",\n> \t\t\t\t oid_to_hex(&oid), p->pack_name);\n> \t\telse if (!data &&\n> -\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n> +\t\t\t (packfile_read_object_stream(&stream, p, entries[i].offset) < 0 ||\n\nAnd now this change is delightfully simple.\n\n> +test_expect_success 'fsck handles multiple packfiles with big blobs' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tblob_one=$(test-tool genrandom one 200k | git hash-object -t blob -w --stdin) &&\n> +\t\tblob_two=$(test-tool genrandom two 200k | git hash-object -t blob -w --stdin) &&\n> +\t\tprintf \"%s\\n\" \"$blob_one\" | git pack-objects .git/objects/pack/pack &&\n> +\t\tprintf \"%s\\n\" \"$blob_two\" | git pack-objects .git/objects/pack/pack &&\n> +\t\tremove_object \"$blob_one\" &&\n> +\t\tremove_object \"$blob_two\" &&\n> +\t\tgit -c core.bigFileThreshold=100k fsck\n> +\t)\n> +'\n\nI like seeing this much-more-specific test case. It does sort of become\na noop if we fix the iteration problem, though.\n\nA more concrete test would probably be something like:\n\n 1. Two packs, $X and $Y, both contain the same object.\n\n 2. The object is corrupt in $X but not in $Y.\n\n 3. Running fsck detects that one copy is corrupt but the other is\n not.\n\nRight now it may or may not fail depending on the ordering of the packs\nin the MRU list (which we might be able to tweak via mtimes). But\nhopefully in the \"after\" state it should deterministically complain\nabout $X.\n\n-Peff\n"},{"id":"536744","messageId":"20260223111346.GD215364@coredump.intra.peff.net","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-1-c29036832b6e@pks.im","subject":"Re: [PATCH 1/4] t/helper: improve \"genrandom\" test helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T11:13:46Z","receivedAt":"2026-02-23T11:13:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 10:50:40AM +0100, Patrick Steinhardt wrote:\n\n> Fix both of these issues by using `git_parse_ulong()` to parse the\n> argumemnt. This function has better error handling, and it knows to\n> handle unit suffixes.\n\nMakes sense, but...\n\n> @@ -22,7 +23,9 @@ int cmd__genrandom(int argc, const char **argv)\n> \t\tnext = next * 11 + *c;\n> \t} while (*c++);\n> \n> -\tcount = (argc == 3) ? strtoul(argv[2], NULL, 0) : ULONG_MAX;\n> +\tcount = ULONG_MAX;\n> +\tif (argc == 3 && git_parse_ulong(argv[2], &count) < 0)\n> +\t\treturn error_errno(\"cannot parse argument '%s'\", argv[2]);\n\n...I think the return value of git_parse_ulong() is boolean 0/1, not\n0/negative.\n\n-Peff\n"},{"id":"536748","messageId":"aZw6W_BHoYiC9RYl@pks.im","threadId":"65051","inReplyTo":"20260223111120.GC215364@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] pack-check: fix verification of large objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T11:30:35Z","receivedAt":"2026-02-23T11:30:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 06:11:20AM -0500, Jeff King wrote:\n> On Mon, Feb 23, 2026 at 10:50:43AM +0100, Patrick Steinhardt wrote:\n> \n> > diff --git a/pack-check.c b/pack-check.c\n> > index 46782a29d5..6149567060 100644\n> > --- a/pack-check.c\n> > +++ b/pack-check.c\n> > @@ -155,7 +155,7 @@ static int verify_packfile(struct repository *r,\n> > \t\t\terr = error(\"packed %s from %s is corrupt\",\n> > \t\t\t\t oid_to_hex(&oid), p->pack_name);\n> > \t\telse if (!data &&\n> > -\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n> > +\t\t\t (packfile_read_object_stream(&stream, p, entries[i].offset) < 0 ||\n> \n> And now this change is delightfully simple.\n> \n> > +test_expect_success 'fsck handles multiple packfiles with big blobs' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\tblob_one=$(test-tool genrandom one 200k | git hash-object -t blob -w --stdin) &&\n> > +\t\tblob_two=$(test-tool genrandom two 200k | git hash-object -t blob -w --stdin) &&\n> > +\t\tprintf \"%s\\n\" \"$blob_one\" | git pack-objects .git/objects/pack/pack &&\n> > +\t\tprintf \"%s\\n\" \"$blob_two\" | git pack-objects .git/objects/pack/pack &&\n> > +\t\tremove_object \"$blob_one\" &&\n> > +\t\tremove_object \"$blob_two\" &&\n> > +\t\tgit -c core.bigFileThreshold=100k fsck\n> > +\t)\n> > +'\n> \n> I like seeing this much-more-specific test case. It does sort of become\n> a noop if we fix the iteration problem, though.\n> \n> A more concrete test would probably be something like:\n> \n> 1. Two packs, $X and $Y, both contain the same object.\n> \n> 2. The object is corrupt in $X but not in $Y.\n> \n> 3. Running fsck detects that one copy is corrupt but the other is\n> not.\n> \n> Right now it may or may not fail depending on the ordering of the packs\n> in the MRU list (which we might be able to tweak via mtimes). But\n> hopefully in the \"after\" state it should deterministically complain\n> about $X.\n\nYeah. The problem I had here is that I'm not sure whether we have any\ntools to reliably create a corrupted object, e.g. with a hash mismatch.\nI'll have a look for v2.\n\nThanks!\n\nPatrick\n"},{"id":"536767","messageId":"aZxGJY7JH4Xwytwb@pks.im","threadId":"65051","inReplyTo":"20260223111346.GD215364@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] t/helper: improve \"genrandom\" test helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T12:20:53Z","receivedAt":"2026-02-23T12:20:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 06:13:46AM -0500, Jeff King wrote:\n> On Mon, Feb 23, 2026 at 10:50:40AM +0100, Patrick Steinhardt wrote:\n> > @@ -22,7 +23,9 @@ int cmd__genrandom(int argc, const char **argv)\n> > \t\tnext = next * 11 + *c;\n> > \t} while (*c++);\n> > \n> > -\tcount = (argc == 3) ? strtoul(argv[2], NULL, 0) : ULONG_MAX;\n> > +\tcount = ULONG_MAX;\n> > +\tif (argc == 3 && git_parse_ulong(argv[2], &count) < 0)\n> > +\t\treturn error_errno(\"cannot parse argument '%s'\", argv[2]);\n> \n> ...I think the return value of git_parse_ulong() is boolean 0/1, not\n> 0/negative.\n\nUgh, right. Will fix.\n\nPatrick\n"},{"id":"536768","messageId":"aZxGLKycnZcVoXPt@pks.im","threadId":"65051","inReplyTo":"20260223104915.GA215364@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] object-file: adapt `stream_object_signature()` to take a stream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T12:21:00Z","receivedAt":"2026-02-23T12:21:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 05:49:15AM -0500, Jeff King wrote:\n> On Mon, Feb 23, 2026 at 10:50:41AM +0100, Patrick Steinhardt wrote:\n> \n> > The function `stream_object_signature()` is responsible for verifying\n> > whether the given object ID matches the actual hash of the object's\n> > contents. In contrast to `check_object_signature()` it does so in a\n> > streaming fashion so that we don't have to load the full object into\n> > memory.\n> \n> Makes sense, and is the first step I expected to see.\n> \n> The code looks OK, though a few small comments below.\n> \n> > -int stream_object_signature(struct repository *r, const struct object_id *oid)\n> > +int stream_object_signature(struct repository *r,\n> > +\t\t\t struct odb_read_stream *st,\n> > +\t\t\t const struct object_id *oid)\n> > {\n> > \tstruct object_id real_oid;\n> > -\tstruct odb_read_stream *st;\n> > \tstruct git_hash_ctx c;\n> > \tchar hdr[MAX_HEADER_LEN];\n> > \tint hdrlen;\n> > \n> > -\tst = odb_read_stream_open(r->objects, oid, NULL);\n> > -\tif (!st)\n> > -\t\treturn -1;\n> > -\n> > \t/* Generate the header */\n> > \thdrlen = format_object_header(hdr, sizeof(hdr), st->type, st->size);\n> > \n> > @@ -160,7 +157,6 @@ int stream_object_signature(struct repository *r, const struct object_id *oid)\n> > \t\tgit_hash_update(&c, buf, readlen);\n> > \t}\n> > \tgit_hash_final_oid(&real_oid, &c);\n> > -\todb_read_stream_close(st);\n> > \treturn !oideq(oid, &real_oid) ? -1 : 0;\n> > }\n> \n> The minimal change for callers would be to give a wrapper like\n> stream_object_signature_from_oid() or similar, that included the\n> open/close. But it's not too many lines, and of the two callers, one is\n> the caller we are trying to split apart anyway. So inlining these bits\n> in the callers makes sense.\n\nYeah. Had there been more callers then I'd have done that, but as you\nnoted there's only two, and one of them will need to be open coded\nanyway.\n\n> > @@ -330,9 +331,17 @@ struct object *parse_object_with_flags(struct repository *r,\n> > \n> > \tif ((!obj || obj->type == OBJ_NONE || obj->type == OBJ_BLOB) &&\n> > \t odb_read_object_info(r->objects, oid, NULL) == OBJ_BLOB) {\n> > -\t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n> > -\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> > -\t\t\treturn NULL;\n> > +\t\tif (!skip_hash) {\n> > +\t\t\tstruct odb_read_stream *stream = odb_read_stream_open(r->objects, oid, NULL);\n> > +\t\t\tif (!stream || stream_object_signature(r, stream, repl) < 0) {\n> > +\t\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> > +\t\t\t\tif (stream)\n> > +\t\t\t\t\todb_read_stream_close(stream);\n> > +\t\t\t\treturn NULL;\n> > +\t\t\t}\n> > +\n> > +\t\t\tif (stream)\n> > +\t\t\t\todb_read_stream_close(stream);\n> > \t\t}\n> \n> This final \"if (stream)\" is a noop; we'd have exited the function\n> already if \"!stream\".\n> \n> In the earlier conditional:\n> \n> if (!stream || stream_object_signature(r, stream, repl) < 0)\n> \n> we're combining two error checks: opening the stream and actually\n> hashing it.\n\nAh, right.\n\n> That matches the existing code (since it all happened in a\n> single function), but should we take this opportunity to give more\n> accurate error messages? I.e., to do:\n> \n> if (!stream) {\n> \terror(_(\"unable to open object stream for %s\"), oid_to_hex(oid));\n> \treturn NULL;\n> }\n> if (stream_object_signature(r, stream, repl) < 0) {\n> \terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> \todb_read_stream_close(stream);\n> \treturn NULL;\n> }\n> odb_read_stream_close(stream);\n> \n> I dunno. It should be quite uncommon to see either of these messages,\n> but that is sometimes the moment when details are most important.\n\nAgreed, that feels like a sensible change indeed. Also makes the code\nflow easier to follow in my opinion.\n\n> Also, as an aside, I found it curious that we still need to pass the\n> repository struct to stream_object_signature(). That's because it needs\n> to know the correct hash_algo. I wondered if the stream struct itself\n> might know about that, but it doesn't seem to (it doesn't know anything\n> about where it came from). So it's unavoidable that we'd need to retain\n> it.\n\nYeah, agreed. I wondered whether we should eventually extend `struct\nodb_read_stream` to have a pointer to the owning object source, and in\nthat case we could've avoided the extra repository parameter. But I\ndecided it was out of scope for this patch series, also because I don't\nwant to cause conflicts with other stuff I'm working on in this vicinity\n:)\n\nPatrick\n"},{"id":"536769","messageId":"aZxGMrGkVNeAdC1N@pks.im","threadId":"65051","inReplyTo":"20260223110722.GB215364@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] packfile: expose function to read object stream for an offset","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T12:21:06Z","receivedAt":"2026-02-23T12:21:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 06:07:22AM -0500, Jeff King wrote:\n> On Mon, Feb 23, 2026 at 10:50:42AM +0100, Patrick Steinhardt wrote:\n> > +int packfile_store_read_object_stream(struct odb_read_stream **out,\n> > +\t\t\t\t struct packfile_store *store,\n> > +\t\t\t\t const struct object_id *oid)\n> > +{\n> > +\tstruct pack_entry e;\n> > +\n> > +\tif (!find_pack_entry(store, oid, &e))\n> > +\t\treturn -1;\n> > +\n> > +\treturn packfile_read_object_stream(out, e.p, e.offset);\n> > +}\n> \n> OK. The original read via packfile_store_read_object_info(), which does\n> a bit more work. It called packed_object_info() and if necessary would\n> trigger mark_bad_packed_object(). But now that we are leaving it to\n> packfile_read_object_stream() to look at the header, we don't need to\n> load any object info, and we have no error code to check.\n> \n> It does make me wonder, though, if we are missing out on marking bad\n> objects here. The idea is that we'd usually do something like:\n> \n> 1. some code wants to access $OID\n> \n> 2. we find $OID in pack $P\n> \n> 3. that turns out to be broken for some reason, so we mark it as bad\n> \n> 4. we try again, skipping $P and finding it in some other pack\n> \n> But now I wonder if code that tries to stream will skip step 3, and then\n> in step 4 we'll find the same broken $P over and over.\n> \n> But I suspect if that is possible, it was already true. We were only\n> asking for the type and size, so any content-level corruption wouldn't\n> be caught here and we'd have the same issue. I think the right thing is\n> probably for the streaming code to know about the pack/oid pair it's\n> trying to read, and to mark it as bad if it hits an error.\n> \n> So your patch here might be making the problem a tiny bit worse, but not\n> in a material way. I think we can ignore it for now.\n\nI guess the \"tiny bit worse\" part is that we don't handle the case\nanymore where `unpack_object_header()` returns `OBJ_BAD`. As you say, we\npreviously didn't fully parse the object anyway, so we couldn't have\ndetected all kinds of corruptions. But we definitely handled the case\nwhere `unpack_object_header()` failed.\n\nSo maybe we should do something like the below patch?\n\nPatrick\n\ndiff --git a/packfile.c b/packfile.c\nindex 9d795a671f..3e61176128 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2554,6 +2554,7 @@ static int close_istream_pack_non_delta(struct odb_read_stream *_st)\n }\n \n int packfile_read_object_stream(struct odb_read_stream **out,\n+\t\t\t\tconst struct object_id *oid,\n \t\t\t\tstruct packed_git *pack,\n \t\t\t\toff_t offset)\n {\n@@ -2571,6 +2572,9 @@ int packfile_read_object_stream(struct odb_read_stream **out,\n \tswitch (in_pack_type) {\n \tdefault:\n \t\treturn -1; /* we do not do deltas for now */\n+\tcase OBJ_BAD:\n+\t\tmark_bad_packed_object(pack, oid);\n+\t\treturn -1;\n \tcase OBJ_COMMIT:\n \tcase OBJ_TREE:\n \tcase OBJ_BLOB:\n@@ -2601,5 +2605,5 @@ int packfile_store_read_object_stream(struct odb_read_stream **out,\n \tif (!find_pack_entry(store, oid, &e))\n \t\treturn -1;\n \n-\treturn packfile_read_object_stream(out, e.p, e.offset);\n+\treturn packfile_read_object_stream(out, oid, e.p, e.offset);\n }\ndiff --git a/packfile.h b/packfile.h\nindex 67d5750140..b9f5f1c18c 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -437,6 +437,7 @@ off_t get_delta_base(struct packed_git *p, struct pack_window **w_curs,\n \t\t off_t delta_obj_offset);\n \n int packfile_read_object_stream(struct odb_read_stream **out,\n+\t\t\t\tconst struct object_id *oid,\n \t\t\t\tstruct packed_git *pack,\n \t\t\t\toff_t offset);\n \n"},{"id":"536785","messageId":"20260223125843.GA215671@coredump.intra.peff.net","threadId":"65051","inReplyTo":"aZw6W_BHoYiC9RYl@pks.im","subject":"Re: [PATCH 4/4] pack-check: fix verification of large objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T12:58:43Z","receivedAt":"2026-02-23T12:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 12:30:35PM +0100, Patrick Steinhardt wrote:\n\n> > A more concrete test would probably be something like:\n> > \n> > 1. Two packs, $X and $Y, both contain the same object.\n> > \n> > 2. The object is corrupt in $X but not in $Y.\n> > \n> > 3. Running fsck detects that one copy is corrupt but the other is\n> > not.\n> > \n> > Right now it may or may not fail depending on the ordering of the packs\n> > in the MRU list (which we might be able to tweak via mtimes). But\n> > hopefully in the \"after\" state it should deterministically complain\n> > about $X.\n> \n> Yeah. The problem I had here is that I'm not sure whether we have any\n> tools to reliably create a corrupted object, e.g. with a hash mismatch.\n> I'll have a look for v2.\n\nYou can see how do_corrupt_object() in t5303 does it. It's basically\nfinding an offset via show-index and then writing a zero over it with\ndd.\n\n-Peff\n"},{"id":"536786","messageId":"20260223125955.GB215671@coredump.intra.peff.net","threadId":"65051","inReplyTo":"aZxGLKycnZcVoXPt@pks.im","subject":"Re: [PATCH 2/4] object-file: adapt `stream_object_signature()` to take a stream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T12:59:55Z","receivedAt":"2026-02-23T12:59:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 01:21:00PM +0100, Patrick Steinhardt wrote:\n\n> > That matches the existing code (since it all happened in a\n> > single function), but should we take this opportunity to give more\n> > accurate error messages? I.e., to do:\n> > \n> > if (!stream) {\n> > \terror(_(\"unable to open object stream for %s\"), oid_to_hex(oid));\n> > \treturn NULL;\n> > }\n> > if (stream_object_signature(r, stream, repl) < 0) {\n> > \terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> > \todb_read_stream_close(stream);\n> > \treturn NULL;\n> > }\n> > odb_read_stream_close(stream);\n> > \n> > I dunno. It should be quite uncommon to see either of these messages,\n> > but that is sometimes the moment when details are most important.\n> \n> Agreed, that feels like a sensible change indeed. Also makes the code\n> flow easier to follow in my opinion.\n\nYeah, the readability was actually what got me thinking on it in the\nfirst place.\n\n> > Also, as an aside, I found it curious that we still need to pass the\n> > repository struct to stream_object_signature(). That's because it needs\n> > to know the correct hash_algo. I wondered if the stream struct itself\n> > might know about that, but it doesn't seem to (it doesn't know anything\n> > about where it came from). So it's unavoidable that we'd need to retain\n> > it.\n> \n> Yeah, agreed. I wondered whether we should eventually extend `struct\n> odb_read_stream` to have a pointer to the owning object source, and in\n> that case we could've avoided the extra repository parameter. But I\n> decided it was out of scope for this patch series, also because I don't\n> want to cause conflicts with other stuff I'm working on in this vicinity\n> :)\n\nYes, definitely out of scope for this series.\n\n-Peff\n"},{"id":"536787","messageId":"20260223131201.GC215671@coredump.intra.peff.net","threadId":"65051","inReplyTo":"aZxGMrGkVNeAdC1N@pks.im","subject":"Re: [PATCH 3/4] packfile: expose function to read object stream for an offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T13:12:01Z","receivedAt":"2026-02-23T13:12:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 01:21:06PM +0100, Patrick Steinhardt wrote:\n\n> > So your patch here might be making the problem a tiny bit worse, but not\n> > in a material way. I think we can ignore it for now.\n> \n> I guess the \"tiny bit worse\" part is that we don't handle the case\n> anymore where `unpack_object_header()` returns `OBJ_BAD`. As you say, we\n> previously didn't fully parse the object anyway, so we couldn't have\n> detected all kinds of corruptions. But we definitely handled the case\n> where `unpack_object_header()` failed.\n\nYeah, I think that would cover it. Technically packed_object_info()\ncould error on more cases (e.g., errors chasing delta bases for\ntype/size info). But we would bail on trying to stream those anyway, so\npresumably any errors would be found via the non-streaming code paths in\nthose cases.\n\n> So maybe we should do something like the below patch?\n> [...]\n> @@ -2571,6 +2572,9 @@ int packfile_read_object_stream(struct odb_read_stream **out,\n> \tswitch (in_pack_type) {\n> \tdefault:\n> \t\treturn -1; /* we do not do deltas for now */\n> +\tcase OBJ_BAD:\n> +\t\tmark_bad_packed_object(pack, oid);\n> +\t\treturn -1;\n> \tcase OBJ_COMMIT:\n> \tcase OBJ_TREE:\n> \tcase OBJ_BLOB:\n\nI think that restores the original behavior. But I'm not sure it's even\nworth it. We are still missing the much more likely case of a bit error\nin the actual zlib stream, which would not be caught until much later.\n\nSo yeah, if you want to feel better about making sure your patch keeps\nthe behavior as identical as possible, I don't mind adding this. But it\nfeels like the tip of the iceberg, and I'd be OK leaving it for later\n(or never).\n\nMy biggest objection is not the two lines above (which I actually think\nclarify what is going on) but rather this interface change:\n\n> int packfile_read_object_stream(struct odb_read_stream **out,\n> +\t\t\t\tconst struct object_id *oid,\n> \t\t\t\tstruct packed_git *pack,\n> \t\t\t\toff_t offset);\n\nNow we are back to taking an oid, except we don't ever use it to look up\nthe object! So it's a little misleading that it's there at all. It may\nbe the best we can do, though.\n\nThe only other way I could think of is for packfile_read_object_stream()\nto return a more detailed error: one of \"success\", \"chose not to\nstream\", or \"broken object\". And then the caller can call\nmark_bad_packed_object() as appropriate. In this case, I think\npackfile_store_read_object_stream() would do so, but verify_pack()\nprobably would not choose to (it is not interested in fallbacks at all\nbut is going through an individual pack).\n\n-Peff\n"},{"id":"536793","messageId":"CAPig+cSSLd0MqEsvaeQFPN2-usZHvSCS=1Nor_w2xbOR+W9eWA@mail.gmail.com","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-1-c29036832b6e@pks.im","subject":"Re: [PATCH 1/4] t/helper: improve \"genrandom\" test helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-02-23T14:01:40Z","receivedAt":"2026-02-23T14:01:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 23, 2026 at 4:51 AM Patrick Steinhardt wrote:\n> The `test-tool genrandom` test helper can be used to generate random\n> data, either as an infinite stream or with a specified number of bytes.\n> The way we handle parsing the number of bytes is lacking though:\n>\n> - We don't have good error handling, so if the caller for example uses\n> `test-tool genrandom 200xyz` then we'll end up generating 200 bytes\n> of random data successfully.\n>\n> - Many callers want to generate e.g. 1 kilobyte or megabyte of data,\n> but they have to either use unwieldy numbers like 1048576, or they\n> have to precompute them.\n>\n> Fix both of these issues by using `git_parse_ulong()` to parse the\n> argumemnt. This function has better error handling, and it knows to\n> handle unit suffixes.\n\ns/argumemnt/argument/\n\n> Adapt a couple of our tests to use suffixes instead of manual\n> computations.\n>\n> Signed-off-by: Patrick Steinhardt \n"},{"id":"536816","messageId":"aZx2uRiLIaa21L-x@pks.im","threadId":"65051","inReplyTo":"20260223125843.GA215671@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] pack-check: fix verification of large objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T15:48:09Z","receivedAt":"2026-02-23T15:48:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 07:58:43AM -0500, Jeff King wrote:\n> On Mon, Feb 23, 2026 at 12:30:35PM +0100, Patrick Steinhardt wrote:\n> \n> > > A more concrete test would probably be something like:\n> > > \n> > > 1. Two packs, $X and $Y, both contain the same object.\n> > > \n> > > 2. The object is corrupt in $X but not in $Y.\n> > > \n> > > 3. Running fsck detects that one copy is corrupt but the other is\n> > > not.\n> > > \n> > > Right now it may or may not fail depending on the ordering of the packs\n> > > in the MRU list (which we might be able to tweak via mtimes). But\n> > > hopefully in the \"after\" state it should deterministically complain\n> > > about $X.\n> > \n> > Yeah. The problem I had here is that I'm not sure whether we have any\n> > tools to reliably create a corrupted object, e.g. with a hash mismatch.\n> > I'll have a look for v2.\n> \n> You can see how do_corrupt_object() in t5303 does it. It's basically\n> finding an offset via show-index and then writing a zero over it with\n> dd.\n\nYeah, that's what I ended up doing indeed. I spotted such a test in t1450.\n\nPatrick\n"},{"id":"536819","messageId":"aZx5Tc-rgih0S4gS@pks.im","threadId":"65051","inReplyTo":"20260223131201.GC215671@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] packfile: expose function to read object stream for an offset","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T15:59:09Z","receivedAt":"2026-02-23T15:59:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 08:12:01AM -0500, Jeff King wrote:\n> On Mon, Feb 23, 2026 at 01:21:06PM +0100, Patrick Steinhardt wrote:\n> \n> > > So your patch here might be making the problem a tiny bit worse, but not\n> > > in a material way. I think we can ignore it for now.\n> > \n> > I guess the \"tiny bit worse\" part is that we don't handle the case\n> > anymore where `unpack_object_header()` returns `OBJ_BAD`. As you say, we\n> > previously didn't fully parse the object anyway, so we couldn't have\n> > detected all kinds of corruptions. But we definitely handled the case\n> > where `unpack_object_header()` failed.\n> \n> Yeah, I think that would cover it. Technically packed_object_info()\n> could error on more cases (e.g., errors chasing delta bases for\n> type/size info). But we would bail on trying to stream those anyway, so\n> presumably any errors would be found via the non-streaming code paths in\n> those cases.\n> \n> > So maybe we should do something like the below patch?\n> > [...]\n> > @@ -2571,6 +2572,9 @@ int packfile_read_object_stream(struct odb_read_stream **out,\n> > \tswitch (in_pack_type) {\n> > \tdefault:\n> > \t\treturn -1; /* we do not do deltas for now */\n> > +\tcase OBJ_BAD:\n> > +\t\tmark_bad_packed_object(pack, oid);\n> > +\t\treturn -1;\n> > \tcase OBJ_COMMIT:\n> > \tcase OBJ_TREE:\n> > \tcase OBJ_BLOB:\n> \n> I think that restores the original behavior. But I'm not sure it's even\n> worth it. We are still missing the much more likely case of a bit error\n> in the actual zlib stream, which would not be caught until much later.\n> \n> So yeah, if you want to feel better about making sure your patch keeps\n> the behavior as identical as possible, I don't mind adding this. But it\n> feels like the tip of the iceberg, and I'd be OK leaving it for later\n> (or never).\n> \n> My biggest objection is not the two lines above (which I actually think\n> clarify what is going on) but rather this interface change:\n> \n> > int packfile_read_object_stream(struct odb_read_stream **out,\n> > +\t\t\t\tconst struct object_id *oid,\n> > \t\t\t\tstruct packed_git *pack,\n> > \t\t\t\toff_t offset);\n> \n> Now we are back to taking an oid, except we don't ever use it to look up\n> the object! So it's a little misleading that it's there at all. It may\n> be the best we can do, though.\n\nYeah, I agree it's a bit ugly. But the function is not likely to gain a\nlot of additional callers anyway, as it is an implementation detail of\nthe packfile store. So overall I think it's okayish.\n\n> The only other way I could think of is for packfile_read_object_stream()\n> to return a more detailed error: one of \"success\", \"chose not to\n> stream\", or \"broken object\". And then the caller can call\n> mark_bad_packed_object() as appropriate. In this case, I think\n> packfile_store_read_object_stream() would do so, but verify_pack()\n> probably would not choose to (it is not interested in fallbacks at all\n> but is going through an individual pack).\n\nWe could of course have it return the `enum object_type` directly, which\ngives us enough context to do this. But on the other hand it'd mean that\nwe might now miss adding calls to `mark_bad_packed_object()` in the\nfuture, and it causes a bit of repetition across callsites.\n\nSo I think I lean towards my proposed patch.\n\nThanks!\n\nPatrick\n"},{"id":"536820","messageId":"20260223-pks-fsck-fix-v2-0-99a0714ea3bd@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im","subject":"[PATCH v2 0/4] pack-check: fix verification of large objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T16:00:05Z","receivedAt":"2026-02-23T16:00:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series addresses the bug reported by brian in [1].\nThanks!\n\nChanges in v2:\n - Extend the test to verify that we actually find corrupted objects in\n both packs, even in the case where a non-corrupt version exists in\n another pack.\n - Reinstate `mark_packed_object_bad()`.\n - Fix error checking for `git_parse_ulong()`.\n - Disambiguate error conditions in `parse_object_with_flags()`.\n - Link to v1: https://lore.kernel.org/r/20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im\n\nPatrick\n\n[1]: <20260222183710.2963424-1-sandals@crustytoothpaste.net>\n\n---\nPatrick Steinhardt (4):\n t/helper: improve \"genrandom\" test helper\n object-file: adapt `stream_object_signature()` to take a stream\n packfile: expose function to read object stream for an offset\n pack-check: fix verification of large objects\n\n object-file.c | 10 +++------\n object-file.h | 4 +++-\n object.c | 19 ++++++++++++++---\n pack-check.c | 12 ++++++++---\n packfile.c | 40 +++++++++++++++++++++--------------\n packfile.h | 5 +++++\n t/helper/test-genrandom.c | 5 ++++-\n t/t1006-cat-file.sh | 2 +-\n t/t1050-large.sh | 6 +++---\n t/t1450-fsck.sh | 40 ++++++++++++++++++++++++++++++++++-\n t/t5301-sliding-window.sh | 2 +-\n t/t5310-pack-bitmaps.sh | 2 +-\n t/t5710-promisor-remote-capability.sh | 4 ++--\n t/t7700-repack.sh | 6 +++---\n 14 files changed, 114 insertions(+), 43 deletions(-)\n\nRange-diff versus v1:\n\n1: 1b1283e837 ! 1: daf895aef6 t/helper: improve \"genrandom\" test helper\n @@ Commit message\n have to precompute them.\n \n Fix both of these issues by using `git_parse_ulong()` to parse the\n - argumemnt. This function has better error handling, and it knows to\n + argument. This function has better error handling, and it knows to\n handle unit suffixes.\n \n Adapt a couple of our tests to use suffixes instead of manual\n @@ t/helper/test-genrandom.c: int cmd__genrandom(int argc, const char **argv)\n \n -\tcount = (argc == 3) ? strtoul(argv[2], NULL, 0) : ULONG_MAX;\n +\tcount = ULONG_MAX;\n -+\tif (argc == 3 && git_parse_ulong(argv[2], &count) < 0)\n ++\tif (argc == 3 && !git_parse_ulong(argv[2], &count))\n +\t\treturn error_errno(\"cannot parse argument '%s'\", argv[2]);\n \n \twhile (count--) {\n2: 9f25ed1a4b ! 2: ebca9efaec object-file: adapt `stream_object_signature()` to take a stream\n @@ Commit message\n a preconstructed stream. Prepare for this by accepting a stream as input\n that the caller needs to assemble.\n \n + While at it, improve the error reporting in `parse_object_with_flags()`\n + to tell apart the two failure modes.\n +\n + Helped-by: Jeff King \n Signed-off-by: Patrick Steinhardt \n \n ## object-file.c ##\n @@ object.c: struct object *parse_object_with_flags(struct repository *r,\n -\t\t\treturn NULL;\n +\t\tif (!skip_hash) {\n +\t\t\tstruct odb_read_stream *stream = odb_read_stream_open(r->objects, oid, NULL);\n -+\t\t\tif (!stream || stream_object_signature(r, stream, repl) < 0) {\n -+\t\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n -+\t\t\t\tif (stream)\n -+\t\t\t\t\todb_read_stream_close(stream);\n ++\n ++\t\t\tif (!stream) {\n ++\t\t\t\terror(_(\"unable to open object stream for %s\"), oid_to_hex(oid));\n +\t\t\t\treturn NULL;\n +\t\t\t}\n +\n -+\t\t\tif (stream)\n ++\t\t\tif (stream_object_signature(r, stream, repl) < 0) {\n ++\t\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n +\t\t\t\todb_read_stream_close(stream);\n ++\t\t\t\treturn NULL;\n ++\t\t\t}\n ++\n ++\t\t\todb_read_stream_close(stream);\n \t\t}\n \t\tparse_blob_buffer(lookup_blob(r, oid));\n \t\treturn lookup_object(r, oid);\n3: 9a28867564 ! 3: bead96797e packfile: expose function to read object stream for an offset\n @@ packfile.c: static int close_istream_pack_non_delta(struct odb_read_stream *_st)\n -\t\t\t\t struct packfile_store *store,\n -\t\t\t\t const struct object_id *oid)\n +int packfile_read_object_stream(struct odb_read_stream **out,\n ++\t\t\t\tconst struct object_id *oid,\n +\t\t\t\tstruct packed_git *pack,\n +\t\t\t\toff_t offset)\n {\n @@ packfile.c: static int close_istream_pack_non_delta(struct odb_read_stream *_st)\n \tswitch (in_pack_type) {\n \tdefault:\n \t\treturn -1; /* we do not do deltas for now */\n ++\tcase OBJ_BAD:\n ++\t\tmark_bad_packed_object(pack, oid);\n ++\t\treturn -1;\n + \tcase OBJ_COMMIT:\n + \tcase OBJ_TREE:\n + \tcase OBJ_BLOB:\n @@ packfile.c: int packfile_store_read_object_stream(struct odb_read_stream **out,\n \tstream->base.type = in_pack_type;\n \tstream->base.size = size;\n @@ packfile.c: int packfile_store_read_object_stream(struct odb_read_stream **out,\n +\tif (!find_pack_entry(store, oid, &e))\n +\t\treturn -1;\n +\n -+\treturn packfile_read_object_stream(out, e.p, e.offset);\n ++\treturn packfile_read_object_stream(out, oid, e.p, e.offset);\n +}\n \n ## packfile.h ##\n @@ packfile.h: off_t get_delta_base(struct packed_git *p, struct pack_window **w_cu\n \t\t off_t delta_obj_offset);\n \n +int packfile_read_object_stream(struct odb_read_stream **out,\n ++\t\t\t\tconst struct object_id *oid,\n +\t\t\t\tstruct packed_git *pack,\n +\t\t\t\toff_t offset);\n +\n4: 4eaf958e57 ! 4: 6b69624d81 pack-check: fix verification of large objects\n @@ pack-check.c: static int verify_packfile(struct repository *r,\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\n \t\telse if (!data &&\n -\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n -+\t\t\t (packfile_read_object_stream(&stream, p, entries[i].offset) < 0 ||\n ++\t\t\t (packfile_read_object_stream(&stream, &oid, p, entries[i].offset) < 0 ||\n \t\t\t stream_object_signature(r, stream, &oid) < 0))\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\n @@ t/t1450-fsck.sh: test_expect_success 'fsck errors in packed objects' '\n +\tgit init repo &&\n +\t(\n +\t\tcd repo &&\n ++\n ++\t\t# We construct two packfiles with two objects in common and one\n ++\t\t# object not in common. The objects in common can then be\n ++\t\t# corrupted in one of the packfiles, respectively. The other\n ++\t\t# objects that are unique to the packs are merely used to not\n ++\t\t# have both packs contain the same data.\n +\t\tblob_one=$(test-tool genrandom one 200k | git hash-object -t blob -w --stdin) &&\n +\t\tblob_two=$(test-tool genrandom two 200k | git hash-object -t blob -w --stdin) &&\n -+\t\tprintf \"%s\\n\" \"$blob_one\" | git pack-objects .git/objects/pack/pack &&\n -+\t\tprintf \"%s\\n\" \"$blob_two\" | git pack-objects .git/objects/pack/pack &&\n -+\t\tremove_object \"$blob_one\" &&\n -+\t\tremove_object \"$blob_two\" &&\n -+\t\tgit -c core.bigFileThreshold=100k fsck\n ++\t\tblob_three=$(test-tool genrandom three 200k | git hash-object -t blob -w --stdin) &&\n ++\t\tblob_four=$(test-tool genrandom four 200k | git hash-object -t blob -w --stdin) &&\n ++\t\tpack_one=$(printf \"%s\\n\" \"$blob_one\" \"$blob_two\" \"$blob_three\" | git pack-objects .git/objects/pack/pack) &&\n ++\t\tpack_two=$(printf \"%s\\n\" \"$blob_two\" \"$blob_three\" \"$blob_four\" | git pack-objects .git/objects/pack/pack) &&\n ++\t\tchmod a+w .git/objects/pack/pack-*.pack &&\n ++\n ++\t\t# Corrupt blob two in the first pack.\n ++\t\tgit verify-pack -v .git/objects/pack/pack-$pack_one >objects &&\n ++\t\toffset_one=$(sed objects &&\n ++\t\toffset_two=$(sed err &&\n ++\t\ttest_grep \"unknown object type 0 at offset $offset_one in .git/objects/pack/pack-$pack_one.pack\" err &&\n ++\t\ttest_grep \"unknown object type 0 at offset $offset_two in .git/objects/pack/pack-$pack_two.pack\" err\n +\t)\n +'\n +\n\n---\nbase-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4\nchange-id: 20260223-pks-fsck-fix-aa8a18a223c8\n\n"},{"id":"536821","messageId":"20260223-pks-fsck-fix-v2-1-99a0714ea3bd@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v2-0-99a0714ea3bd@pks.im","subject":"[PATCH v2 1/4] t/helper: improve \"genrandom\" test helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T16:00:06Z","receivedAt":"2026-02-23T16:00:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `test-tool genrandom` test helper can be used to generate random\ndata, either as an infinite stream or with a specified number of bytes.\nThe way we handle parsing the number of bytes is lacking though:\n\n - We don't have good error handling, so if the caller for example uses\n `test-tool genrandom 200xyz` then we'll end up generating 200 bytes\n of random data successfully.\n\n - Many callers want to generate e.g. 1 kilobyte or megabyte of data,\n but they have to either use unwieldy numbers like 1048576, or they\n have to precompute them.\n\nFix both of these issues by using `git_parse_ulong()` to parse the\nargument. This function has better error handling, and it knows to\nhandle unit suffixes.\n\nAdapt a couple of our tests to use suffixes instead of manual\ncomputations.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/helper/test-genrandom.c | 5 ++++-\n t/t1006-cat-file.sh | 2 +-\n t/t1050-large.sh | 6 +++---\n t/t1450-fsck.sh | 2 +-\n t/t5301-sliding-window.sh | 2 +-\n t/t5310-pack-bitmaps.sh | 2 +-\n t/t5710-promisor-remote-capability.sh | 4 ++--\n t/t7700-repack.sh | 6 +++---\n 8 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/t/helper/test-genrandom.c b/t/helper/test-genrandom.c\nindex 51b67f2f87..d681961abb 100644\n--- a/t/helper/test-genrandom.c\n+++ b/t/helper/test-genrandom.c\n@@ -6,6 +6,7 @@\n \n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n+#include \"parse.h\"\n \n int cmd__genrandom(int argc, const char **argv)\n {\n@@ -22,7 +23,9 @@ int cmd__genrandom(int argc, const char **argv)\n \t\tnext = next * 11 + *c;\n \t} while (*c++);\n \n-\tcount = (argc == 3) ? strtoul(argv[2], NULL, 0) : ULONG_MAX;\n+\tcount = ULONG_MAX;\n+\tif (argc == 3 && !git_parse_ulong(argv[2], &count))\n+\t\treturn error_errno(\"cannot parse argument '%s'\", argv[2]);\n \n \twhile (count--) {\n \t\tnext = next * 1103515245 + 12345;\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 0eee3bb878..5499be8dc9 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -643,7 +643,7 @@ test_expect_success 'object reference via commit text search' '\n '\n \n test_expect_success 'setup blobs which are likely to delta' '\n-\ttest-tool genrandom foo 10240 >foo &&\n+\ttest-tool genrandom foo 10k >foo &&\n \t{ cat foo && echo plus; } >foo-plus &&\n \tgit add foo foo-plus &&\n \tgit commit -m foo &&\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex 5be273611a..7d40d08521 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -104,9 +104,9 @@ test_expect_success 'packsize limit' '\n \t\t# mid1 and mid2 will fit within 256k limit but\n \t\t# appending mid3 will bust the limit and will\n \t\t# result in a separate packfile.\n-\t\ttest-tool genrandom \"a\" $(( 66 * 1024 )) >mid1 &&\n-\t\ttest-tool genrandom \"b\" $(( 80 * 1024 )) >mid2 &&\n-\t\ttest-tool genrandom \"c\" $(( 128 * 1024 )) >mid3 &&\n+\t\ttest-tool genrandom \"a\" 66k >mid1 &&\n+\t\ttest-tool genrandom \"b\" 80k >mid2 &&\n+\t\ttest-tool genrandom \"c\" 128k >mid3 &&\n \t\tgit add mid1 mid2 mid3 &&\n \n \t\tcount=0 &&\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 3fae05f9d9..8fb79b3e5d 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -918,7 +918,7 @@ test_expect_success 'fsck detects trailing loose garbage (large blob)' '\n test_expect_success 'fsck detects truncated loose object' '\n \t# make it big enough that we know we will truncate in the data\n \t# portion, not the header\n-\ttest-tool genrandom truncate 4096 >file &&\n+\ttest-tool genrandom truncate 4k >file &&\n \tblob=$(git hash-object -w file) &&\n \tfile=$(sha1_file $blob) &&\n \ttest_when_finished \"remove_object $blob\" &&\ndiff --git a/t/t5301-sliding-window.sh b/t/t5301-sliding-window.sh\nindex ff6b5159a3..3c3666b278 100755\n--- a/t/t5301-sliding-window.sh\n+++ b/t/t5301-sliding-window.sh\n@@ -12,7 +12,7 @@ test_expect_success 'setup' '\n \tfor i in a b c\n \tdo\n \techo $i >$i &&\n-\ttest-tool genrandom \"$i\" 32768 >>$i &&\n+\ttest-tool genrandom \"$i\" 32k >>$i &&\n \tgit update-index --add $i || return 1\n \tdone &&\n \techo d >d && cat c >>d && git update-index --add d &&\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 6718fb98c0..3e3366f57d 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -242,7 +242,7 @@ test_bitmap_cases () {\n \t'\n \n \ttest_expect_success 'splitting packs does not generate bogus bitmaps' '\n-\t\ttest-tool genrandom foo $((1024 * 1024)) >rand &&\n+\t\ttest-tool genrandom foo 1m >rand &&\n \t\tgit add rand &&\n \t\tgit commit -m \"commit with big file\" &&\n \t\tgit -c pack.packSizeLimit=500k repack -adb &&\ndiff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh\nindex 023735d6a8..66af84cd56 100755\n--- a/t/t5710-promisor-remote-capability.sh\n+++ b/t/t5710-promisor-remote-capability.sh\n@@ -20,7 +20,7 @@ test_expect_success 'setup: create \"template\" repository' '\n \ttest_commit -C template 1 &&\n \ttest_commit -C template 2 &&\n \ttest_commit -C template 3 &&\n-\ttest-tool genrandom foo 10240 >template/foo &&\n+\ttest-tool genrandom foo 10k >template/foo &&\n \tgit -C template add foo &&\n \tgit -C template commit -m foo\n '\n@@ -376,7 +376,7 @@ test_expect_success \"clone with promisor.advertise set to 'true' but don't delet\n \n test_expect_success \"setup for subsequent fetches\" '\n \t# Generate new commit with large blob\n-\ttest-tool genrandom bar 10240 >template/bar &&\n+\ttest-tool genrandom bar 10k >template/bar &&\n \tgit -C template add bar &&\n \tgit -C template commit -m bar &&\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 73b78bdd88..439ab24d23 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -319,7 +319,7 @@ test_expect_success 'no bitmaps created if .keep files present' '\n \n test_expect_success 'auto-bitmaps do not complain if unavailable' '\n \ttest_config -C bare.git pack.packSizeLimit 1M &&\n-\tblob=$(test-tool genrandom big $((1024*1024)) |\n+\tblob=$(test-tool genrandom big 1m |\n \t git -C bare.git hash-object -w --stdin) &&\n \tgit -C bare.git update-ref refs/tags/big $blob &&\n \n@@ -495,9 +495,9 @@ test_expect_success '--filter works with --max-pack-size' '\n \t\tcd max-pack-size &&\n \t\ttest_commit base &&\n \t\t# two blobs which exceed the maximum pack size\n-\t\ttest-tool genrandom foo 1048576 >foo &&\n+\t\ttest-tool genrandom foo 1m >foo &&\n \t\tgit hash-object -w foo &&\n-\t\ttest-tool genrandom bar 1048576 >bar &&\n+\t\ttest-tool genrandom bar 1m >bar &&\n \t\tgit hash-object -w bar &&\n \t\tgit add foo bar &&\n \t\tgit commit -m \"adding foo and bar\"\n\n-- \n2.53.0.536.g309c995771.dirty\n\n"},{"id":"536822","messageId":"20260223-pks-fsck-fix-v2-2-99a0714ea3bd@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v2-0-99a0714ea3bd@pks.im","subject":"[PATCH v2 2/4] object-file: adapt `stream_object_signature()` to take a stream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T16:00:07Z","receivedAt":"2026-02-23T16:00:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `stream_object_signature()` is responsible for verifying\nwhether the given object ID matches the actual hash of the object's\ncontents. In contrast to `check_object_signature()` it does so in a\nstreaming fashion so that we don't have to load the full object into\nmemory.\n\nIn a subsequent commit we'll want to adapt one of its callsites to pass\na preconstructed stream. Prepare for this by accepting a stream as input\nthat the caller needs to assemble.\n\nWhile at it, improve the error reporting in `parse_object_with_flags()`\nto tell apart the two failure modes.\n\nHelped-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n object-file.c | 10 +++-------\n object-file.h | 4 +++-\n object.c | 19 ++++++++++++++++---\n pack-check.c | 12 +++++++++---\n 4 files changed, 31 insertions(+), 14 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 1b62996ef0..ca2c4dddf3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -129,18 +129,15 @@ int check_object_signature(struct repository *r, const struct object_id *oid,\n \treturn !oideq(oid, &real_oid) ? -1 : 0;\n }\n \n-int stream_object_signature(struct repository *r, const struct object_id *oid)\n+int stream_object_signature(struct repository *r,\n+\t\t\t struct odb_read_stream *st,\n+\t\t\t const struct object_id *oid)\n {\n \tstruct object_id real_oid;\n-\tstruct odb_read_stream *st;\n \tstruct git_hash_ctx c;\n \tchar hdr[MAX_HEADER_LEN];\n \tint hdrlen;\n \n-\tst = odb_read_stream_open(r->objects, oid, NULL);\n-\tif (!st)\n-\t\treturn -1;\n-\n \t/* Generate the header */\n \thdrlen = format_object_header(hdr, sizeof(hdr), st->type, st->size);\n \n@@ -160,7 +157,6 @@ int stream_object_signature(struct repository *r, const struct object_id *oid)\n \t\tgit_hash_update(&c, buf, readlen);\n \t}\n \tgit_hash_final_oid(&real_oid, &c);\n-\todb_read_stream_close(st);\n \treturn !oideq(oid, &real_oid) ? -1 : 0;\n }\n \ndiff --git a/object-file.h b/object-file.h\nindex a62d0de394..733d232309 100644\n--- a/object-file.h\n+++ b/object-file.h\n@@ -164,7 +164,9 @@ int check_object_signature(struct repository *r, const struct object_id *oid,\n * Try reading the object named with \"oid\" using\n * the streaming interface and rehash it to do the same.\n */\n-int stream_object_signature(struct repository *r, const struct object_id *oid);\n+int stream_object_signature(struct repository *r,\n+\t\t\t struct odb_read_stream *stream,\n+\t\t\t const struct object_id *oid);\n \n enum finalize_object_file_flags {\n \tFOF_SKIP_COLLISION_CHECK = 1,\ndiff --git a/object.c b/object.c\nindex 4669b8d65e..9d2c676b16 100644\n--- a/object.c\n+++ b/object.c\n@@ -6,6 +6,7 @@\n #include \"object.h\"\n #include \"replace-object.h\"\n #include \"object-file.h\"\n+#include \"odb/streaming.h\"\n #include \"blob.h\"\n #include \"statinfo.h\"\n #include \"tree.h\"\n@@ -330,9 +331,21 @@ struct object *parse_object_with_flags(struct repository *r,\n \n \tif ((!obj || obj->type == OBJ_NONE || obj->type == OBJ_BLOB) &&\n \t odb_read_object_info(r->objects, oid, NULL) == OBJ_BLOB) {\n-\t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n-\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n-\t\t\treturn NULL;\n+\t\tif (!skip_hash) {\n+\t\t\tstruct odb_read_stream *stream = odb_read_stream_open(r->objects, oid, NULL);\n+\n+\t\t\tif (!stream) {\n+\t\t\t\terror(_(\"unable to open object stream for %s\"), oid_to_hex(oid));\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\n+\t\t\tif (stream_object_signature(r, stream, repl) < 0) {\n+\t\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n+\t\t\t\todb_read_stream_close(stream);\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\n+\t\t\todb_read_stream_close(stream);\n \t\t}\n \t\tparse_blob_buffer(lookup_blob(r, oid));\n \t\treturn lookup_object(r, oid);\ndiff --git a/pack-check.c b/pack-check.c\nindex 67cb2cf72f..46782a29d5 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -9,6 +9,7 @@\n #include \"packfile.h\"\n #include \"object-file.h\"\n #include \"odb.h\"\n+#include \"odb/streaming.h\"\n \n struct idx_entry {\n \toff_t offset;\n@@ -104,6 +105,7 @@ static int verify_packfile(struct repository *r,\n \tQSORT(entries, nr_objects, compare_entries);\n \n \tfor (i = 0; i < nr_objects; i++) {\n+\t\tstruct odb_read_stream *stream = NULL;\n \t\tvoid *data;\n \t\tstruct object_id oid;\n \t\tenum object_type type;\n@@ -152,7 +154,9 @@ static int verify_packfile(struct repository *r,\n \t\t\t\t\t\t\ttype) < 0)\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\n-\t\telse if (!data && stream_object_signature(r, &oid) < 0)\n+\t\telse if (!data &&\n+\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n+\t\t\t stream_object_signature(r, stream, &oid) < 0))\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\n \t\telse if (fn) {\n@@ -163,12 +167,14 @@ static int verify_packfile(struct repository *r,\n \t\t}\n \t\tif (((base_count + i) & 1023) == 0)\n \t\t\tdisplay_progress(progress, base_count + i);\n-\t\tfree(data);\n \n+\t\tif (stream)\n+\t\t\todb_read_stream_close(stream);\n+\t\tfree(data);\n \t}\n+\n \tdisplay_progress(progress, base_count + i);\n \tfree(entries);\n-\n \treturn err;\n }\n \n\n-- \n2.53.0.536.g309c995771.dirty\n\n"},{"id":"536823","messageId":"20260223-pks-fsck-fix-v2-3-99a0714ea3bd@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v2-0-99a0714ea3bd@pks.im","subject":"[PATCH v2 3/4] packfile: expose function to read object stream for an offset","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T16:00:08Z","receivedAt":"2026-02-23T16:00:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `packfile_store_read_object_stream()` takes as input an\nobject ID and then constructs a `struct odb_read_stream` from it. In a\nsubsequent commit we'll want to create an object stream for a given\ncombination of packfile and offset though, which is not something that\ncan currently be done.\n\nExtract a new function `packfile_read_object_stream()` that makes this\nfunctionality available.\n\nSigned-off-by: Patrick Steinhardt \n---\n packfile.c | 40 ++++++++++++++++++++++++----------------\n packfile.h | 5 +++++\n 2 files changed, 29 insertions(+), 16 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 402c3b5dc7..3e61176128 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2553,32 +2553,28 @@ static int close_istream_pack_non_delta(struct odb_read_stream *_st)\n \treturn 0;\n }\n \n-int packfile_store_read_object_stream(struct odb_read_stream **out,\n-\t\t\t\t struct packfile_store *store,\n-\t\t\t\t const struct object_id *oid)\n+int packfile_read_object_stream(struct odb_read_stream **out,\n+\t\t\t\tconst struct object_id *oid,\n+\t\t\t\tstruct packed_git *pack,\n+\t\t\t\toff_t offset)\n {\n \tstruct odb_packed_read_stream *stream;\n \tstruct pack_window *window = NULL;\n-\tstruct object_info oi = OBJECT_INFO_INIT;\n \tenum object_type in_pack_type;\n \tunsigned long size;\n \n-\toi.sizep = &size;\n+\tin_pack_type = unpack_object_header(pack, &window, &offset, &size);\n+\tunuse_pack(&window);\n \n-\tif (packfile_store_read_object_info(store, oid, &oi, 0) ||\n-\t oi.u.packed.type == PACKED_OBJECT_TYPE_REF_DELTA ||\n-\t oi.u.packed.type == PACKED_OBJECT_TYPE_OFS_DELTA ||\n-\t repo_settings_get_big_file_threshold(store->source->odb->repo) >= size)\n+\tif (repo_settings_get_big_file_threshold(pack->repo) >= size)\n \t\treturn -1;\n \n-\tin_pack_type = unpack_object_header(oi.u.packed.pack,\n-\t\t\t\t\t &window,\n-\t\t\t\t\t &oi.u.packed.offset,\n-\t\t\t\t\t &size);\n-\tunuse_pack(&window);\n \tswitch (in_pack_type) {\n \tdefault:\n \t\treturn -1; /* we do not do deltas for now */\n+\tcase OBJ_BAD:\n+\t\tmark_bad_packed_object(pack, oid);\n+\t\treturn -1;\n \tcase OBJ_COMMIT:\n \tcase OBJ_TREE:\n \tcase OBJ_BLOB:\n@@ -2592,10 +2588,22 @@ int packfile_store_read_object_stream(struct odb_read_stream **out,\n \tstream->base.type = in_pack_type;\n \tstream->base.size = size;\n \tstream->z_state = ODB_PACKED_READ_STREAM_UNINITIALIZED;\n-\tstream->pack = oi.u.packed.pack;\n-\tstream->pos = oi.u.packed.offset;\n+\tstream->pack = pack;\n+\tstream->pos = offset;\n \n \t*out = &stream->base;\n \n \treturn 0;\n }\n+\n+int packfile_store_read_object_stream(struct odb_read_stream **out,\n+\t\t\t\t struct packfile_store *store,\n+\t\t\t\t const struct object_id *oid)\n+{\n+\tstruct pack_entry e;\n+\n+\tif (!find_pack_entry(store, oid, &e))\n+\t\treturn -1;\n+\n+\treturn packfile_read_object_stream(out, oid, e.p, e.offset);\n+}\ndiff --git a/packfile.h b/packfile.h\nindex acc5c55ad5..b9f5f1c18c 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -436,6 +436,11 @@ off_t get_delta_base(struct packed_git *p, struct pack_window **w_curs,\n \t\t off_t *curpos, enum object_type type,\n \t\t off_t delta_obj_offset);\n \n+int packfile_read_object_stream(struct odb_read_stream **out,\n+\t\t\t\tconst struct object_id *oid,\n+\t\t\t\tstruct packed_git *pack,\n+\t\t\t\toff_t offset);\n+\n void release_pack_memory(size_t);\n \n /* global flag to enable extra checks when accessing packed objects */\n\n-- \n2.53.0.536.g309c995771.dirty\n\n"},{"id":"536824","messageId":"20260223-pks-fsck-fix-v2-4-99a0714ea3bd@pks.im","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v2-0-99a0714ea3bd@pks.im","subject":"[PATCH v2 4/4] pack-check: fix verification of large objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T16:00:09Z","receivedAt":"2026-02-23T16:00:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported [1] that git-fsck(1) may sometimes run into an infinite\nloop when processing packfiles. This bug was bisected to c31bad4f7d\n(packfile: track packs via the MRU list exclusively, 2025-10-30), which\nrefactored our lsit of packfiles to only be tracked via an MRU list,\nexclusively. This isn't entirely surprising: any caller that iterates\nthrough the list of packfiles and then hits `find_pack_entry()`, for\nexample because they read an object from it, may cause the MRU list to\nbe updated. And if the caller is unlucky, this may cause the mentioned\ninfinite loop.\n\nWhile this mechanism is somewhat fragile, it is still surprising that we\nencounter it when verifying the packfile. We iterate through objects in\na given pack one by one and then read them via their offset, and doing\nthis shouldn't ever end up in `find_pack_entry()`.\n\nBut there is an edge case here: when the object in question is a blob\nbigger than \"core.largeFileThreshold\", then we will be careful to not\nread it into memory. Instead, we read it via an object stream by calling\n`odb_read_object_stream()`, and that function will perform an object\nlookup via `odb_read_object_info()`. So in the case where there are at\nleast two blobs in two different packfiles, and both of these blobs\nexceed \"core.largeFileThreshold\", then we'll run into an infinite loop\nbecause we'll always update the MRU.\n\nWe could fix this by improving `repo_for_each_pack()` to not update the\nMRU, and this would address the issue. But the fun part is that using\n`odb_read_object_stream()` is the wrong thing to do in the first place:\nit may open _any_ instance of this object, so we ultimately cannot be\nsure that we even verified the object in our given packfile.\n\nFix this bug by creating the object stream for the packed object\ndirectly via `packfile_read_object_stream()`. Add a test that would have\ncaused the infinite loop.\n\n[1]: <20260222183710.2963424-1-sandals@crustytoothpaste.net>\n\nReported-by: brian m. carlson \nSigned-off-by: Patrick Steinhardt \n---\n pack-check.c | 2 +-\n t/t1450-fsck.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex 46782a29d5..7378c80730 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -155,7 +155,7 @@ static int verify_packfile(struct repository *r,\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\n \t\telse if (!data &&\n-\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n+\t\t\t (packfile_read_object_stream(&stream, &oid, p, entries[i].offset) < 0 ||\n \t\t\t stream_object_signature(r, stream, &oid) < 0))\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t oid_to_hex(&oid), p->pack_name);\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8fb79b3e5d..54e81c2636 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -852,6 +852,44 @@ test_expect_success 'fsck errors in packed objects' '\n \t! grep corrupt out\n '\n \n+test_expect_success 'fsck handles multiple packfiles with big blobs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# We construct two packfiles with two objects in common and one\n+\t\t# object not in common. The objects in common can then be\n+\t\t# corrupted in one of the packfiles, respectively. The other\n+\t\t# objects that are unique to the packs are merely used to not\n+\t\t# have both packs contain the same data.\n+\t\tblob_one=$(test-tool genrandom one 200k | git hash-object -t blob -w --stdin) &&\n+\t\tblob_two=$(test-tool genrandom two 200k | git hash-object -t blob -w --stdin) &&\n+\t\tblob_three=$(test-tool genrandom three 200k | git hash-object -t blob -w --stdin) &&\n+\t\tblob_four=$(test-tool genrandom four 200k | git hash-object -t blob -w --stdin) &&\n+\t\tpack_one=$(printf \"%s\\n\" \"$blob_one\" \"$blob_two\" \"$blob_three\" | git pack-objects .git/objects/pack/pack) &&\n+\t\tpack_two=$(printf \"%s\\n\" \"$blob_two\" \"$blob_three\" \"$blob_four\" | git pack-objects .git/objects/pack/pack) &&\n+\t\tchmod a+w .git/objects/pack/pack-*.pack &&\n+\n+\t\t# Corrupt blob two in the first pack.\n+\t\tgit verify-pack -v .git/objects/pack/pack-$pack_one >objects &&\n+\t\toffset_one=$(sed objects &&\n+\t\toffset_two=$(sed err &&\n+\t\ttest_grep \"unknown object type 0 at offset $offset_one in .git/objects/pack/pack-$pack_one.pack\" err &&\n+\t\ttest_grep \"unknown object type 0 at offset $offset_two in .git/objects/pack/pack-$pack_two.pack\" err\n+\t)\n+'\n+\n test_expect_success 'fsck fails on corrupt packfile' '\n \thsh=$(git commit-tree -m mycommit HEAD^{tree}) &&\n \tpack=$(echo $hsh | git pack-objects .git/objects/pack/pack) &&\n\n-- \n2.53.0.536.g309c995771.dirty\n\n"},{"id":"536884","messageId":"xmqqsearkxjv.fsf@gitster.g","threadId":"65051","inReplyTo":"20260223-pks-fsck-fix-v1-4-c29036832b6e@pks.im","subject":"Re: [PATCH 4/4] pack-check: fix verification of large objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-23T20:35:48Z","receivedAt":"2026-02-23T20:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> It was reported [1] that git-fsck(1) may sometimes run into an infinite\n> loop when processing packfiles. This bug was bisected to c31bad4f7d\n> (packfile: track packs via the MRU list exclusively, 2025-10-30), which\n> refactored our lsit of packfiles to only be tracked via an MRU list,\n\n\"lsit of\" -> \"list of\"\n\n> exclusively. This isn't entirely surprising: any caller that iterates\n> through the list of packfiles and then hits `find_pack_entry()`, for\n> example because they read an object from it, may cause the MRU list to\n> be updated. And if the caller is unlucky, this may cause the mentioned\n> infinite loop.\n>\n> While this mechanism is somewhat fragile, it is still surprising that we\n> encounter it when verifying the packfile. We iterate through objects in\n> a given pack one by one and then read them via their offset, and doing\n> this shouldn't ever end up in `find_pack_entry()`.\n>\n> But there is an edge case here: when the object in question is a blob\n> bigger than \"core.largeFileThreshold\", then we will be careful to not\n> read it into memory. Instead, we read it via an object stream by calling\n> `odb_read_object_stream()`, and that function will perform an object\n> lookup via `odb_read_object_info()`. So in the case where there are at\n> least two blobs in two different packfiles, and both of these blobs\n> exceed \"core.largeFileThreshold\", then we'll run into an infinite loop\n> because we'll always update the MRU.\n\nGood find, and it is not surprising. What is surprising is that we\ndo not see this kind of breakage more often. The mechanism does\nsound fragile, not just \"somewhat\" X-<.\n\n> We could fix this by improving `repo_for_each_pack()` to not update the\n> MRU, and this would address the issue. But the fun part is that using\n> `odb_read_object_stream()` is the wrong thing to do in the first place:\n> it may open _any_ instance of this object, so we ultimately cannot be\n> sure that we even verified the object in our given packfile.\n\nAgain, very good reasoning.\n\n> Fix this bug by creating the object stream for the packed object\n> directly via `packfile_read_object_stream()`. Add a test that would have\n> caused the infinite loop.\n\nCurious that we have a completely different test. I've locally\napplied (without committing or amending) t1050 update from brian's\npatch and with this series, fsck there does not seem to get stuck.\nOf course, the new test added here doesn't either ;-).\n\n>\n> [1]: <20260222183710.2963424-1-sandals@crustytoothpaste.net>\n>\n> Reported-by: brian m. carlson \n> Signed-off-by: Patrick Steinhardt \n> ---\n> pack-check.c | 2 +-\n> t/t1450-fsck.sh | 15 +++++++++++++++\n> 2 files changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/pack-check.c b/pack-check.c\n> index 46782a29d5..6149567060 100644\n> --- a/pack-check.c\n> +++ b/pack-check.c\n> @@ -155,7 +155,7 @@ static int verify_packfile(struct repository *r,\n> \t\t\terr = error(\"packed %s from %s is corrupt\",\n> \t\t\t\t oid_to_hex(&oid), p->pack_name);\n> \t\telse if (!data &&\n> -\t\t\t (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) ||\n> +\t\t\t (packfile_read_object_stream(&stream, p, entries[i].offset) < 0 ||\n> \t\t\t stream_object_signature(r, stream, &oid) < 0))\n> \t\t\terr = error(\"packed %s from %s is corrupt\",\n> \t\t\t\t oid_to_hex(&oid), p->pack_name);\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 8fb79b3e5d..ec68397ea3 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -852,6 +852,21 @@ test_expect_success 'fsck errors in packed objects' '\n> \t! grep corrupt out\n> '\n> \n> +test_expect_success 'fsck handles multiple packfiles with big blobs' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tblob_one=$(test-tool genrandom one 200k | git hash-object -t blob -w --stdin) &&\n> +\t\tblob_two=$(test-tool genrandom two 200k | git hash-object -t blob -w --stdin) &&\n> +\t\tprintf \"%s\\n\" \"$blob_one\" | git pack-objects .git/objects/pack/pack &&\n> +\t\tprintf \"%s\\n\" \"$blob_two\" | git pack-objects .git/objects/pack/pack &&\n> +\t\tremove_object \"$blob_one\" &&\n> +\t\tremove_object \"$blob_two\" &&\n> +\t\tgit -c core.bigFileThreshold=100k fsck\n> +\t)\n> +'\n> +\n> test_expect_success 'fsck fails on corrupt packfile' '\n> \thsh=$(git commit-tree -m mycommit HEAD^{tree}) &&\n> \tpack=$(echo $hsh | git pack-objects .git/objects/pack/pack) &&\n"},{"id":"536920","messageId":"aZ1EpbaPfILWFbcT@pks.im","threadId":"65051","inReplyTo":"xmqqsearkxjv.fsf@gitster.g","subject":"Re: [PATCH 4/4] pack-check: fix verification of large objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-24T06:26:45Z","receivedAt":"2026-02-24T06:26:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 12:35:48PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> > Fix this bug by creating the object stream for the packed object\n> > directly via `packfile_read_object_stream()`. Add a test that would have\n> > caused the infinite loop.\n> \n> Curious that we have a completely different test. I've locally\n> applied (without committing or amending) t1050 update from brian's\n> patch and with this series, fsck there does not seem to get stuck.\n> Of course, the new test added here doesn't either ;-).\n\nYeah, the fact that the test in t1050 hit the bug was pure coincidence,\nand I wanted to have something a bit more reliable that also allowed\nmyself to prove what exact circumstances cause this to fail.\n\nPatrick\n"}]}