git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
GCGuillaume Chauvel <guillaume.chauvel@gmail.com>
Date
Oct 2, 2026, 13:34 UTC
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:
Show 5 quoted lines
> 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
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 6 of 18 in “packfile: fix corruption due to stale delta base cache entries”
  1. 0/2 packfile: fix corruption due to stale delta base cache entriesPatrick Steinhardt, Oct 2, 2026
  2. 1/2 packfile: move around `close_pack()`Patrick Steinhardt, Oct 2, 2026
  3. Mark C. Chu-CarrollOct 2, 2026
  4. Patrick SteinhardtOct 2, 2026
  5. 2/2 packfile: fix corruption due to stale delta base cache entriesPatrick Steinhardt, Oct 2, 2026
  6. Guillaume ChauvelOct 2, 2026
  7. Patrick SteinhardtOct 2, 2026
  8. Philippe BlainOct 2, 2026
  9. Patrick SteinhardtOct 2, 2026
  10. Jeff KingOct 2, 2026
  11. Patrick SteinhardtOct 5, 2026
  12. Jeff KingOct 7, 2026
  13. Junio C HamanoOct 7, 2026
  14. 0/2 packfile: fix corruption due to stale delta base cache entriesPatrick Steinhardt, Oct 6, 2026
  15. 1/2 packfile: move around `close_pack()`Patrick Steinhardt, Oct 6, 2026
  16. 2/2 packfile: fix corruption due to stale delta base cache entriesPatrick Steinhardt, Oct 6, 2026
  17. Junio C HamanoOct 6, 2026
  18. Patrick SteinhardtOct 7, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.