On 2024.08.28 20:29, Calvin Wan wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
> > When releasing the skipping negotiator we free its priority queue, but
> > not the contained entries. Fix this to plug a memory leak.
> >
> > Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> > ---
> > negotiator/skipping.c | 7 +++++--
> > t/t5552-skipping-fetch-negotiator.sh | 2 ++
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/negotiator/skipping.c b/negotiator/skipping.c
> > index f65d47858b4..b738fe4faef 100644
> > --- a/negotiator/skipping.c
> > +++ b/negotiator/skipping.c
> > @@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
> >
> > static void release(struct fetch_negotiator *n)
> > {
> > - clear_prio_queue(&((struct data *)n->data)->rev_list);
> > - FREE_AND_NULL(n->data);
> > + struct data *data = n->data;
> > + for (int i = 0; i < data->rev_list.nr; i++)
> > + free(data->rev_list.array[i].data);
> > + clear_prio_queue(&data->rev_list);
> > + FREE_AND_NULL(data);
> > }
> >
>
> It seems unintuitive that clear_prio_queue() doesn't also free the data
> underneath and that a caller would have to know to free that as well to
> avoid leaking memory. Would it make more sense to add this change to
> clear_prio_queue() instead? Patch 14 has that pattern already.
I'm assuming the reasoning is that clear_prio_queue() can't know if its
items need more complicated cleanup of their own, so if the caller
(potentially) needs to clean up items individually anyway, the caller
can also free them at the same time?
> Thanks again for the cleanups -- I'm tempted to take a stab at some of
> the other memory leaks you mentioned during our biweekly hackathon. All
> of the other patches look reasonable to me.
The series also looks good to me, thanks!
Reviewed-by: Josh Steadmon <steadmon@xxxxxxxxxx>
