| Message ID | 920db3a2912c609d4ac1fca2fc6b137513c8eceb.1724656120.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.6) | expand |
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.
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>
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
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 --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' '
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(-)