Re: [PATCH v5] pack-bitmap: remove checks before bitmap_free

> 
> "Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>> 
>> In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed
>> if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only
>> use roots_bitmap as a mutable reference but not takes roots_bitmap's
>> ownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.
> 
> "Once cascade_pseudo_merges_1() succeeds", perhaps?
> 
>> And this leak currently lacks a dedicated test to detect it.
>> 
>> To fix this leak, remove if cascade_pseudo_merges_1() succeed check and
>> always calling bitmap_free(roots_bitmap);
>> 
>> To trigger this leak, we need roots_bitmap contains at least one pseudo
>> merge.
> 
> "contains" -> "that contains"?
> 
>> diff --git a/pack-bitmap.c b/pack-bitmap.c
>> index ac6d62b980c..8727f316de9 100644
>> --- a/pack-bitmap.c
>> +++ b/pack-bitmap.c
>> @@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,
>> bitmap_set(roots_bitmap, pos);
>> }
>> 
>> - if (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))
>> - bitmap_free(roots_bitmap);
>> + cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);
>> + bitmap_free(roots_bitmap);
> 
> This makes it as if the original _wanted_ to leak it when the call
> failed.  Readers may wonder how we got into this state in the first
> place.  Was it a simple thinko when 11d45a6e (pack-bitmap.c: use
> pseudo-merges during traversal, 2024-05-23) was written, I have to
> wonder.