threads / patch / 45844

patchsmall memory leak fix

Subject: [PATCH] small memory leak fix

## tl;dr

3 messages between May 1, 2017 and May 1, 2017. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

David CARLIER· May 1, 2017, 20:40 UTC · lore

Hi this is my first submission, a memory leak found in the maint branch (might be in master as well).

Kind regards.

From d98f3944780730447f111a4178c9d99f5110c260 Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen@gmail.com>
Date: Mon, 1 May 2017 21:14:09 +0100
Subject: [PATCH] memory leaks fixes for remote lists.

both push and fetch lists were not freed thus using free_refspecs usage.

Signed-off-by: David Carlier <devnexen@gmail.com>
---
 remote.c | 2 ++
 1 file changed, 2 insertions(+)

Show changes to remote.c +2 −0

diff --git a/remote.c b/remote.c
index 9f83fe2c4..2f8cb35a3 100644
--- a/remote.c
+++ b/remote.c
@@ -742,6 +742,8 @@ int for_each_remote(each_remote_fn fn, void *priv)
 			r->push = parse_push_refspec(r->push_refspec_nr,
 						     r->push_refspec);
 		result = fn(r, priv);
+		free_refspecs(r->push, r->push_refspec_nr);
+		free_refspecs(r->fetch, r->fetch_refspec_nr);
 	}
 	return result;
 }

-- 
2.12.2

Stefan Beller· May 1, 2017, 20:50 UTC · re: David CARLIER · lore

Re: [PATCH] small memory leak fix

On Mon, May 1, 2017 at 1:40 PM, David CARLIER <devnexen@gmail.com> wrote:

> Hi this is my first submission, a memory leak found in the maint branch (might be in master as well).
>
> Kind regards.

Hi and welcome to Git!

> From d98f3944780730447f111a4178c9d99f5110c260 Mon Sep 17 00:00:00 2001
> From: David Carlier <devnexen@gmail.com>
> Date: Mon, 1 May 2017 21:14:09 +0100
> Subject: [PATCH] memory leaks fixes for remote lists.

We usually word the subject line as "area: what we do" (C.f. $ git log --oneline --no-merges -- remote.c) Maybe:

    remote.c: plug memleak in for_each_remote

Show 19 quoted lines

>
> both push and fetch lists were not freed thus
> using free_refspecs usage.
>
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
> remote.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/remote.c b/remote.c
> index 9f83fe2c4..2f8cb35a3 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -742,6 +742,8 @@ int for_each_remote(each_remote_fn fn, void *priv)
>  r->push = parse_push_refspec(r->push_refspec_nr,
>      r->push_refspec);
>  result = fn(r, priv);
> + free_refspecs(r->push, r->push_refspec_nr);
> + free_refspecs(r->fetch, r->fetch_refspec_nr);

After freeing the refspec, r->push/fetch still points to the (now free'd) memory. We'd want to reset it to NULL as well, such that e.g. in this function

    if (!r->fetch)
        ...

still works.

After reading this, I think we'd rather want to keep the fetch/push refspec around for the next access of the struct remote, and only free the memory when the remote itself is freed?

That however is a problem as we never free them, they are globals :(

Thanks, Stefan

Jeff King· May 1, 2017, 21:38 UTC · re: Stefan Beller · lore

Re: [PATCH] small memory leak fix

On Mon, May 01, 2017 at 01:50:57PM -0700, Stefan Beller wrote:

Show 25 quoted lines

> > diff --git a/remote.c b/remote.c
> > index 9f83fe2c4..2f8cb35a3 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -742,6 +742,8 @@ int for_each_remote(each_remote_fn fn, void *priv)
> >  r->push = parse_push_refspec(r->push_refspec_nr,
> >      r->push_refspec);
> >  result = fn(r, priv);
> > + free_refspecs(r->push, r->push_refspec_nr);
> > + free_refspecs(r->fetch, r->fetch_refspec_nr);
> 
> After freeing the refspec, r->push/fetch still points to
> the (now free'd) memory. We'd want to reset it to NULL as well,
> such that e.g. in this function
> 
>     if (!r->fetch)
>         ...
> 
> still works.
> 
> After reading this, I think we'd rather want to keep the fetch/push refspec
> around for the next access of the struct remote, and only free the memory
> when the remote itself is freed?
> 
> That however is a problem as we never free them, they are globals :(

Yeah, I think the point is that the whole "remotes" array is a program-length global that never goes away (and must not, because after read_config() sets the "loaded" flag, we would never reload it).

The "fetch" and "push" bits are lazily parsed from the refspec strings, but are intended to have the same lifetime. Unlike the rest of it, we _could_ drop them after use and then lazy-parse them again.

But note that we call an arbitrary callback in this function. What expectations does it have for the lifetimes? Do any of the callbacks record pointers to the refspecs? Or for that mater, the patch as shown frees the refspecs even if we didn't just lazily allocate them in this function (e.g., if we did so in remote_get_1()).

So I don't think freeing them is safe unless we do a complete audit of access to those refspecs. And it's probably not worth the trouble; these should just follow the same global-until-exit allocation scheme as the rest of "struct remote".

-Peff