diff mbox series

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

Message ID 920db3a2912c609d4ac1fca2fc6b137513c8eceb.1724656120.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Memory leak fixes (pt.6) | expand

Commit Message

Patrick Steinhardt Aug. 26, 2024, 7:22 a.m. UTC
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(-)

Comments

Calvin Wan Aug. 28, 2024, 8:29 p.m. UTC | #1
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.

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.
Josh Steadmon Aug. 28, 2024, 10:19 p.m. UTC | #2
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?

> 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@google.com>
Patrick Steinhardt Aug. 29, 2024, 8:41 a.m. UTC | #3
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
Calvin Wan Aug. 29, 2024, 5:29 p.m. UTC | #4
On Thu, Aug 29, 2024 at 1:41 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> 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?

Yes it does! Thanks for the clarification
diff mbox series

Patch

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);
 }
 
 void skipping_negotiator_init(struct fetch_negotiator *negotiator)
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index b55a9f65e6b..4f2e5ae8dfa 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -1,6 +1,8 @@ 
 #!/bin/sh
 
 test_description='test skipping fetch negotiator'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'fetch.negotiationalgorithm config' '