From: Lidong Yan <redacted>

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 roots_bitmap contains at least one pseudo
merge. So that we can use pseudo merge bitmap when we compute roots
reachable bitmap. Here we create two commits: first A then B. Add A
to the pseudo-merge 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 <redacted>
---
    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-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v5
Pull-Request: https://github.com/git/git/pull/1977

Range-diff vs v4:

 1:  fa443065436 ! 1:  4bc90c83a40 pack-bitmap: remove checks before bitmap_free
     @@ Commit message
          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.
     +    To trigger this leak, we need roots_bitmap contains at least one pseudo
     +    merge. So that we can use pseudo merge bitmap when we compute roots
     +    reachable bitmap. Here we create two commits: first A then B. Add A
     +    to the pseudo-merge 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 [off-list ref]
      
     @@ t/t5333-pseudo-merge-bitmaps.sh: test_expect_success 'pseudo-merge closure' '
      +		cd pseudo-merge-boundary-traversal &&
      +
      +		git config bitmapPseudoMerge.test.pattern refs/ &&
     -+		git config bitmapPseudoMerge.test.threshold now &&
     -+		git config bitmapPseudoMerge.test.stableThreshold now &&
     ++		git config pack.useBitmapBoundaryTraversal true &&
      +
      +		test_commit A &&
      +		git repack -adb &&
      +		test_commit B &&
      +
     -+		echo '1' >expect &&
     -+		GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 \
     -+			git rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&
     -+		test_cmp expect actual
     ++		nr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&
     ++		test 1 -eq "$nr"
      +	)
      +'
      +


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

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);
 	}
 
 	/*

diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh
index 56674db562f..ba5ae6a00c9 100755
--- a/t/t5333-pseudo-merge-bitmaps.sh
+++ b/t/t5333-pseudo-merge-bitmaps.sh

@@ -445,4 +445,21 @@ test_expect_success 'pseudo-merge closure' '
 	)
 '
 
+test_expect_success 'use pseudo-merge in boundary traversal' '
+	git init pseudo-merge-boundary-traversal &&
+	(
+		cd pseudo-merge-boundary-traversal &&
+
+		git config bitmapPseudoMerge.test.pattern refs/ &&
+		git config pack.useBitmapBoundaryTraversal true &&
+
+		test_commit A &&
+		git repack -adb &&
+		test_commit B &&
+
+		nr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&
+		test 1 -eq "$nr"
+	)
+'
+
 test_done

base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
-- 
gitgitgadget