From: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxx>
## 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@xxxxxxxxxxxxxxxx>
+ ## 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(-)
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..454f8c7a817 100755
--- a/t/t5333-pseudo-merge-bitmaps.sh
+++ b/t/t5333-pseudo-merge-bitmaps.sh
@@ -445,4 +445,24 @@ 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 bitmapPseudoMerge.test.threshold now &&
+ git config bitmapPseudoMerge.test.stableThreshold now &&
+ GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&
+
+ test_commit A &&
+ git repack -adb &&
+ test_commit B &&
+
+ echo '1' >expect &&
+ git rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
--
gitgitgadget
