Junio C Hamano <gitster@xxxxxxxxx> 写道： > > "Lidong Yan via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> 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. > > "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. I think this was a simple thinko, similar to "we need to free resources if something fails. Since cascade_pseudo_merges_1() fails, we need to free roots_bitmap”. In commit 55e563a (pseudo-merge: fix various memory leaks, 2024-09-30), Patrick fixed a similar leak in find_objects(). However, since t5333 doesn’t test boundary traversal, the leak in find_boundary_objects() remains unresolved.
