[PATCH] small memory leak fix

[PATCH] small memory leak fix

[David CARLIER]

[-- Attachment #1: Type: text/plain, Size: 118 bytes --]

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

Kind regards.

[-- Attachment #2: 0001-memory-leaks-fixes-for-remote-lists.patch --]
[-- Type: application/octet-stream, Size: 820 bytes --]

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(+)

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

Re: [PATCH] small memory leak fix

[Stefan Beller]

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

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

Re: [PATCH] small memory leak fix

[Jeff King]

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

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