[PATCH 0/2] packfile: fix corruption due to stale delta base cache entries

[PATCH 0/2] packfile: fix corruption due to stale delta base cache entries

[Patrick Steinhardt]

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]: <CAP4DsUexEmm1qo6jH+Qzy+n3dQs_OCJ8yg=ReF+aVrcTrC7NeQ@mail.gmail.com>

---
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

[PATCH 1/2] packfile: move around `close_pack()`

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

[PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Patrick Steinhardt]

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 <guillaume.chauvel@gmail.com>
Helped-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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

Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Guillaume Chauvel]

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 <first.idx> <first-delta> "
+		      "<second.idx> <second-delta>");
+
+	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

Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Philippe Blain]

Hi Patrick, 

> Le 2 oct. 2026 à 03:34, Patrick Steinhardt <ps@pks.im> 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 <guillaume.chauvel@gmail.com>
> Helped-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

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. 

Re: [PATCH 1/2] packfile: move around `close_pack()`

[Mark C. Chu-Carroll]

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

Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Patrick Steinhardt]

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

Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Patrick Steinhardt]

On Fri, Oct 02, 2026 at 01:48:46PM -0400, Philippe Blain wrote:
> Hi Patrick, 
> 
> > Le 2 oct. 2026 à 03:34, Patrick Steinhardt <ps@pks.im> 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 <guillaume.chauvel@gmail.com>
> > Helped-by: Philippe Blain <levraiphilippeblain@gmail.com>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> 
> 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

Re: [PATCH 1/2] packfile: move around `close_pack()`

[Patrick Steinhardt]

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

Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Jeff King]

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

Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Patrick Steinhardt]

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

[PATCH v2 0/2] packfile: fix corruption due to stale delta base cache entries

[Patrick Steinhardt]

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.

Changes in v2:
  - Commit message improvements.
  - Link to v1: https://patch.msgid.link/20261002-pks-packfile-stale-delta-base-cache-v1-0-7592a3e31ae0@pks.im

Thanks!

Patrick

[1]: <CAP4DsUexEmm1qo6jH+Qzy+n3dQs_OCJ8yg=ReF+aVrcTrC7NeQ@mail.gmail.com>

---
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(-)

Range-diff versus v1:

1:  e7c340df39 ! 1:  a8af6a79c7 packfile: move around `close_pack()`
    @@ Commit message
         packfile: move around `close_pack()`
     
         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.
    +    `close_pack()`. Move the function after the declaration of the cache so
    +    that we won't need a forward declaration.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
2:  25efe15034 ! 2:  d6d45b5bf9 packfile: fix corruption due to stale delta base cache entries
    @@ Commit message
         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.
    +    bug somewhat reliably! When doing a merge 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
    +    their 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

---
base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
change-id: 20261002-pks-packfile-stale-delta-base-cache-0d4730487643

[PATCH v2 1/2] packfile: move around `close_pack()`

[Patrick Steinhardt]

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 so
that we won't need a forward declaration.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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.406.ga2d225a756.dirty

[PATCH v2 2/2] packfile: fix corruption due to stale delta base cache entries

[Patrick Steinhardt]

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 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
their 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 <guillaume.chauvel@gmail.com>
Helped-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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.406.ga2d225a756.dirty

Re: [PATCH v2 2/2] packfile: fix corruption due to stale delta base cache entries

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> 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.

In other words, the test will not detect the bug, when the fix is
reverted, unless the glibc allocator is used?

Adding an unreliable reproducer for a bug that is already fixed may
be of dubious value.  However, even if the test is unreliable (since
other allocators might hide the bug when the fix is reverted), it
may be OK as long as it catches the bug on widely used
configurations and does not trigger false positives.  On the other
hand, the earlier suggestion to write custom low-level code to
simulate a colliding allocation address somehow smells like a
maintenance burden to me.

Thanks.

Re: [PATCH v2 2/2] packfile: fix corruption due to stale delta base cache entries

[Patrick Steinhardt]

On Tue, Oct 06, 2026 at 12:57:41PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.
> 
> In other words, the test will not detect the bug, when the fix is
> reverted, unless the glibc allocator is used?

It is specific to memory allocation patterns and thus to the platform,
yes. But I just confirmed that the test also fails on for example Alpine
Linux, so it even reproduces with musl libc. I haven't tested any other
platforms though.

> Adding an unreliable reproducer for a bug that is already fixed may be
> of dubious value.  However, even if the test is unreliable (since
> other allocators might hide the bug when the fix is reverted), it may
> be OK as long as it catches the bug on widely used configurations and
> does not trigger false positives.

Yeah, it at least catches the bug on some systems. And I think even if
it eventually didn't anymore, it exercises a part of our system (doing
submodule merges across many submodules) that wasn't previously
exercised, I think. So it would still have some value there.

> On the other hand, the earlier suggestion to write custom low-level
> code to simulate a colliding allocation address somehow smells like a
> maintenance burden to me.

Agreed. It simply is too much boilerplate for too specific a failure, if
you ask me.

Patrick

Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Jeff King]

On Mon, Oct 05, 2026 at 07:32:18AM +0200, Patrick Steinhardt wrote:

> > 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).
> [...]
> 
> 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.

Ah, I get it now. It is a little funny to key the hash on the in-core
pointer we happen to have, but it does provide a certain uniqueness. I
suspect that doing this would be mostly correct:

diff --git a/packfile.c b/packfile.c
index 7cb9ff5ffb..f69e2535fc 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1192,7 +1192,7 @@ get_delta_base_cache_entry(struct packed_git *p, off_t base_offset)
 static int delta_base_cache_key_eq(const struct delta_base_cache_key *a,
 				   const struct delta_base_cache_key *b)
 {
-	return a->p == b->p && a->base_offset == b->base_offset;
+	return !strcmp(a->p->name, b->p->name) && a->base_offset == b->base_offset;
 }
 
 static int delta_base_cache_hash_cmp(const void *cmp_data UNUSED,

and would trigger the use-after-free, but:

  1. It introduces weird semantic questions, like: what if you freed and
     then reopened a pack of the same name and it didn't have the same
     contents?

  2. It's more expensive.

  3. Changing the bug from "hard to detect hash equality mismatch" to
     "undefined behavior" is not really much of an improvement. ;)

So I think just fixing the bug is good, along with accepting that it
only triggered in certain specific cases and testing that. And your
patch looks like the obviously correct fix.

-Peff

Re: [PATCH 2/2] packfile: fix corruption due to stale delta base cache entries

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ah, I get it now. It is a little funny to key the hash on the in-core
> pointer we happen to have, but it does provide a certain uniqueness. I
> suspect that doing this would be mostly correct:
> ...
> and would trigger the use-after-free, but:
>
>   1. It introduces weird semantic questions, like: what if you freed and
>      then reopened a pack of the same name and it didn't have the same
>      contents?
>
>   2. It's more expensive.
>
>   3. Changing the bug from "hard to detect hash equality mismatch" to
>      "undefined behavior" is not really much of an improvement. ;)
>
> So I think just fixing the bug is good, along with accepting that it
> only triggered in certain specific cases and testing that. And your
> patch looks like the obviously correct fix.

Thanks for writing and reviewing, all.  Very much appreciated.

Let me mark the topic for 'next'.