diff mbox series

[v2,03/24] pack-bitmap-write.c: free existing bitmaps

Message ID 490d733d121e206ccdc335812c03f31c380bcd86.1624314293.git.me@ttaylorr.com (mailing list archive)
State New
Headers show
Series multi-pack reachability bitmaps | expand

Commit Message

Taylor Blau June 21, 2021, 10:25 p.m. UTC
When writing a new bitmap, the bitmap writer code attempts to read the
existing bitmap (if one is present). This is done in order to quickly
permute the bits of any bitmaps for commits which appear in the existing
bitmap, and were also selected for the new bitmap.

But since this code was added in 341fa34887 (pack-bitmap-write: use
existing bitmaps, 2020-12-08), the resources associated with opening an
existing bitmap were never released.

It's fine to ignore this, but it's bad hygiene. It will also cause a
problem for the multi-pack-index builtin, which will be responsible not
only for writing bitmaps, but also for expiring any old multi-pack
bitmaps.

If an existing bitmap was reused here, it will also be expired. That
will cause a problem on platforms which require file resources to be
closed before unlinking them, like Windows. Avoid this by ensuring we
close reused bitmaps with free_bitmap_index() before removing them.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap-write.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jeff King July 21, 2021, 9:54 a.m. UTC | #1
On Mon, Jun 21, 2021 at 06:25:04PM -0400, Taylor Blau wrote:

> When writing a new bitmap, the bitmap writer code attempts to read the
> existing bitmap (if one is present). This is done in order to quickly
> permute the bits of any bitmaps for commits which appear in the existing
> bitmap, and were also selected for the new bitmap.
> 
> But since this code was added in 341fa34887 (pack-bitmap-write: use
> existing bitmaps, 2020-12-08), the resources associated with opening an
> existing bitmap were never released.
> 
> It's fine to ignore this, but it's bad hygiene. It will also cause a
> problem for the multi-pack-index builtin, which will be responsible not
> only for writing bitmaps, but also for expiring any old multi-pack
> bitmaps.
> 
> If an existing bitmap was reused here, it will also be expired. That
> will cause a problem on platforms which require file resources to be
> closed before unlinking them, like Windows. Avoid this by ensuring we
> close reused bitmaps with free_bitmap_index() before removing them.

I agree with all of that. But just "it's a memory leak" would have
contented me, too. :)

-Peff
diff mbox series

Patch

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index d374f7884b..142fd0adb8 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -520,6 +520,7 @@  int bitmap_writer_build(struct packing_data *to_pack)
 	clear_prio_queue(&queue);
 	clear_prio_queue(&tree_queue);
 	bitmap_builder_clear(&bb);
+	free_bitmap_index(old_bitmap);
 	free(mapping);
 
 	trace2_region_leave("pack-bitmap-write", "building_bitmaps_total",