Re: [PATCH] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed

From: Jeff King <peff@peff.net>
Date: May 14, 2025, 18:03 UTC
Message-ID: <20250514180325.GB2196784@coredump.intra.peff.net>
In-Reply-To: <aCOFqYdnPp1Lne4Y@nand.local>

On Tue, May 13, 2025 at 01:47:21PM -0400, Taylor Blau wrote:

Show 11 quoted lines

> > > In pack-bitmap.c:load_bitmap_entries_v1, the function `read_bitmap_1`
> > > allocates a bitmap and reads index data into it. However, if any of
> > > the validation checks following the allocation fail, the allocated bitmap
> > > is not freed, resulting in a memory leak. To avoid this, the validation
> > > checks should be performed before the bitmap is allocated.
> >
> > Thanks, this looks correct to me.
> 
> It looks correct to me as well, and is a strict improvement. But I think
> there is a leak outside of this function as well that is not touched by
> this patch.

Good catch, and your analysis looks correct to me. I don't think that changes anything for this patch, which is fixing a more "inner" issue of the allocated memory hitting store_bitmap() at all.

So I think this can graduate independently, and then you can prepare your fix on top (but no rush).

It would be nice if we triggered these cases in the test suite so that LSan could confirm that all leaks are covered. But I suspect it may not be worth the effort to craft a bitmap file that is broken in such particular ways.

Show 5 quoted lines

> I suspect the fix looks something like:
> [...]
> , since all callers of load_bitmap() will themselves call
> free_bitmap_index(), so there is no need for us to open-code a portion
> of that function's implementation ourselves.

Deleting that extra code would be doubly satisfying.

-Peff