git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH] ref-filter: fix memory leak in `free_array_item()`
@ 2019-07-10 18:36 Martin Ågren
  2019-07-10 20:41 ` Junio C Hamano
  0 siblings, 1 reply; 2+ messages in thread
From: Martin Ågren @ 2019-07-10 18:36 UTC (permalink / raw)
  To: git

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] ref-filter: fix memory leak in `free_array_item()`
  2019-07-10 18:36 [PATCH] ref-filter: fix memory leak in `free_array_item()` Martin Ågren
@ 2019-07-10 20:41 ` Junio C Hamano
  0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2019-07-10 20:41 UTC (permalink / raw)
  To: Martin Ågren; +Cc: git

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);

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-10 18:36 [PATCH] ref-filter: fix memory leak in `free_array_item()` Martin Ågren
2019-07-10 20:41 ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox