From: Patrick Steinhardt Date: Mon, 23 Feb 2026 16:00:05 GMT Subject: [PATCH v2 0/4] pack-check: fix verification of large objects Message-ID: <20260223-pks-fsck-fix-v2-0-99a0714ea3bd@pks.im> In-Reply-To: <20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im> Hi, this small patch series addresses the bug reported by brian in [1]. Thanks! Changes in v2: - Extend the test to verify that we actually find corrupted objects in both packs, even in the case where a non-corrupt version exists in another pack. - Reinstate `mark_packed_object_bad()`. - Fix error checking for `git_parse_ulong()`. - Disambiguate error conditions in `parse_object_with_flags()`. - Link to v1: https://lore.kernel.org/r/20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im Patrick [1]: <20260222183710.2963424-1-sandals@crustytoothpaste.net> --- Patrick Steinhardt (4): t/helper: improve "genrandom" test helper object-file: adapt `stream_object_signature()` to take a stream packfile: expose function to read object stream for an offset pack-check: fix verification of large objects object-file.c | 10 +++------ object-file.h | 4 +++- object.c | 19 ++++++++++++++--- pack-check.c | 12 ++++++++--- packfile.c | 40 +++++++++++++++++++++-------------- packfile.h | 5 +++++ t/helper/test-genrandom.c | 5 ++++- t/t1006-cat-file.sh | 2 +- t/t1050-large.sh | 6 +++--- t/t1450-fsck.sh | 40 ++++++++++++++++++++++++++++++++++- t/t5301-sliding-window.sh | 2 +- t/t5310-pack-bitmaps.sh | 2 +- t/t5710-promisor-remote-capability.sh | 4 ++-- t/t7700-repack.sh | 6 +++--- 14 files changed, 114 insertions(+), 43 deletions(-) Range-diff versus v1: 1: 1b1283e837 ! 1: daf895aef6 t/helper: improve "genrandom" test helper @@ Commit message have to precompute them. Fix both of these issues by using `git_parse_ulong()` to parse the - argumemnt. This function has better error handling, and it knows to + argument. This function has better error handling, and it knows to handle unit suffixes. Adapt a couple of our tests to use suffixes instead of manual @@ t/helper/test-genrandom.c: int cmd__genrandom(int argc, const char **argv) - count = (argc == 3) ? strtoul(argv[2], NULL, 0) : ULONG_MAX; + count = ULONG_MAX; -+ if (argc == 3 && git_parse_ulong(argv[2], &count) < 0) ++ if (argc == 3 && !git_parse_ulong(argv[2], &count)) + return error_errno("cannot parse argument '%s'", argv[2]); while (count--) { 2: 9f25ed1a4b ! 2: ebca9efaec object-file: adapt `stream_object_signature()` to take a stream @@ Commit message a preconstructed stream. Prepare for this by accepting a stream as input that the caller needs to assemble. + While at it, improve the error reporting in `parse_object_with_flags()` + to tell apart the two failure modes. + + Helped-by: Jeff King Signed-off-by: Patrick Steinhardt ## object-file.c ## @@ object.c: struct object *parse_object_with_flags(struct repository *r, - return NULL; + if (!skip_hash) { + struct odb_read_stream *stream = odb_read_stream_open(r->objects, oid, NULL); -+ if (!stream || stream_object_signature(r, stream, repl) < 0) { -+ error(_("hash mismatch %s"), oid_to_hex(oid)); -+ if (stream) -+ odb_read_stream_close(stream); ++ ++ if (!stream) { ++ error(_("unable to open object stream for %s"), oid_to_hex(oid)); + return NULL; + } + -+ if (stream) ++ if (stream_object_signature(r, stream, repl) < 0) { ++ error(_("hash mismatch %s"), oid_to_hex(oid)); + odb_read_stream_close(stream); ++ return NULL; ++ } ++ ++ odb_read_stream_close(stream); } parse_blob_buffer(lookup_blob(r, oid)); return lookup_object(r, oid); 3: 9a28867564 ! 3: bead96797e packfile: expose function to read object stream for an offset @@ packfile.c: static int close_istream_pack_non_delta(struct odb_read_stream *_st) - struct packfile_store *store, - const struct object_id *oid) +int packfile_read_object_stream(struct odb_read_stream **out, ++ const struct object_id *oid, + struct packed_git *pack, + off_t offset) { @@ packfile.c: static int close_istream_pack_non_delta(struct odb_read_stream *_st) switch (in_pack_type) { default: return -1; /* we do not do deltas for now */ ++ case OBJ_BAD: ++ mark_bad_packed_object(pack, oid); ++ return -1; + case OBJ_COMMIT: + case OBJ_TREE: + case OBJ_BLOB: @@ packfile.c: int packfile_store_read_object_stream(struct odb_read_stream **out, stream->base.type = in_pack_type; stream->base.size = size; @@ packfile.c: int packfile_store_read_object_stream(struct odb_read_stream **out, + if (!find_pack_entry(store, oid, &e)) + return -1; + -+ return packfile_read_object_stream(out, e.p, e.offset); ++ return packfile_read_object_stream(out, oid, e.p, e.offset); +} ## packfile.h ## @@ packfile.h: off_t get_delta_base(struct packed_git *p, struct pack_window **w_cu off_t delta_obj_offset); +int packfile_read_object_stream(struct odb_read_stream **out, ++ const struct object_id *oid, + struct packed_git *pack, + off_t offset); + 4: 4eaf958e57 ! 4: 6b69624d81 pack-check: fix verification of large objects @@ pack-check.c: static int verify_packfile(struct repository *r, oid_to_hex(&oid), p->pack_name); else if (!data && - (!(stream = odb_read_stream_open(r->objects, &oid, NULL)) || -+ (packfile_read_object_stream(&stream, p, entries[i].offset) < 0 || ++ (packfile_read_object_stream(&stream, &oid, p, entries[i].offset) < 0 || stream_object_signature(r, stream, &oid) < 0)) err = error("packed %s from %s is corrupt", oid_to_hex(&oid), p->pack_name); @@ t/t1450-fsck.sh: test_expect_success 'fsck errors in packed objects' ' + git init repo && + ( + cd repo && ++ ++ # We construct two packfiles with two objects in common and one ++ # object not in common. The objects in common can then be ++ # corrupted in one of the packfiles, respectively. The other ++ # objects that are unique to the packs are merely used to not ++ # have both packs contain the same data. + blob_one=$(test-tool genrandom one 200k | git hash-object -t blob -w --stdin) && + blob_two=$(test-tool genrandom two 200k | git hash-object -t blob -w --stdin) && -+ printf "%s\n" "$blob_one" | git pack-objects .git/objects/pack/pack && -+ printf "%s\n" "$blob_two" | git pack-objects .git/objects/pack/pack && -+ remove_object "$blob_one" && -+ remove_object "$blob_two" && -+ git -c core.bigFileThreshold=100k fsck ++ blob_three=$(test-tool genrandom three 200k | git hash-object -t blob -w --stdin) && ++ blob_four=$(test-tool genrandom four 200k | git hash-object -t blob -w --stdin) && ++ pack_one=$(printf "%s\n" "$blob_one" "$blob_two" "$blob_three" | git pack-objects .git/objects/pack/pack) && ++ pack_two=$(printf "%s\n" "$blob_two" "$blob_three" "$blob_four" | git pack-objects .git/objects/pack/pack) && ++ chmod a+w .git/objects/pack/pack-*.pack && ++ ++ # Corrupt blob two in the first pack. ++ git verify-pack -v .git/objects/pack/pack-$pack_one >objects && ++ offset_one=$(sed objects && ++ offset_two=$(sed err && ++ test_grep "unknown object type 0 at offset $offset_one in .git/objects/pack/pack-$pack_one.pack" err && ++ test_grep "unknown object type 0 at offset $offset_two in .git/objects/pack/pack-$pack_two.pack" err + ) +' + --- base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4 change-id: 20260223-pks-fsck-fix-aa8a18a223c8