Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 28, 2024 at 03:19:10PM -0700, Josh Steadmon wrote:
> On 2024.08.28 20:29, Calvin Wan wrote:
> > Patrick Steinhardt <ps@pks.im> 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@pks.im>
> > > ---
> > >  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?

Yeah, that's mostly the reason. We have e.g. `string_list_clear_func()`
that works around this issue by making the caller provide the cleanup
function, and we could use the same pattern here. But it seems as if
most of the callers of `clear_prio_queue()` don't need this because they
already drain the queue during normal operations anyway.

With patch 14 you probably refer to `clear_shallow_info()`? We're not
using a priority queue there, so it is not quite related to the prio
queue we have here. So I'm inclined to leave this as-is, and if we ever
see that we have more callsites that want to clean up the prio queue and
its contents we can introduce `prio_queue_clear_func()`. Does that work
for you?

> > 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.

I'd certainly be happy to shed some of the load here. If you do, please
give me a quick ping so that we don't duplicate any work by accident.

> The series also looks good to me, thanks!
> 
> Reviewed-by: Josh Steadmon <steadmon@google.com>

Thanks!

Patrick