{"thread":{"id":"66443","subject":"[PATCH 0/2] packfile: fix corruption due to stale delta base cache entries","startedAt":"2026-10-02T07:34:26Z","lastAt":"2026-10-05T05:32:26Z","messageCount":11,"participants":["Patrick Steinhardt","Guillaume Chauvel","Philippe Blain","Mark C. Chu-Carroll","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"553902","messageId":"20261002-pks-packfile-stale-delta-base-cache-v1-0-7592a3e31ae0@pks.im","threadId":"66443","inReplyTo":null,"subject":"[PATCH 0/2] packfile: fix corruption due to stale delta base cache entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-02T07:34:05Z","receivedAt":"2026-10-02T07:34:26Z","isPatch":true,"body":"Hi,\n\nthis small patch series fixes the bug reported in [1].\n\nTo summarize: we never evict delta base cache entries when closing the\nowning pack. The cache may thus contain stale entries which are keyed by\nby the memory address of `struct packed_git` and the offset of the entry\nin the packfile. Now when allocating a new pack that happens to have the\nexact same address and that has entries sitting at the same offset, we\nmay try to use these stale entries and thus yield corrupted data.\n\nThis all sounds very unlikely, but the interesting part is that this can\nbe reproduced by using recursive merges with submodules, as we open and\nclose the object databases of each respective submodule. And if they\nhave similar packfiles, then we may trigger the bug.\n\nThe series is built on top of v2.56.0.\n\nThanks!\n\nPatrick\n\n[1]: <CAP4DsUexEmm1qo6jH+Qzy+n3dQs_OCJ8yg=ReF+aVrcTrC7NeQ@mail.gmail.com>\n\n---\nPatrick Steinhardt (2):\n      packfile: move around `close_pack()`\n      packfile: fix corruption due to stale delta base cache entries\n\n packfile.c                 | 33 ++++++++++++++++++++++----------\n t/t6437-submodule-merge.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 10 deletions(-)\n\n\n---\nbase-commit: a018953688f1b10bddf91bff8747068f5f4746a4\nchange-id: 20261002-pks-packfile-stale-delta-base-cache-0d4730487643\n\n"},{"id":"553903","messageId":"20261002-pks-packfile-stale-delta-base-cache-v1-1-7592a3e31ae0@pks.im","threadId":"66443","inReplyTo":"20261002-pks-packfile-stale-delta-base-cache-v1-0-7592a3e31ae0@pks.im","subject":"[PATCH 1/2] packfile: move around `close_pack()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-02T07:34:06Z","receivedAt":"2026-10-02T07:34:27Z","isPatch":true,"body":"In the next commit we'll want to access the delta base cache in\n`close_pack()`. Move the function after the declaration of the cache to\nprepare for this.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n packfile.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 4fa5fd67c8..af1b837974 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -355,16 +355,6 @@ static void close_pack_mtimes(struct packed_git *p)\n \tp->mtimes_map = NULL;\n }\n \n-void close_pack(struct packed_git *p)\n-{\n-\tclose_pack_windows(p);\n-\tclose_pack_fd(p);\n-\tclose_pack_index(p);\n-\tclose_pack_revindex(p);\n-\tclose_pack_mtimes(p);\n-\toidset_clear(&p->bad_objects);\n-}\n-\n void unlink_pack_path(const char *pack_name, int force_delete)\n {\n \tstatic const char *exts[] = {\".idx\", \".pack\", \".rev\", \".keep\", \".bitmap\", \".promisor\", \".mtimes\"};\n@@ -1263,6 +1253,16 @@ void clear_delta_base_cache(void)\n \t}\n }\n \n+void close_pack(struct packed_git *p)\n+{\n+\tclose_pack_windows(p);\n+\tclose_pack_fd(p);\n+\tclose_pack_index(p);\n+\tclose_pack_revindex(p);\n+\tclose_pack_mtimes(p);\n+\toidset_clear(&p->bad_objects);\n+}\n+\n static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n \t\t\t\t void *base, size_t base_size,\n \t\t\t\t size_t delta_base_cache_limit,\n\n-- \n2.56.0.353.g0856645cf6.dirty\n\n"},{"id":"553904","messageId":"20261002-pks-packfile-stale-delta-base-cache-v1-2-7592a3e31ae0@pks.im","threadId":"66443","inReplyTo":"20261002-pks-packfile-stale-delta-base-cache-v1-0-7592a3e31ae0@pks.im","subject":"[PATCH 2/2] packfile: fix corruption due to stale delta base cache entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-02T07:34:07Z","receivedAt":"2026-10-02T07:34:34Z","isPatch":true,"body":"The delta base cache is a process-global hashmap that is keyed by the\naddress of the `struct packed_git` plus the offset of the base object\nwithin that pack. Entries part of the cache are never removed when a\npack is closed, and neither when the pack is subsequently freed. As a\nconsequence, the cache may contain stale entries.\n\nFor a long time, the worst consequence of this leaking cache was that we\nheld on to memory that we could've released. But the reason for this was\nthat we didn't even free the packfiles, either. That has changed in\n6f1e9394e2 (object: fix leaking packfiles when closing object store,\n2024-08-08), where we plugged that leak.\n\nNow that we free them, a new packfile may be allocated using the exact\nsame address as a previously allocated one. And if the new packfile has\nboth the same address and a similar layout, it may happen that a\npreexisting entry from a previously-allocated in the delta base cache\nwould have the exact same key.\n\nAll of this sounds very theoretical, but we can actually trigger this\nbug somewhat reliably! When doing a merge with \"--recurse-submodules\" in\na repository with lots of submodules that have similar-looking packfiles\nwe end up opening and then closing the object databases of each of the\nsubmodules in sequence. Because of the above mentioned commit we would\nclose and free each of the packfiles part of the respective databases,\nbut we wouldn't evict thire delta base entries from the cache.\n\nWhen using glibc, one of the packfiles will eventually get the exact\nsame address, and that will then cause Git to read the wrong entry from\nthe cache. Git detects this and aborts with an error:\n\n    $ git merge branch-b\n    error: Could not read 584ef938be4a749bfa13f68d5ac5545bc029e529\n    error: could not parse commit 584ef938be4a749bfa13f68d5ac5545bc029e529\n    error: failed to merge submodule G (repository corrupt)\n\nNow in this case we're lucky that Git detects this error because we try\nto read a commit from a different submodule via an object database that\ndoesn't have it. But potentially, in an even more contrived scenario, we\nmight even silently yield wrong data from the cache.\n\nFix this bug by evicting cache entries that belong to a specific pack\nwhen closing it.\n\nNote that the added test reliably reproduces the above bug on my machine\nthat uses NixOS at c59305bab206 (cosmic-applets: add missing runtime\ndependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on\nspecific allocation behaviour of glibc it is very likely that the test\nwill not work on other platforms.\n\nReported-by: Guillaume Chauvel <guillaume.chauvel@gmail.com>\nHelped-by: Philippe Blain <levraiphilippeblain@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n packfile.c                 | 13 +++++++++++++\n t/t6437-submodule-merge.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 60 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex af1b837974..365c54c7dc 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1253,6 +1253,18 @@ void clear_delta_base_cache(void)\n \t}\n }\n \n+static void delta_base_cache_evict_entry(struct packed_git *p)\n+{\n+\tstruct list_head *lru, *tmp;\n+\n+\tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n+\t\tstruct delta_base_cache_entry *entry =\n+\t\t\tlist_entry(lru, struct delta_base_cache_entry, lru);\n+\t\tif (entry->key.p == p)\n+\t\t\trelease_delta_base_cache(entry);\n+\t}\n+}\n+\n void close_pack(struct packed_git *p)\n {\n \tclose_pack_windows(p);\n@@ -1261,6 +1273,7 @@ void close_pack(struct packed_git *p)\n \tclose_pack_revindex(p);\n \tclose_pack_mtimes(p);\n \toidset_clear(&p->bad_objects);\n+\tdelta_base_cache_evict_entry(p);\n }\n \n static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\ndiff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh\nindex 1546d5f773..0ee3684206 100755\n--- a/t/t6437-submodule-merge.sh\n+++ b/t/t6437-submodule-merge.sh\n@@ -514,4 +514,51 @@ test_expect_success 'merging should fail with no merge base' '\n \t)\n '\n \n+test_expect_success 'merge with many packed submodules reports conflicts' '\n+\ttest_config_global protocol.file.allow always &&\n+\n+\t# Create 16 submodules with two divergent branches each.\n+\tsubmodules=\"A B C D E F G H I J K L M N O P\" &&\n+\tfor name in $submodules\n+\tdo\n+\t\tgit init source-$name &&\n+\t\ttest_commit -C source-$name $name-main &&\n+\t\tgit -C source-$name switch --create branch-a main &&\n+\t\tgit -C source-$name commit --allow-empty --message $name-branch-a &&\n+\t\tgit -C source-$name switch --create branch-b main &&\n+\t\tgit -C source-$name commit --allow-empty --message $name-branch-b || return 1\n+\tdone &&\n+\n+\t# Create the superproject and add all submodules.\n+\tgit init many-packed &&\n+\tfor name in $submodules\n+\tdo\n+\t\tgit -C many-packed submodule add --branch main \"file://$PWD/source-$name\" $name || return 1\n+\tdone &&\n+\tgit -C many-packed commit --message main &&\n+\n+\t# Create two divergent commits in the superproject that update all\n+\t# submodules to the divergent branches.\n+\tfor branch in branch-a branch-b\n+\tdo\n+\t\tgit -C many-packed switch -c $branch main &&\n+\t\tfor name in $submodules\n+\t\tdo\n+\t\t\tgit -C many-packed/$name switch $branch || return 1\n+\t\tdone &&\n+\t\tgit -C many-packed add $submodules &&\n+\t\tgit -C many-packed commit --message $branch || return 1\n+\tdone &&\n+\n+\t# Clone the superproject to ensure that everything is well-packed and\n+\t# then merge the two branches, creating conflicts for every submodule.\n+\tgit clone many-packed many-packed-clone &&\n+\tgit -C many-packed-clone submodule update --init &&\n+\tgit -C many-packed-clone switch branch-a &&\n+\ttest_expect_code 1 git -C many-packed-clone -c advice.submoduleMergeConflict=false merge branch-b >out 2>err &&\n+\tgrep \"^CONFLICT (submodule)\" out >conflicts &&\n+\ttest_line_count = 16 conflicts &&\n+\ttest_must_be_empty err\n+'\n+\n test_done\n\n-- \n2.56.0.353.g0856645cf6.dirty\n\n"},{"id":"553965","messageId":"20261002133405.1284-1-guillaume.chauvel@gmail.com","threadId":"66443","inReplyTo":"20261002-pks-packfile-stale-delta-base-cache-v1-2-7592a3e31ae0@pks.im","subject":"Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries","fromName":"Guillaume Chauvel","fromEmail":"guillaume.chauvel@gmail.com","sentAt":"2026-10-02T13:34:05Z","receivedAt":"2026-10-02T13:34:34Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 09:34:07AM +0200, Patrick Steinhardt wrote:\n> Note that the added test reliably reproduces the above bug on my machine\n> that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime\n> dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on\n> specific allocation behaviour of glibc it is very likely that the test\n> will not work on other platforms.\n\nWhat about forcing the address reuse in the test for deterministic\nbehavior ?\nA test helper can close a pack and move a second packed_git, whose\ndelta base sits at the same offset, into its memory.\n\nThis was written with the help of AI tools.\nTo apply on top of [PATCH 1/2].\n\n-- >8 --\n Makefile                         |  1 +\n packfile.c                       | 13 ++++++\n t/helper/meson.build             |  1 +\n t/helper/test-delta-base-cache.c | 92 ++++++++++++++++++++++++++++++++++++++++\n t/helper/test-pack-deltas.c      | 22 +++++++++-\n t/helper/test-tool.c             |  1 +\n t/helper/test-tool.h             |  1 +\n t/meson.build                    |  1 +\n t/t5336-pack-delta-base-cache.sh | 46 ++++++++++++++++++++\n 9 files changed, 177 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-delta-base-cache.c\n create mode 100755 t/t5336-pack-delta-base-cache.sh\n\ndiff --git a/Makefile b/Makefile\nindex c649c93c51..771ca00e33 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -818,6 +818,7 @@ TEST_BUILTINS_OBJS += test-crontab.o\n TEST_BUILTINS_OBJS += test-csprng.o\n TEST_BUILTINS_OBJS += test-date.o\n TEST_BUILTINS_OBJS += test-delete-gpgsig.o\n+TEST_BUILTINS_OBJS += test-delta-base-cache.o\n TEST_BUILTINS_OBJS += test-delta.o\n TEST_BUILTINS_OBJS += test-dir-iterator.o\n TEST_BUILTINS_OBJS += test-drop-caches.o\ndiff --git a/packfile.c b/packfile.c\nindex af1b837974..365c54c7dc 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1253,6 +1253,18 @@ void clear_delta_base_cache(void)\n \t}\n }\n \n+static void delta_base_cache_evict_entry(struct packed_git *p)\n+{\n+\tstruct list_head *lru, *tmp;\n+\n+\tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n+\t\tstruct delta_base_cache_entry *entry =\n+\t\t\tlist_entry(lru, struct delta_base_cache_entry, lru);\n+\t\tif (entry->key.p == p)\n+\t\t\trelease_delta_base_cache(entry);\n+\t}\n+}\n+\n void close_pack(struct packed_git *p)\n {\n \tclose_pack_windows(p);\n@@ -1261,6 +1273,7 @@ void close_pack(struct packed_git *p)\n \tclose_pack_revindex(p);\n \tclose_pack_mtimes(p);\n \toidset_clear(&p->bad_objects);\n+\tdelta_base_cache_evict_entry(p);\n }\n \n static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\ndiff --git a/t/helper/meson.build b/t/helper/meson.build\nindex 3235f10ab8..225c46a88c 100644\n--- a/t/helper/meson.build\n+++ b/t/helper/meson.build\n@@ -11,6 +11,7 @@ test_tool_sources = [\n   'test-csprng.c',\n   'test-date.c',\n   'test-delete-gpgsig.c',\n+  'test-delta-base-cache.c',\n   'test-delta.c',\n   'test-dir-iterator.c',\n   'test-drop-caches.c',\ndiff --git a/t/helper/test-delta-base-cache.c b/t/helper/test-delta-base-cache.c\nnew file mode 100644\nindex 0000000000..54a0d43f2e\n--- /dev/null\n+++ b/t/helper/test-delta-base-cache.c\n@@ -0,0 +1,92 @@\n+#define USE_THE_REPOSITORY_VARIABLE\n+\n+#include \"test-tool.h\"\n+#include \"hex.h\"\n+#include \"object-file.h\"\n+#include \"packfile.h\"\n+#include \"setup.h\"\n+\n+static off_t delta_base_offset(struct packed_git *pack,\n+\t\t\t       const struct object_id *oid)\n+{\n+\tstruct pack_window *window = NULL;\n+\toff_t offset, pos, base;\n+\tsize_t size;\n+\tint type;\n+\n+\toffset = find_pack_entry_one(oid, pack);\n+\tif (!offset)\n+\t\tdie(\"object is missing from pack: %s\", oid_to_hex(oid));\n+\tpos = offset;\n+\ttype = unpack_object_header(pack, &window, &pos, &size);\n+\tif (type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA)\n+\t\tdie(\"object is not stored as a delta: %s\", oid_to_hex(oid));\n+\tbase = get_delta_base(pack, &window, &pos, type, offset);\n+\tunuse_pack(&window);\n+\tif (!base)\n+\t\tdie(\"cannot locate delta base for %s\", oid_to_hex(oid));\n+\treturn base;\n+}\n+\n+int cmd__delta_base_cache(int argc, const char **argv)\n+{\n+\tstruct packed_git *first, *second;\n+\tstruct object_id first_oid, second_oid, actual_oid;\n+\tenum object_type type;\n+\tsize_t size;\n+\tvoid *data;\n+\n+\tif (argc != 5)\n+\t\tusage(\"test-tool delta-base-cache <first.idx> <first-delta> \"\n+\t\t      \"<second.idx> <second-delta>\");\n+\n+\tsetup_git_directory(the_repository);\n+\n+\tif (get_oid_hex(argv[2], &first_oid) || get_oid_hex(argv[4], &second_oid))\n+\t\tdie(\"invalid object ID\");\n+\tif (strlen(argv[1]) != strlen(argv[3]))\n+\t\tdie(\"pack index paths must have the same length\");\n+\n+\tfirst = add_packed_git(the_repository, argv[1], strlen(argv[1]), 1);\n+\tsecond = add_packed_git(the_repository, argv[3], strlen(argv[3]), 1);\n+\tif (!first || !second || open_pack_index(first) || open_pack_index(second))\n+\t\tdie(\"cannot open pack indexes\");\n+\n+\tif (delta_base_offset(first, &first_oid) !=\n+\t    delta_base_offset(second, &second_oid))\n+\t\tdie(\"delta bases have different pack offsets\");\n+\n+\tdata = unpack_entry(the_repository, first,\n+\t\t\t    find_pack_entry_one(&first_oid, first), NULL, NULL);\n+\tif (!data)\n+\t\tdie(\"cannot unpack first object\");\n+\tfree(data);\n+\n+\tclose_pack(first);\n+\n+\t/*\n+\t * Simulate the allocator handing the address of the closed pack to\n+\t * a new one. The resources of \"second\" now belong to \"first\".\n+\t */\n+\tmemcpy(first, second, sizeof(*first) + strlen(second->pack_name) + 1);\n+\tfree(second);\n+\n+\tdata = unpack_entry(the_repository, first,\n+\t\t\t    find_pack_entry_one(&second_oid, first), &type, &size);\n+\tif (!data)\n+\t\tdie(\"cannot unpack second object\");\n+\thash_object_file(the_repository->hash_algo, data, size, type, &actual_oid);\n+\tfree(data);\n+\tclose_pack(first);\n+\tfree(first);\n+\n+\t/*\n+\t * The test checked that applying the second object's delta to the\n+\t * first pack's base does not give the second object, so a stale\n+\t * cache entry shows up as an object ID mismatch.\n+\t */\n+\tif (!oideq(&actual_oid, &second_oid))\n+\t\treturn error(\"second object differs after pack reuse\");\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-pack-deltas.c b/t/helper/test-pack-deltas.c\nindex 959705feca..6cd5a7a7d8 100644\n--- a/t/helper/test-pack-deltas.c\n+++ b/t/helper/test-pack-deltas.c\n@@ -43,6 +43,26 @@ static unsigned long do_compress(void **pptr, unsigned long size)\n \treturn stream.total_out;\n }\n \n+static void write_full(struct hashfile *f, struct object_id *oid)\n+{\n+\tunsigned char header[MAX_PACK_OBJECT_HEADER];\n+\tunsigned long compressed_size, hdrlen;\n+\tsize_t size;\n+\tenum object_type type;\n+\tvoid *buf = odb_read_object(the_repository->objects,\n+\t\t\t\t    oid, &type, &size);\n+\n+\tif (!buf)\n+\t\tdie(\"unable to read %s\", oid_to_hex(oid));\n+\n+\tcompressed_size = do_compress(&buf, cast_size_t_to_ulong(size));\n+\thdrlen = encode_in_pack_object_header(header, sizeof(header),\n+\t\t\t\t\t      type, size);\n+\thashwrite(f, header, hdrlen);\n+\thashwrite(f, buf, compressed_size);\n+\tfree(buf);\n+}\n+\n static void write_ref_delta(struct hashfile *f,\n \t\t\t    struct object_id *oid,\n \t\t\t    struct object_id *base)\n@@ -136,7 +156,7 @@ int cmd__pack_deltas(int argc, const char **argv)\n \t\telse if (!strcmp(type_str, \"OFS_DELTA\"))\n \t\t\tdie(\"OFS_DELTA not implemented\");\n \t\telse if (!strcmp(type_str, \"FULL\"))\n-\t\t\tdie(\"FULL not implemented\");\n+\t\t\twrite_full(f, &content_oid);\n \t\telse\n \t\t\tdie(\"unknown pack type: %s\", type_str);\n \t}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex b71a22b43b..83b2e99344 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -22,6 +22,7 @@ static struct test_cmd cmds[] = {\n \t{ \"date\", cmd__date },\n \t{ \"delete-gpgsig\", cmd__delete_gpgsig },\n \t{ \"delta\", cmd__delta },\n+\t{ \"delta-base-cache\", cmd__delta_base_cache },\n \t{ \"dir-iterator\", cmd__dir_iterator },\n \t{ \"drop-caches\", cmd__drop_caches },\n \t{ \"dump-cache-tree\", cmd__dump_cache_tree },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex f2885b33d5..8c95a1dada 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -14,6 +14,7 @@ int cmd__crontab(int argc, const char **argv);\n int cmd__csprng(int argc, const char **argv);\n int cmd__date(int argc, const char **argv);\n int cmd__delta(int argc, const char **argv);\n+int cmd__delta_base_cache(int argc, const char **argv);\n int cmd__delete_gpgsig(int argc, const char **argv);\n int cmd__dir_iterator(int argc, const char **argv);\n int cmd__drop_caches(int argc, const char **argv);\ndiff --git a/t/meson.build b/t/meson.build\nindex 3ca7b27104..7d01040a07 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -639,6 +639,7 @@ integration_tests = [\n   't5333-pseudo-merge-bitmaps.sh',\n   't5334-incremental-multi-pack-index.sh',\n   't5335-compact-multi-pack-index.sh',\n+  't5336-pack-delta-base-cache.sh',\n   't5351-unpack-large-objects.sh',\n   't5400-send-pack.sh',\n   't5401-update-hooks.sh',\ndiff --git a/t/t5336-pack-delta-base-cache.sh b/t/t5336-pack-delta-base-cache.sh\nnew file mode 100755\nindex 0000000000..2abc12559a\n--- /dev/null\n+++ b/t/t5336-pack-delta-base-cache.sh\n@@ -0,0 +1,46 @@\n+#!/bin/sh\n+\n+test_description='delta base cache lifetime across pack closure'\n+\n+. ./test-lib.sh\n+\n+# The delta base cache is keyed by (packed_git pointer, base offset).\n+# Reading B from A-B.pack caches A.\n+# The helper then closes that pack and, since B has the same offset\n+# as A, reuses its packed_git structure for B-C.pack to reproduce\n+# the same cache key. If closing the pack leaves A in the cache,\n+# reading C would use A instead of B as its delta base, which the\n+# helper detects by checking the resulting object ID.\n+test_expect_success 'delta base cache entries do not outlive their pack' '\n+\ttest-tool genrandom cache-data 1024 >common &&\n+\t{ printf \"a0\" && cat common; } >a &&\n+\t{ printf \"b1\" && cat common; } >b &&\n+\t{ printf \"c1\" && cat common; } >c &&\n+\tA=$(git hash-object -w a) &&\n+\tB=$(git hash-object -w b) &&\n+\tC=$(git hash-object -w c) &&\n+\n+\t# Applying the B-to-C delta to A must not give C. Otherwise the\n+\t# helper could not detect a stale cache entry from the object ID.\n+\ttest-tool delta -d b c b-c.delta &&\n+\ttest-tool delta -p a b-c.delta stale &&\n+\t! cmp -s c stale &&\n+\n+\ttest-tool pack-deltas --num-objects=2 >A-B.pack <<-EOF &&\n+\tFULL $A\n+\tREF_DELTA $B $A\n+\tEOF\n+\ttest-tool pack-deltas --num-objects=2 >B-C.pack <<-EOF &&\n+\tFULL $B\n+\tREF_DELTA $C $B\n+\tEOF\n+\n+\tgit index-pack -o A-B.idx A-B.pack &&\n+\tgit index-pack -o B-C.idx B-C.pack &&\n+\n+\ttest-tool delta-base-cache \\\n+\t\tA-B.idx $B \\\n+\t\tB-C.idx $C\n+'\n+\n+test_done\n"},{"id":"553992","messageId":"046C5954-DB91-4B7B-A89C-70B418CFDFB7@gmail.com","threadId":"66443","inReplyTo":"20261002-pks-packfile-stale-delta-base-cache-v1-2-7592a3e31ae0@pks.im","subject":"Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2026-10-02T17:48:46Z","receivedAt":"2026-10-02T17:49:04Z","isPatch":true,"body":"Hi Patrick, \n\n> Le 2 oct. 2026 à 03:34, Patrick Steinhardt <ps@pks.im> a écrit :\n> \n> The delta base cache is a process-global hashmap that is keyed by the\n> address of the `struct packed_git` plus the offset of the base object\n> within that pack. Entries part of the cache are never removed when a\n> pack is closed, and neither when the pack is subsequently freed. As a\n> consequence, the cache may contain stale entries.\n> \n> For a long time, the worst consequence of this leaking cache was that we\n> held on to memory that we could've released. But the reason for this was\n> that we didn't even free the packfiles, either. That has changed in\n> 6f1e9394e2 (object: fix leaking packfiles when closing object store,\n> 2024-08-08), where we plugged that leak.\n> \n> Now that we free them, a new packfile may be allocated using the exact\n> same address as a previously allocated one. And if the new packfile has\n> both the same address and a similar layout, it may happen that a\n> preexisting entry from a previously-allocated in the delta base cache\n> would have the exact same key.\n> \n> All of this sounds very theoretical, but we can actually trigger this\n> bug somewhat reliably! When doing a merge with \"--recurse-submodules\" in\n> a repository with lots of submodules that have similar-looking packfiles\n\nmerge 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 :))\n\n> we end up opening and then closing the object databases of each of the\n> submodules in sequence. Because of the above mentioned commit we would\n> close and free each of the packfiles part of the respective databases,\n> but we wouldn't evict thire delta base entries from the cache.\n\ns/thire/their\n\n> \n> When using glibc, one of the packfiles will eventually get the exact\n> same address, and that will then cause Git to read the wrong entry from\n> the cache. Git detects this and aborts with an error:\n> \n>    $ git merge branch-b\n>    error: Could not read 584ef938be4a749bfa13f68d5ac5545bc029e529\n>    error: could not parse commit 584ef938be4a749bfa13f68d5ac5545bc029e529\n>    error: failed to merge submodule G (repository corrupt)\n> \n> Now in this case we're lucky that Git detects this error because we try\n> to read a commit from a different submodule via an object database that\n> doesn't have it. But potentially, in an even more contrived scenario, we\n> might even silently yield wrong data from the cache.\n> \n> Fix this bug by evicting cache entries that belong to a specific pack\n> when closing it.\n> \n> Note that the added test reliably reproduces the above bug on my machine\n> that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime\n> dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on\n> specific allocation behaviour of glibc it is very likely that the test\n> will not work on other platforms.\n> \n> Reported-by: Guillaume Chauvel <guillaume.chauvel@gmail.com>\n> Helped-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n\nThanks for the trailer and the quick fix !!\nI’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. \n\nI 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?\n\nCheers, \n\nPhilippe. "},{"id":"554001","messageId":"DLUL2YDALTAV.15LO4AB7BDMLR@fastmail.com","threadId":"66443","inReplyTo":"20261002-pks-packfile-stale-delta-base-cache-v1-1-7592a3e31ae0@pks.im","subject":"Re: [PATCH 1/2] packfile: move around `close_pack()`","fromName":"Mark C. Chu-Carroll","fromEmail":"markchucarroll@fastmail.com","sentAt":"2026-10-02T19:02:30Z","receivedAt":"2026-10-02T19:02:33Z","isPatch":true,"body":"On Fri Oct 2, 2026 at 3:34 AM EDT, Patrick Steinhardt wrote:\n> In the next commit we'll want to access the delta base cache in\n> `close_pack()`. Move the function after the declaration of the cache to\n> prepare for this.\n\nMaybe I'm just being clueless, but how does moving an unmodified function\nhelp with the subsequent change?\n\n-- \nMark Craig Chu-Carroll (@MarkChuCarroll at gitlab)\n*** Software Tools/Math Geek - Software Engineer at Gitlab\n*** Work Email: mcarroll@gitlab.com / markchucarroll@fastmail.com\n*** Personal Blog: http://goodmath.org/blog / Personal email: markcc@gmail.com\n\n"},{"id":"554006","messageId":"asAHLJvsz0gpSB7j@pks.im","threadId":"66443","inReplyTo":"20261002133405.1284-1-guillaume.chauvel@gmail.com","subject":"Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-02T19:34:04Z","receivedAt":"2026-10-02T19:34:11Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 03:34:05PM +0200, Guillaume Chauvel wrote:\n> On Fri, Oct 02, 2026 at 09:34:07AM +0200, Patrick Steinhardt wrote:\n> > Note that the added test reliably reproduces the above bug on my machine\n> > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime\n> > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on\n> > specific allocation behaviour of glibc it is very likely that the test\n> > will not work on other platforms.\n> \n> What about forcing the address reuse in the test for deterministic\n> behavior ?\n> A test helper can close a pack and move a second packed_git, whose\n> delta base sits at the same offset, into its memory.\n> \n> This was written with the help of AI tools.\n> To apply on top of [PATCH 1/2].\n\nYou can do that, of course. But given the amount of code that we'd have\nto add to test for a very specific edge case it doesn't quite feel\nreasonable to me to add all this infrastructure.\n\nPatrick\n"},{"id":"554007","messageId":"asAH9ma00tb4r-ks@pks.im","threadId":"66443","inReplyTo":"046C5954-DB91-4B7B-A89C-70B418CFDFB7@gmail.com","subject":"Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-02T19:37:26Z","receivedAt":"2026-10-02T19:37:33Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 01:48:46PM -0400, Philippe Blain wrote:\n> Hi Patrick, \n> \n> > Le 2 oct. 2026 à 03:34, Patrick Steinhardt <ps@pks.im> a écrit :\n> > \n> > The delta base cache is a process-global hashmap that is keyed by the\n> > address of the `struct packed_git` plus the offset of the base object\n> > within that pack. Entries part of the cache are never removed when a\n> > pack is closed, and neither when the pack is subsequently freed. As a\n> > consequence, the cache may contain stale entries.\n> > \n> > For a long time, the worst consequence of this leaking cache was that we\n> > held on to memory that we could've released. But the reason for this was\n> > that we didn't even free the packfiles, either. That has changed in\n> > 6f1e9394e2 (object: fix leaking packfiles when closing object store,\n> > 2024-08-08), where we plugged that leak.\n> > \n> > Now that we free them, a new packfile may be allocated using the exact\n> > same address as a previously allocated one. And if the new packfile has\n> > both the same address and a similar layout, it may happen that a\n> > preexisting entry from a previously-allocated in the delta base cache\n> > would have the exact same key.\n> > \n> > All of this sounds very theoretical, but we can actually trigger this\n> > bug somewhat reliably! When doing a merge with \"--recurse-submodules\" in\n> > a repository with lots of submodules that have similar-looking packfiles\n> \n> merge does not have a --recurse-submodules flag, submodules are merged\n> by default (but not updated after the merge, which would be what the\n> flag would do if it existed :))\n\nOh, right, will fix.\n\n> > When using glibc, one of the packfiles will eventually get the exact\n> > same address, and that will then cause Git to read the wrong entry from\n> > the cache. Git detects this and aborts with an error:\n> > \n> >    $ git merge branch-b\n> >    error: Could not read 584ef938be4a749bfa13f68d5ac5545bc029e529\n> >    error: could not parse commit 584ef938be4a749bfa13f68d5ac5545bc029e529\n> >    error: failed to merge submodule G (repository corrupt)\n> > \n> > Now in this case we're lucky that Git detects this error because we try\n> > to read a commit from a different submodule via an object database that\n> > doesn't have it. But potentially, in an even more contrived scenario, we\n> > might even silently yield wrong data from the cache.\n> > \n> > Fix this bug by evicting cache entries that belong to a specific pack\n> > when closing it.\n> > \n> > Note that the added test reliably reproduces the above bug on my machine\n> > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime\n> > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on\n> > specific allocation behaviour of glibc it is very likely that the test\n> > will not work on other platforms.\n> > \n> > Reported-by: Guillaume Chauvel <guillaume.chauvel@gmail.com>\n> > Helped-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> \n> Thanks for the trailer and the quick fix !!\n> I’m still puzzled why it worked correctly on 2.56.0-rc1 on my WSL\n> instance. From your commit message, I guess for some reason I get\n> different adresses and so the bug does not trigger. \n\nYeah, it strongly depends on the exact allocation sequence and on your\nenvironment.\n\n> I see you the test you add merges more than two submodules, in\n> contrast to Guillaume’s reproducer. Is that necessary for the bug to\n> trigger for you?\n\nYes, I was not able to reproduce the bug with less submodules. The thing\nis that this also depends on the length of the path in which your tests\nrun because of how glibc classifies, and probably on other factors, too.\nWhen running in \"/tmp/\" directly for example I require less submodules.\n\nPatrick\n"},{"id":"554009","messageId":"asAIH5JOfGMpJqEN@pks.im","threadId":"66443","inReplyTo":"DLUL2YDALTAV.15LO4AB7BDMLR@fastmail.com","subject":"Re: [PATCH 1/2] packfile: move around `close_pack()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-02T19:38:07Z","receivedAt":"2026-10-02T19:38:12Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 03:02:30PM -0400, Mark C. Chu-Carroll wrote:\n> On Fri Oct 2, 2026 at 3:34 AM EDT, Patrick Steinhardt wrote:\n> > In the next commit we'll want to access the delta base cache in\n> > `close_pack()`. Move the function after the declaration of the cache to\n> > prepare for this.\n> \n> Maybe I'm just being clueless, but how does moving an unmodified function\n> help with the subsequent change?\n\nThis is mostly done to avoid a forward declaration of the function that\nwould otherwise be necessary.\n\nPatrick\n"},{"id":"554018","messageId":"20261002222335.GC833115@coredump.intra.peff.net","threadId":"66443","inReplyTo":"20261002-pks-packfile-stale-delta-base-cache-v1-2-7592a3e31ae0@pks.im","subject":"Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-02T22:23:35Z","receivedAt":"2026-10-02T22:23:37Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 09:34:07AM +0200, Patrick Steinhardt wrote:\n\n> Note that the added test reliably reproduces the above bug on my machine\n> that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime\n> dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on\n> specific allocation behaviour of glibc it is very likely that the test\n> will not work on other platforms.\n\nAt its core this is a user-after-free bug, isn't it? If so, I think it\nwould be fine to say that ASan will reliably find it (and we don't even\nreally need to demonstrate the complex case where the packed_git has the\nsame address; all bets are off once we access the freed pointer).\n\n-Peff\n"},{"id":"554141","messageId":"asM2YoImN8bHLHj8@pks.im","threadId":"66443","inReplyTo":"20261002222335.GC833115@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-05T05:32:18Z","receivedAt":"2026-10-05T05:32:26Z","isPatch":true,"body":"On Fri, Oct 02, 2026 at 06:23:35PM -0400, Jeff King wrote:\n> On Fri, Oct 02, 2026 at 09:34:07AM +0200, Patrick Steinhardt wrote:\n> \n> > Note that the added test reliably reproduces the above bug on my machine\n> > that uses NixOS at c59305bab206 (cosmic-applets: add missing runtime\n> > dependency (#566040), 2026-10-01) with glibc 2.44-25. But as we rely on\n> > specific allocation behaviour of glibc it is very likely that the test\n> > will not work on other platforms.\n> \n> At its core this is a user-after-free bug, isn't it? If so, I think it\n> would be fine to say that ASan will reliably find it (and we don't even\n> really need to demonstrate the complex case where the packed_git has the\n> same address; all bets are off once we access the freed pointer).\n\nIt doesn't though. The key of the cache is the address of the freed\nobject, but the value is a still-live object:\n\n\tstruct delta_base_cache_key {\n\t\tstruct packed_git *p;\n\t\toff_t base_offset;\n\t};\n\t\n\tstruct delta_base_cache_entry {\n\t\tstruct hashmap_entry ent;\n\t\tstruct delta_base_cache_key key;\n\t\tstruct list_head lru;\n\t\tvoid *data;\n\t\tsize_t size;\n\t\tenum object_type type;\n\t};\n\nWe only use the value of `p`, but never dereference it. In fact, when\nI enable ASan I cannot reproduce the bug at all anymore because it will\nhand out unique addresses.\n\nPatrick\n"}]}