On 2024.08.28 20:29, Calvin Wan wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

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