[PATCH v3] pack-bitmap: remove checks before bitmap_free

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. 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 a pseudo-merge whose size is equal to or smaller than roots_bitmap (which corresponds to the set of "haves" commits in prepare_bitmap_walk()). To do this, we can create two commits: A and B. Add A to the pseudo-merge list and perform a traversal over the range A..B. In this scenario, the "haves" set will be {A}, and cascade_pseudo_merges_1() will succeed, thereby exposing the leak due to the missing roots_bitmap cleanup.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
    pack-bitmap: remove checks before bitmap_free
    
    In pack-bitmap.c:find_boundary_objects, remove cascade success check and
    always free roots_bitmap afterward to make static analysis tool works
    better.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1977%2Fbrandb97%2Fremove-check-before-bitmap-free-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v3
Pull-Request: https://github.com/git/git/pull/1977

Range-diff vs v2:

 1:  d7b7a0e29ec < -:  ----------- pack-bitmap: remove checks before bitmap_free
 2:  56b24d681cb ! 1:  151a7f5dc70 t5333: test memory leak when use pseudo-merge in boundary traversal
     @@ Metadata
      Author: Lidong Yan <502024330056@smail.nju.edu.cn>
      
       ## Commit message ##
     -    t5333: test memory leak when use pseudo-merge in boundary traversal
     +    pack-bitmap: remove checks before bitmap_free
      
          In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed
     -    if cascade_pseudo_merges_1() fails. Otherwise, it leaks, leading to
     -    a memory leak that currently lacks a dedicated test to detect it.
     +    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.
     +    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 a pseudo-merge whose size is equal to
          or smaller than roots_bitmap (which corresponds to the set of "haves"
          commits in prepare_bitmap_walk()). To do this, we can create two
          commits: A and B. Add A to the pseudo-merge list and perform a traversal
          over the range A..B. In this scenario, the "haves" set will be {A},
     -    and cascade_pseudo_merges_1() will succeed — thereby exposing the leak
     +    and cascade_pseudo_merges_1() will succeed, thereby exposing the leak
          due to the missing roots_bitmap cleanup.
      
          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
      
     + ## pack-bitmap.c ##
     +@@ pack-bitmap.c: 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);
     + 	}
     + 
     + 	/*
     +
       ## t/t5333-pseudo-merge-bitmaps.sh ##
      @@ t/t5333-pseudo-merge-bitmaps.sh: test_expect_success 'pseudo-merge closure' '
       	)
     @@ t/t5333-pseudo-merge-bitmaps.sh: test_expect_success 'pseudo-merge closure' '
      +		git config bitmapPseudoMerge.test.pattern refs/ &&
      +		git config bitmapPseudoMerge.test.threshold now &&
      +		git config bitmapPseudoMerge.test.stableThreshold now &&
     -+		export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&
     ++		GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&
      +
      +		test_commit A &&
      +		git repack -adb &&

 pack-bitmap.c                   |  4 ++--
 t/t5333-pseudo-merge-bitmaps.sh | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)