[PATCH] ref-filter: fix memory leak in `free_array_item()`

[PATCH] ref-filter: fix memory leak in `free_array_item()`

[Martin Ågren]

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 <martin.agren@gmail.com>
---
 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

Re: [PATCH] ref-filter: fix memory leak in `free_array_item()`

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> 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 <martin.agren@gmail.com>
> ---
>  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);