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

[Philippe Blain <levraiphilippeblain@gmail.com>]

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.