| Message ID | 6f5ff96998946f3f49da56fd05c096b949521339.1701198172.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-objects: multi-pack verbatim reuse | expand |
On Tue, Nov 28, 2023 at 02:07:59PM -0500, Taylor Blau wrote: > The `bb_commit` commit slab is used by the pack-bitmap-write machinery > to track various pieces of bookkeeping used to generate reachability > bitmaps. > > Even though we clear the slab when freeing the bitmap_builder struct > (with `bitmap_builder_clear()`), there are still pointers which point to > locations in memory that have not yet been freed, resulting in a leak. > > Plug the leak by introducing a suitable `free_fn` for the `struct > bb_commit` type, and make sure it is called on each member of the slab > via the `deep_clear_bb_data()` function. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > pack-bitmap-write.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c > index f4ecdf8b0e..dd3a415b9d 100644 > --- a/pack-bitmap-write.c > +++ b/pack-bitmap-write.c > @@ -198,6 +198,13 @@ struct bb_commit { > unsigned idx; /* within selected array */ > }; > > +static void clear_bb_commit(struct bb_commit *commit) > +{ > + free(commit->reverse_edges); I'd have expected to see `free_commit_list()` here instead of a simple free. Is there any reason why we don't use it? Patrick > + bitmap_free(commit->commit_mask); > + bitmap_free(commit->bitmap); > +} > + > define_commit_slab(bb_data, struct bb_commit); > > struct bitmap_builder { > @@ -339,7 +346,7 @@ static void bitmap_builder_init(struct bitmap_builder *bb, > > static void bitmap_builder_clear(struct bitmap_builder *bb) > { > - clear_bb_data(&bb->data); > + deep_clear_bb_data(&bb->data, clear_bb_commit); > free(bb->commits); > bb->commits_nr = bb->commits_alloc = 0; > } > -- > 2.43.0.24.g980b318f98 >
On Thu, Nov 30, 2023 at 11:18:31AM +0100, Patrick Steinhardt wrote: > > diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c > > index f4ecdf8b0e..dd3a415b9d 100644 > > --- a/pack-bitmap-write.c > > +++ b/pack-bitmap-write.c > > @@ -198,6 +198,13 @@ struct bb_commit { > > unsigned idx; /* within selected array */ > > }; > > > > +static void clear_bb_commit(struct bb_commit *commit) > > +{ > > + free(commit->reverse_edges); > > I'd have expected to see `free_commit_list()` here instead of a simple > free. Is there any reason why we don't use it? Thanks for spotting an oversight on my part. We should definitely be using free_commit_list() here instead of a bare free() to avoid leaking the tail. Thanks, Taylor
On Tue, Nov 28, 2023 at 02:07:59PM -0500, Taylor Blau wrote: > +static void clear_bb_commit(struct bb_commit *commit) > +{ > + free(commit->reverse_edges); > + bitmap_free(commit->commit_mask); > + bitmap_free(commit->bitmap); > +} I think these bitmaps may sometimes be NULL. But double-checking bitmap_free(), it sensibly is noop when passed NULL. So this look good to me. -Peff
On Tue, Dec 12, 2023 at 02:04:06AM -0500, Jeff King wrote: > On Tue, Nov 28, 2023 at 02:07:59PM -0500, Taylor Blau wrote: > > > +static void clear_bb_commit(struct bb_commit *commit) > > +{ > > + free(commit->reverse_edges); > > + bitmap_free(commit->commit_mask); > > + bitmap_free(commit->bitmap); > > +} > > I think these bitmaps may sometimes be NULL. But double-checking > bitmap_free(), it sensibly is noop when passed NULL. So this look good > to me. Yeah, bitamp_free() handles a NULL input correctly, so we can pass a possibly-NULL `commit->commit_mask` or `commit->bitmap` argument and be OK. Thanks, Taylor
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index f4ecdf8b0e..dd3a415b9d 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -198,6 +198,13 @@ struct bb_commit { unsigned idx; /* within selected array */ }; +static void clear_bb_commit(struct bb_commit *commit) +{ + free(commit->reverse_edges); + bitmap_free(commit->commit_mask); + bitmap_free(commit->bitmap); +} + define_commit_slab(bb_data, struct bb_commit); struct bitmap_builder { @@ -339,7 +346,7 @@ static void bitmap_builder_init(struct bitmap_builder *bb, static void bitmap_builder_clear(struct bitmap_builder *bb) { - clear_bb_data(&bb->data); + deep_clear_bb_data(&bb->data, clear_bb_commit); free(bb->commits); bb->commits_nr = bb->commits_alloc = 0; }
The `bb_commit` commit slab is used by the pack-bitmap-write machinery to track various pieces of bookkeeping used to generate reachability bitmaps. Even though we clear the slab when freeing the bitmap_builder struct (with `bitmap_builder_clear()`), there are still pointers which point to locations in memory that have not yet been freed, resulting in a leak. Plug the leak by introducing a suitable `free_fn` for the `struct bb_commit` type, and make sure it is called on each member of the slab via the `deep_clear_bb_data()` function. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pack-bitmap-write.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)