# [PATCH 0/2] packfile: fix corruption due to stale delta base cache entries 11 messages from 2026-10-02 to 2026-10-05. Participants: Patrick Steinhardt, Guillaume Chauvel, Philippe Blain, Mark C. Chu-Carroll, Jeff King. Thread: https://gitlist.dev/t/66443 ## Patrick Steinhardt, 2026-10-02 07:34 Subject: [PATCH 0/2] packfile: fix corruption due to stale delta base cache entries Message-ID: <20261002-pks-packfile-stale-delta-base-cache-v1-0-7592a3e31ae0@pks.im> ``` Hi, this small patch series fixes the bug reported in [1]. To summarize: we never evict delta base cache entries when closing the owning pack. The cache may thus contain stale entries which are keyed by by the memory address of `struct packed_git` and the offset of the entry in the packfile. Now when allocating a new pack that happens to have the exact same address and that has entries sitting at the same offset, we may try to use these stale entries and thus yield corrupted data. This all sounds very unlikely, but the interesting part is that this can be reproduced by using recursive merges with submodules, as we open and close the object databases of each respective submodule. And if they have similar packfiles, then we may trigger the bug. The series is built on top of v2.56.0. Thanks! Patrick [1]: --- Patrick Steinhardt (2): packfile: move around `close_pack()` packfile: fix corruption due to stale delta base cache entries packfile.c | 33 ++++++++++++++++++++++---------- t/t6437-submodule-merge.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 10 deletions(-) --- base-commit: a018953688f1b10bddf91bff8747068f5f4746a4 change-id: 20261002-pks-packfile-stale-delta-base-cache-0d4730487643 ``` ## Patrick Steinhardt, 2026-10-02 07:34 Subject: [PATCH 1/2] packfile: move around `close_pack()` Message-ID: <20261002-pks-packfile-stale-delta-base-cache-v1-1-7592a3e31ae0@pks.im> In-Reply-To: <20261002-pks-packfile-stale-delta-base-cache-v1-0-7592a3e31ae0@pks.im> ``` In the next commit we'll want to access the delta base cache in `close_pack()`. Move the function after the declaration of the cache to prepare for this. Signed-off-by: Patrick Steinhardt --- packfile.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packfile.c b/packfile.c index 4fa5fd67c8..af1b837974 100644 --- a/packfile.c +++ b/packfile.c @@ -355,16 +355,6 @@ static void close_pack_mtimes(struct packed_git *p) p->mtimes_map = NULL; } -void close_pack(struct packed_git *p) -{ - close_pack_windows(p); - close_pack_fd(p); - close_pack_index(p); - close_pack_revindex(p); - close_pack_mtimes(p); - oidset_clear(&p->bad_objects); -} - void unlink_pack_path(const char *pack_name, int force_delete) { static const char *exts[] = {".idx", ".pack", ".rev", ".keep", ".bitmap", ".promisor", ".mtimes"}; @@ -1263,6 +1253,16 @@ void clear_delta_base_cache(void) } } +void close_pack(struct packed_git *p) +{ + close_pack_windows(p); + close_pack_fd(p); + close_pack_index(p); + close_pack_revindex(p); + close_pack_mtimes(p); + oidset_clear(&p->bad_objects); +} + static void add_delta_base_cache(struct packed_git *p, off_t base_offset, void *base, size_t base_size, size_t delta_base_cache_limit, -- 2.56.0.353.g0856645cf6.dirty ``` ## Patrick Steinhardt, 2026-10-02 07:34 Subject: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries Message-ID: <20261002-pks-packfile-stale-delta-base-cache-v1-2-7592a3e31ae0@pks.im> In-Reply-To: <20261002-pks-packfile-stale-delta-base-cache-v1-0-7592a3e31ae0@pks.im> ``` The delta base cache is a process-global hashmap that is keyed by the address of the `struct packed_git` plus the offset of the base object within that pack. Entries part of the cache are never removed when a pack is closed, and neither when the pack is subsequently freed. As a consequence, the cache may contain stale entries. For a long time, the worst consequence of this leaking cache was that we held on to memory that we could've released. But the reason for this was that we didn't even free the packfiles, either. That has changed in 6f1e9394e2 (object: fix leaking packfiles when closing object store, 2024-08-08), where we plugged that leak. Now that we free them, a new packfile may be allocated using the exact same address as a previously allocated one. And if the new packfile has both the same address and a similar layout, it may happen that a preexisting entry from a previously-allocated in the delta base cache would have the exact same key. All of this sounds very theoretical, but we can actually trigger this bug somewhat reliably! When doing a merge with "--recurse-submodules" in a repository with lots of submodules that have similar-looking packfiles we end up opening and then closing the object databases of each of the submodules in sequence. Because of the above mentioned commit we would close and free each of the packfiles part of the respective databases, but we wouldn't evict thire delta base entries from the cache. When using glibc, one of the packfiles will eventually get the exact same address, and that will then cause Git to read the wrong entry from the cache. Git detects this and aborts with an error: $ git merge branch-b error: Could not read 584ef938be4a749bfa13f68d5ac5545bc029e529 error: could not parse commit 584ef938be4a749bfa13f68d5ac5545bc029e529 error: failed to merge submodule G (repository corrupt) Now in this case we're lucky that Git detects this error because we try to read a commit from a different submodule via an object database that doesn't have it. But potentially, in an even more contrived scenario, we might even silently yield wrong data from the cache. Fix this bug by evicting cache entries that belong to a specific pack when closing it. Note that the added test reliably reproduces the above bug on my machine that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on specific allocation behaviour of glibc it is very likely that the test will not work on other platforms. Reported-by: Guillaume Chauvel Helped-by: Philippe Blain Signed-off-by: Patrick Steinhardt --- packfile.c | 13 +++++++++++++ t/t6437-submodule-merge.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/packfile.c b/packfile.c index af1b837974..365c54c7dc 100644 --- a/packfile.c +++ b/packfile.c @@ -1253,6 +1253,18 @@ void clear_delta_base_cache(void) } } +static void delta_base_cache_evict_entry(struct packed_git *p) +{ + struct list_head *lru, *tmp; + + list_for_each_safe(lru, tmp, &delta_base_cache_lru) { + struct delta_base_cache_entry *entry = + list_entry(lru, struct delta_base_cache_entry, lru); + if (entry->key.p == p) + release_delta_base_cache(entry); + } +} + void close_pack(struct packed_git *p) { close_pack_windows(p); @@ -1261,6 +1273,7 @@ void close_pack(struct packed_git *p) close_pack_revindex(p); close_pack_mtimes(p); oidset_clear(&p->bad_objects); + delta_base_cache_evict_entry(p); } static void add_delta_base_cache(struct packed_git *p, off_t base_offset, diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh index 1546d5f773..0ee3684206 100755 --- a/t/t6437-submodule-merge.sh +++ b/t/t6437-submodule-merge.sh @@ -514,4 +514,51 @@ test_expect_success 'merging should fail with no merge base' ' ) ' +test_expect_success 'merge with many packed submodules reports conflicts' ' + test_config_global protocol.file.allow always && + + # Create 16 submodules with two divergent branches each. + submodules="A B C D E F G H I J K L M N O P" && + for name in $submodules + do + git init source-$name && + test_commit -C source-$name $name-main && + git -C source-$name switch --create branch-a main && + git -C source-$name commit --allow-empty --message $name-branch-a && + git -C source-$name switch --create branch-b main && + git -C source-$name commit --allow-empty --message $name-branch-b || return 1 + done && + + # Create the superproject and add all submodules. + git init many-packed && + for name in $submodules + do + git -C many-packed submodule add --branch main "file://$PWD/source-$name" $name || return 1 + done && + git -C many-packed commit --message main && + + # Create two divergent commits in the superproject that update all + # submodules to the divergent branches. + for branch in branch-a branch-b + do + git -C many-packed switch -c $branch main && + for name in $submodules + do + git -C many-packed/$name switch $branch || return 1 + done && + git -C many-packed add $submodules && + git -C many-packed commit --message $branch || return 1 + done && + + # Clone the superproject to ensure that everything is well-packed and + # then merge the two branches, creating conflicts for every submodule. + git clone many-packed many-packed-clone && + git -C many-packed-clone submodule update --init && + git -C many-packed-clone switch branch-a && + test_expect_code 1 git -C many-packed-clone -c advice.submoduleMergeConflict=false merge branch-b >out 2>err && + grep "^CONFLICT (submodule)" out >conflicts && + test_line_count = 16 conflicts && + test_must_be_empty err +' + test_done -- 2.56.0.353.g0856645cf6.dirty ``` ## Guillaume Chauvel, 2026-10-02 13:34 Subject: Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries Message-ID: <20261002133405.1284-1-guillaume.chauvel@gmail.com> In-Reply-To: <20261002-pks-packfile-stale-delta-base-cache-v1-2-7592a3e31ae0@pks.im> ``` On Fri, Oct 02, 2026 at 09:34:07AM +0200, Patrick Steinhardt wrote: > Note that the added test reliably reproduces the above bug on my machine > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on > specific allocation behaviour of glibc it is very likely that the test > will not work on other platforms. What about forcing the address reuse in the test for deterministic behavior ? A test helper can close a pack and move a second packed_git, whose delta base sits at the same offset, into its memory. This was written with the help of AI tools. To apply on top of [PATCH 1/2]. -- >8 -- Makefile | 1 + packfile.c | 13 ++++++ t/helper/meson.build | 1 + t/helper/test-delta-base-cache.c | 92 ++++++++++++++++++++++++++++++++++++++++ t/helper/test-pack-deltas.c | 22 +++++++++- t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/meson.build | 1 + t/t5336-pack-delta-base-cache.sh | 46 ++++++++++++++++++++ 9 files changed, 177 insertions(+), 1 deletion(-) create mode 100644 t/helper/test-delta-base-cache.c create mode 100755 t/t5336-pack-delta-base-cache.sh diff --git a/Makefile b/Makefile index c649c93c51..771ca00e33 100644 --- a/Makefile +++ b/Makefile @@ -818,6 +818,7 @@ TEST_BUILTINS_OBJS += test-crontab.o TEST_BUILTINS_OBJS += test-csprng.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delete-gpgsig.o +TEST_BUILTINS_OBJS += test-delta-base-cache.o TEST_BUILTINS_OBJS += test-delta.o TEST_BUILTINS_OBJS += test-dir-iterator.o TEST_BUILTINS_OBJS += test-drop-caches.o diff --git a/packfile.c b/packfile.c index af1b837974..365c54c7dc 100644 --- a/packfile.c +++ b/packfile.c @@ -1253,6 +1253,18 @@ void clear_delta_base_cache(void) } } +static void delta_base_cache_evict_entry(struct packed_git *p) +{ + struct list_head *lru, *tmp; + + list_for_each_safe(lru, tmp, &delta_base_cache_lru) { + struct delta_base_cache_entry *entry = + list_entry(lru, struct delta_base_cache_entry, lru); + if (entry->key.p == p) + release_delta_base_cache(entry); + } +} + void close_pack(struct packed_git *p) { close_pack_windows(p); @@ -1261,6 +1273,7 @@ void close_pack(struct packed_git *p) close_pack_revindex(p); close_pack_mtimes(p); oidset_clear(&p->bad_objects); + delta_base_cache_evict_entry(p); } static void add_delta_base_cache(struct packed_git *p, off_t base_offset, diff --git a/t/helper/meson.build b/t/helper/meson.build index 3235f10ab8..225c46a88c 100644 --- a/t/helper/meson.build +++ b/t/helper/meson.build @@ -11,6 +11,7 @@ test_tool_sources = [ 'test-csprng.c', 'test-date.c', 'test-delete-gpgsig.c', + 'test-delta-base-cache.c', 'test-delta.c', 'test-dir-iterator.c', 'test-drop-caches.c', diff --git a/t/helper/test-delta-base-cache.c b/t/helper/test-delta-base-cache.c new file mode 100644 index 0000000000..54a0d43f2e --- /dev/null +++ b/t/helper/test-delta-base-cache.c @@ -0,0 +1,92 @@ +#define USE_THE_REPOSITORY_VARIABLE + +#include "test-tool.h" +#include "hex.h" +#include "object-file.h" +#include "packfile.h" +#include "setup.h" + +static off_t delta_base_offset(struct packed_git *pack, + const struct object_id *oid) +{ + struct pack_window *window = NULL; + off_t offset, pos, base; + size_t size; + int type; + + offset = find_pack_entry_one(oid, pack); + if (!offset) + die("object is missing from pack: %s", oid_to_hex(oid)); + pos = offset; + type = unpack_object_header(pack, &window, &pos, &size); + if (type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA) + die("object is not stored as a delta: %s", oid_to_hex(oid)); + base = get_delta_base(pack, &window, &pos, type, offset); + unuse_pack(&window); + if (!base) + die("cannot locate delta base for %s", oid_to_hex(oid)); + return base; +} + +int cmd__delta_base_cache(int argc, const char **argv) +{ + struct packed_git *first, *second; + struct object_id first_oid, second_oid, actual_oid; + enum object_type type; + size_t size; + void *data; + + if (argc != 5) + usage("test-tool delta-base-cache " + " "); + + setup_git_directory(the_repository); + + if (get_oid_hex(argv[2], &first_oid) || get_oid_hex(argv[4], &second_oid)) + die("invalid object ID"); + if (strlen(argv[1]) != strlen(argv[3])) + die("pack index paths must have the same length"); + + first = add_packed_git(the_repository, argv[1], strlen(argv[1]), 1); + second = add_packed_git(the_repository, argv[3], strlen(argv[3]), 1); + if (!first || !second || open_pack_index(first) || open_pack_index(second)) + die("cannot open pack indexes"); + + if (delta_base_offset(first, &first_oid) != + delta_base_offset(second, &second_oid)) + die("delta bases have different pack offsets"); + + data = unpack_entry(the_repository, first, + find_pack_entry_one(&first_oid, first), NULL, NULL); + if (!data) + die("cannot unpack first object"); + free(data); + + close_pack(first); + + /* + * Simulate the allocator handing the address of the closed pack to + * a new one. The resources of "second" now belong to "first". + */ + memcpy(first, second, sizeof(*first) + strlen(second->pack_name) + 1); + free(second); + + data = unpack_entry(the_repository, first, + find_pack_entry_one(&second_oid, first), &type, &size); + if (!data) + die("cannot unpack second object"); + hash_object_file(the_repository->hash_algo, data, size, type, &actual_oid); + free(data); + close_pack(first); + free(first); + + /* + * The test checked that applying the second object's delta to the + * first pack's base does not give the second object, so a stale + * cache entry shows up as an object ID mismatch. + */ + if (!oideq(&actual_oid, &second_oid)) + return error("second object differs after pack reuse"); + + return 0; +} diff --git a/t/helper/test-pack-deltas.c b/t/helper/test-pack-deltas.c index 959705feca..6cd5a7a7d8 100644 --- a/t/helper/test-pack-deltas.c +++ b/t/helper/test-pack-deltas.c @@ -43,6 +43,26 @@ static unsigned long do_compress(void **pptr, unsigned long size) return stream.total_out; } +static void write_full(struct hashfile *f, struct object_id *oid) +{ + unsigned char header[MAX_PACK_OBJECT_HEADER]; + unsigned long compressed_size, hdrlen; + size_t size; + enum object_type type; + void *buf = odb_read_object(the_repository->objects, + oid, &type, &size); + + if (!buf) + die("unable to read %s", oid_to_hex(oid)); + + compressed_size = do_compress(&buf, cast_size_t_to_ulong(size)); + hdrlen = encode_in_pack_object_header(header, sizeof(header), + type, size); + hashwrite(f, header, hdrlen); + hashwrite(f, buf, compressed_size); + free(buf); +} + static void write_ref_delta(struct hashfile *f, struct object_id *oid, struct object_id *base) @@ -136,7 +156,7 @@ int cmd__pack_deltas(int argc, const char **argv) else if (!strcmp(type_str, "OFS_DELTA")) die("OFS_DELTA not implemented"); else if (!strcmp(type_str, "FULL")) - die("FULL not implemented"); + write_full(f, &content_oid); else die("unknown pack type: %s", type_str); } diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index b71a22b43b..83b2e99344 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -22,6 +22,7 @@ static struct test_cmd cmds[] = { { "date", cmd__date }, { "delete-gpgsig", cmd__delete_gpgsig }, { "delta", cmd__delta }, + { "delta-base-cache", cmd__delta_base_cache }, { "dir-iterator", cmd__dir_iterator }, { "drop-caches", cmd__drop_caches }, { "dump-cache-tree", cmd__dump_cache_tree }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index f2885b33d5..8c95a1dada 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -14,6 +14,7 @@ int cmd__crontab(int argc, const char **argv); int cmd__csprng(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); +int cmd__delta_base_cache(int argc, const char **argv); int cmd__delete_gpgsig(int argc, const char **argv); int cmd__dir_iterator(int argc, const char **argv); int cmd__drop_caches(int argc, const char **argv); diff --git a/t/meson.build b/t/meson.build index 3ca7b27104..7d01040a07 100644 --- a/t/meson.build +++ b/t/meson.build @@ -639,6 +639,7 @@ integration_tests = [ 't5333-pseudo-merge-bitmaps.sh', 't5334-incremental-multi-pack-index.sh', 't5335-compact-multi-pack-index.sh', + 't5336-pack-delta-base-cache.sh', 't5351-unpack-large-objects.sh', 't5400-send-pack.sh', 't5401-update-hooks.sh', diff --git a/t/t5336-pack-delta-base-cache.sh b/t/t5336-pack-delta-base-cache.sh new file mode 100755 index 0000000000..2abc12559a --- /dev/null +++ b/t/t5336-pack-delta-base-cache.sh @@ -0,0 +1,46 @@ +#!/bin/sh + +test_description='delta base cache lifetime across pack closure' + +. ./test-lib.sh + +# The delta base cache is keyed by (packed_git pointer, base offset). +# Reading B from A-B.pack caches A. +# The helper then closes that pack and, since B has the same offset +# as A, reuses its packed_git structure for B-C.pack to reproduce +# the same cache key. If closing the pack leaves A in the cache, +# reading C would use A instead of B as its delta base, which the +# helper detects by checking the resulting object ID. +test_expect_success 'delta base cache entries do not outlive their pack' ' + test-tool genrandom cache-data 1024 >common && + { printf "a0" && cat common; } >a && + { printf "b1" && cat common; } >b && + { printf "c1" && cat common; } >c && + A=$(git hash-object -w a) && + B=$(git hash-object -w b) && + C=$(git hash-object -w c) && + + # Applying the B-to-C delta to A must not give C. Otherwise the + # helper could not detect a stale cache entry from the object ID. + test-tool delta -d b c b-c.delta && + test-tool delta -p a b-c.delta stale && + ! cmp -s c stale && + + test-tool pack-deltas --num-objects=2 >A-B.pack <<-EOF && + FULL $A + REF_DELTA $B $A + EOF + test-tool pack-deltas --num-objects=2 >B-C.pack <<-EOF && + FULL $B + REF_DELTA $C $B + EOF + + git index-pack -o A-B.idx A-B.pack && + git index-pack -o B-C.idx B-C.pack && + + test-tool delta-base-cache \ + A-B.idx $B \ + B-C.idx $C +' + +test_done ``` ## Philippe Blain, 2026-10-02 17:48 Subject: Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries Message-ID: <046C5954-DB91-4B7B-A89C-70B418CFDFB7@gmail.com> In-Reply-To: <20261002-pks-packfile-stale-delta-base-cache-v1-2-7592a3e31ae0@pks.im> ``` Hi Patrick, > Le 2 oct. 2026 à 03:34, Patrick Steinhardt a écrit : > > The delta base cache is a process-global hashmap that is keyed by the > address of the `struct packed_git` plus the offset of the base object > within that pack. Entries part of the cache are never removed when a > pack is closed, and neither when the pack is subsequently freed. As a > consequence, the cache may contain stale entries. > > For a long time, the worst consequence of this leaking cache was that we > held on to memory that we could've released. But the reason for this was > that we didn't even free the packfiles, either. That has changed in > 6f1e9394e2 (object: fix leaking packfiles when closing object store, > 2024-08-08), where we plugged that leak. > > Now that we free them, a new packfile may be allocated using the exact > same address as a previously allocated one. And if the new packfile has > both the same address and a similar layout, it may happen that a > preexisting entry from a previously-allocated in the delta base cache > would have the exact same key. > > All of this sounds very theoretical, but we can actually trigger this > bug somewhat reliably! When doing a merge with "--recurse-submodules" in > a repository with lots of submodules that have similar-looking packfiles merge does not have a --recurse-submodules flag, submodules are merged by default (but not updated after the merge, which would be what the flag would do if it existed :)) > we end up opening and then closing the object databases of each of the > submodules in sequence. Because of the above mentioned commit we would > close and free each of the packfiles part of the respective databases, > but we wouldn't evict thire delta base entries from the cache. s/thire/their > > When using glibc, one of the packfiles will eventually get the exact > same address, and that will then cause Git to read the wrong entry from > the cache. Git detects this and aborts with an error: > > $ git merge branch-b > error: Could not read 584ef938be4a749bfa13f68d5ac5545bc029e529 > error: could not parse commit 584ef938be4a749bfa13f68d5ac5545bc029e529 > error: failed to merge submodule G (repository corrupt) > > Now in this case we're lucky that Git detects this error because we try > to read a commit from a different submodule via an object database that > doesn't have it. But potentially, in an even more contrived scenario, we > might even silently yield wrong data from the cache. > > Fix this bug by evicting cache entries that belong to a specific pack > when closing it. > > Note that the added test reliably reproduces the above bug on my machine > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on > specific allocation behaviour of glibc it is very likely that the test > will not work on other platforms. > > Reported-by: Guillaume Chauvel > Helped-by: Philippe Blain > Signed-off-by: Patrick Steinhardt Thanks for the trailer and the quick fix !! I’m still puzzled why it worked correctly on 2.56.0-rc1 on my WSL instance. From your commit message, I guess for some reason I get different adresses and so the bug does not trigger. I see you the test you add merges more than two submodules, in contrast to Guillaume’s reproducer. Is that necessary for the bug to trigger for you? Cheers, Philippe. ``` ## Mark C. Chu-Carroll, 2026-10-02 19:02 Subject: Re: [PATCH 1/2] packfile: move around `close_pack()` Message-ID: In-Reply-To: <20261002-pks-packfile-stale-delta-base-cache-v1-1-7592a3e31ae0@pks.im> ``` On Fri Oct 2, 2026 at 3:34 AM EDT, Patrick Steinhardt wrote: > In the next commit we'll want to access the delta base cache in > `close_pack()`. Move the function after the declaration of the cache to > prepare for this. Maybe I'm just being clueless, but how does moving an unmodified function help with the subsequent change? -- Mark Craig Chu-Carroll (@MarkChuCarroll at gitlab) *** Software Tools/Math Geek - Software Engineer at Gitlab *** Work Email: mcarroll@gitlab.com / markchucarroll@fastmail.com *** Personal Blog: http://goodmath.org/blog / Personal email: markcc@gmail.com ``` ## Patrick Steinhardt, 2026-10-02 19:34 Subject: Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries Message-ID: In-Reply-To: <20261002133405.1284-1-guillaume.chauvel@gmail.com> ``` On Fri, Oct 02, 2026 at 03:34:05PM +0200, Guillaume Chauvel wrote: > On Fri, Oct 02, 2026 at 09:34:07AM +0200, Patrick Steinhardt wrote: > > Note that the added test reliably reproduces the above bug on my machine > > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime > > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on > > specific allocation behaviour of glibc it is very likely that the test > > will not work on other platforms. > > What about forcing the address reuse in the test for deterministic > behavior ? > A test helper can close a pack and move a second packed_git, whose > delta base sits at the same offset, into its memory. > > This was written with the help of AI tools. > To apply on top of [PATCH 1/2]. You can do that, of course. But given the amount of code that we'd have to add to test for a very specific edge case it doesn't quite feel reasonable to me to add all this infrastructure. Patrick ``` ## Patrick Steinhardt, 2026-10-02 19:37 Subject: Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries Message-ID: In-Reply-To: <046C5954-DB91-4B7B-A89C-70B418CFDFB7@gmail.com> ``` On Fri, Oct 02, 2026 at 01:48:46PM -0400, Philippe Blain wrote: > Hi Patrick, > > > Le 2 oct. 2026 à 03:34, Patrick Steinhardt a écrit : > > > > The delta base cache is a process-global hashmap that is keyed by the > > address of the `struct packed_git` plus the offset of the base object > > within that pack. Entries part of the cache are never removed when a > > pack is closed, and neither when the pack is subsequently freed. As a > > consequence, the cache may contain stale entries. > > > > For a long time, the worst consequence of this leaking cache was that we > > held on to memory that we could've released. But the reason for this was > > that we didn't even free the packfiles, either. That has changed in > > 6f1e9394e2 (object: fix leaking packfiles when closing object store, > > 2024-08-08), where we plugged that leak. > > > > Now that we free them, a new packfile may be allocated using the exact > > same address as a previously allocated one. And if the new packfile has > > both the same address and a similar layout, it may happen that a > > preexisting entry from a previously-allocated in the delta base cache > > would have the exact same key. > > > > All of this sounds very theoretical, but we can actually trigger this > > bug somewhat reliably! When doing a merge with "--recurse-submodules" in > > a repository with lots of submodules that have similar-looking packfiles > > merge does not have a --recurse-submodules flag, submodules are merged > by default (but not updated after the merge, which would be what the > flag would do if it existed :)) Oh, right, will fix. > > When using glibc, one of the packfiles will eventually get the exact > > same address, and that will then cause Git to read the wrong entry from > > the cache. Git detects this and aborts with an error: > > > > $ git merge branch-b > > error: Could not read 584ef938be4a749bfa13f68d5ac5545bc029e529 > > error: could not parse commit 584ef938be4a749bfa13f68d5ac5545bc029e529 > > error: failed to merge submodule G (repository corrupt) > > > > Now in this case we're lucky that Git detects this error because we try > > to read a commit from a different submodule via an object database that > > doesn't have it. But potentially, in an even more contrived scenario, we > > might even silently yield wrong data from the cache. > > > > Fix this bug by evicting cache entries that belong to a specific pack > > when closing it. > > > > Note that the added test reliably reproduces the above bug on my machine > > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime > > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on > > specific allocation behaviour of glibc it is very likely that the test > > will not work on other platforms. > > > > Reported-by: Guillaume Chauvel > > Helped-by: Philippe Blain > > Signed-off-by: Patrick Steinhardt > > Thanks for the trailer and the quick fix !! > I’m still puzzled why it worked correctly on 2.56.0-rc1 on my WSL > instance. From your commit message, I guess for some reason I get > different adresses and so the bug does not trigger. Yeah, it strongly depends on the exact allocation sequence and on your environment. > I see you the test you add merges more than two submodules, in > contrast to Guillaume’s reproducer. Is that necessary for the bug to > trigger for you? Yes, I was not able to reproduce the bug with less submodules. The thing is that this also depends on the length of the path in which your tests run because of how glibc classifies, and probably on other factors, too. When running in "/tmp/" directly for example I require less submodules. Patrick ``` ## Patrick Steinhardt, 2026-10-02 19:38 Subject: Re: [PATCH 1/2] packfile: move around `close_pack()` Message-ID: In-Reply-To: ``` On Fri, Oct 02, 2026 at 03:02:30PM -0400, Mark C. Chu-Carroll wrote: > On Fri Oct 2, 2026 at 3:34 AM EDT, Patrick Steinhardt wrote: > > In the next commit we'll want to access the delta base cache in > > `close_pack()`. Move the function after the declaration of the cache to > > prepare for this. > > Maybe I'm just being clueless, but how does moving an unmodified function > help with the subsequent change? This is mostly done to avoid a forward declaration of the function that would otherwise be necessary. Patrick ``` ## Jeff King, 2026-10-02 22:23 Subject: Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries Message-ID: <20261002222335.GC833115@coredump.intra.peff.net> In-Reply-To: <20261002-pks-packfile-stale-delta-base-cache-v1-2-7592a3e31ae0@pks.im> ``` On Fri, Oct 02, 2026 at 09:34:07AM +0200, Patrick Steinhardt wrote: > Note that the added test reliably reproduces the above bug on my machine > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on > specific allocation behaviour of glibc it is very likely that the test > will not work on other platforms. At its core this is a user-after-free bug, isn't it? If so, I think it would be fine to say that ASan will reliably find it (and we don't even really need to demonstrate the complex case where the packed_git has the same address; all bets are off once we access the freed pointer). -Peff ``` ## Patrick Steinhardt, 2026-10-05 05:32 Subject: Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries Message-ID: In-Reply-To: <20261002222335.GC833115@coredump.intra.peff.net> ``` On Fri, Oct 02, 2026 at 06:23:35PM -0400, Jeff King wrote: > On Fri, Oct 02, 2026 at 09:34:07AM +0200, Patrick Steinhardt wrote: > > > Note that the added test reliably reproduces the above bug on my machine > > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime > > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on > > specific allocation behaviour of glibc it is very likely that the test > > will not work on other platforms. > > At its core this is a user-after-free bug, isn't it? If so, I think it > would be fine to say that ASan will reliably find it (and we don't even > really need to demonstrate the complex case where the packed_git has the > same address; all bets are off once we access the freed pointer). It doesn't though. The key of the cache is the address of the freed object, but the value is a still-live object: struct delta_base_cache_key { struct packed_git *p; off_t base_offset; }; struct delta_base_cache_entry { struct hashmap_entry ent; struct delta_base_cache_key key; struct list_head lru; void *data; size_t size; enum object_type type; }; We only use the value of `p`, but never dereference it. In fact, when I enable ASan I cannot reproduce the bug at all anymore because it will hand out unique addresses. Patrick ```