Re: [PATCH] small memory leak fix

[Jeff King <peff@peff.net>]

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