Re: [PATCH V2 5/6] pack-bitmap: fix a memleak

From: Jeff King <peff@peff.net>
Date: Mar 31, 2015, 03:29 UTC
Message-ID: <20150331032953.GA842@peff.net>
In-Reply-To: <1427495569-10863-6-git-send-email-sbeller@google.com>

On Fri, Mar 27, 2015 at 03:32:48PM -0700, Stefan Beller wrote:

> `recent_bitmaps` is allocated in the function load_bitmap_entries_v1
> and it is not passed into any function, so it's safe to free it before
> leaving that function.

I think this is OK, though it might be easier still to just turn the array into a stack variable. It's only 160 * sizeof(ptr), or about 1280 bytes on a 64-bit system. Note that the xcalloc there looks wrong (it is way over-allocating by using the sizeof the struct, not a pointer to the struct).

> Notes:
>     I wonder however if we need to free the actual bitmaps
>     stored in the recent_bitmaps as well.

No, those are just weak pointers. The memory is owned by the hash that store_bitmap puts the bitmaps into.

>  		bitmap = read_bitmap_1(index);

This line allocates, too. So if we get down to...

Show 6 quoted lines

> -		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i)
> -			return error("Corrupted bitmap pack index");
> +		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {
> +			ret = error("Corrupted bitmap pack index");
> +			goto out;
> +		}

...here, for example, where we have not yet called store_bitmap, then it's a leak, too.

-Peff