# [PATCH] ref-filter: fix memory leak in `free_array_item()` 2 messages from 2019-07-10 to 2019-07-10. Participants: Martin Ågren, Junio C Hamano. Thread: https://gitlist.dev/t/51459 ## Martin Ågren, 2019-07-10 18:36 Subject: [PATCH] ref-filter: fix memory leak in `free_array_item()` Message-ID: <20190710183639.4123992-1-martin.agren@gmail.com> URL: https://gitlist.dev/e/20190710183639.4123992-1-martin.agren%40gmail.com ``` We treat the `value` pointer as a pointer to a struct and free its `s` field. But `value` is in fact an array of structs. As a result, we only free the first `s` out of `used_atom_cnt`-many and leak the rest. Make sure we free all items in `value`. In the caller, `ref_array_clear()`, this means we need to be careful not to zero `used_atom_cnt` until after we've called `free_array_item()`. We could move just a single line, but let's keep related things close together instead, by first handling `array`, then `used_atom`. Signed-off-by: Martin Ågren --- ref-filter.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 791f0648a6..1c1a2af880 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2105,7 +2105,9 @@ static void free_array_item(struct ref_array_item *item) { free((char *)item->symref); if (item->value) { - free((char *)item->value->s); + int i; + for (i = 0; i < used_atom_cnt; i++) + free((char *)item->value[i].s); free(item->value); } free(item); @@ -2116,14 +2118,16 @@ void ref_array_clear(struct ref_array *array) { int i; - for (i = 0; i < used_atom_cnt; i++) - free((char *)used_atom[i].name); - FREE_AND_NULL(used_atom); - used_atom_cnt = 0; for (i = 0; i < array->nr; i++) free_array_item(array->items[i]); FREE_AND_NULL(array->items); array->nr = array->alloc = 0; + + for (i = 0; i < used_atom_cnt; i++) + free((char *)used_atom[i].name); + FREE_AND_NULL(used_atom); + used_atom_cnt = 0; + if (ref_to_worktree_map.worktrees) { hashmap_free(&(ref_to_worktree_map.map), 1); free_worktrees(ref_to_worktree_map.worktrees); -- 2.22.0.428.g6d5b264208 ``` ## Junio C Hamano, 2019-07-10 20:41 Subject: Re: [PATCH] ref-filter: fix memory leak in `free_array_item()` Message-ID: URL: https://gitlist.dev/e/xmqqr26xsjv7.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190710183639.4123992-1-martin.agren@gmail.com> ``` Martin Ågren writes: > We treat the `value` pointer as a pointer to a struct and free its `s` > field. But `value` is in fact an array of structs. As a result, we only > free the first `s` out of `used_atom_cnt`-many and leak the rest. Make > sure we free all items in `value`. Thanks for spotting. We do allocate an array of used_atom_cnt elements in populate_value() and we need to free them all. > In the caller, `ref_array_clear()`, this means we need to be careful not > to zero `used_atom_cnt` until after we've called `free_array_item()`. We > could move just a single line, but let's keep related things close > together instead, by first handling `array`, then `used_atom`. Yup. Looking good. Thanks. > > Signed-off-by: Martin Ågren > --- > ref-filter.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 791f0648a6..1c1a2af880 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -2105,7 +2105,9 @@ static void free_array_item(struct ref_array_item *item) > { > free((char *)item->symref); > if (item->value) { > - free((char *)item->value->s); > + int i; > + for (i = 0; i < used_atom_cnt; i++) > + free((char *)item->value[i].s); > free(item->value); > } > free(item); > @@ -2116,14 +2118,16 @@ void ref_array_clear(struct ref_array *array) > { > int i; > > - for (i = 0; i < used_atom_cnt; i++) > - free((char *)used_atom[i].name); > - FREE_AND_NULL(used_atom); > - used_atom_cnt = 0; > for (i = 0; i < array->nr; i++) > free_array_item(array->items[i]); > FREE_AND_NULL(array->items); > array->nr = array->alloc = 0; > + > + for (i = 0; i < used_atom_cnt; i++) > + free((char *)used_atom[i].name); > + FREE_AND_NULL(used_atom); > + used_atom_cnt = 0; > + > if (ref_to_worktree_map.worktrees) { > hashmap_free(&(ref_to_worktree_map.map), 1); > free_worktrees(ref_to_worktree_map.worktrees); ```