[PATCH 3/4] pack-revindex: avoid double-loading .rev files

The usual entry point for loading the pack revindex is the load_pack_revindex() function. It returns immediately if the packed_git has a non-NULL revindex or revindex data field (representing an in-memory or mmap'd .rev file, respectively), since the data is already loaded.

But in 5a6072f631 (fsck: validate .rev file header, 2023-04-17) the fsck code path switched to calling load_pack_revindex_from_disk() directly, since it wants to check the on-disk data (if there is any). But that function does _not_ check to see if the data has already been loaded; it just maps the file, overwriting the revindex_map pointer (and pointing revindex_data inside that map). And in that case we've leaked the mmap() pointed to by revindex_map (if it was non-NULL).

This usually doesn't happen, since fsck wouldn't need to load the revindex for any reason before we get to these checks. But there are some cases where it does. For example, is_promisor_object() runs odb_for_each_object() with the PACK_ORDER flag, which uses the revindex.

This happens a few times in our test suite, but SANITIZE=leak doesn't detect it because we are leaking an mmap(), not a heap-allocated buffer from malloc(). However, if you build with NO_MMAP, then our compat mmap will read into a heap buffer instead, and LSan will complain. This causes failures in t5601, t0410, t5702, and t5616.

We can fix it by checking for existing revindex_data when loading from disk. This is redundant when we're called from load_pack_revindex(), but it's a cheap check. The alternative is to teach check_pack_rev_indexes() in fsck to skip the load, but that seems messier; it doesn't otherwise know about internals like revindex_map and revindex_data.

Signed-off-by: Jeff King <peff@peff.net>
---
 pack-revindex.c | 4 ++++
 1 file changed, 4 insertions(+)