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

From: Jeff King <peff@peff.net>
Date: Oct 7, 2026, 08:18 UTC
Message-ID: <20261007081844.GA606386@coredump.intra.peff.net>
In-Reply-To: <asM2YoImN8bHLHj8@pks.im>

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

Show 9 quoted lines

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