Re: [PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak

From: John Keeping <john@keeping.me.uk>
Date: Mar 28, 2015, 11:09 UTC
Message-ID: <20150328110957.GB1097@serenity.lan>
In-Reply-To: <CAPig+cR8zA_FLhhnUgKZPdUFpCM3yNKNfbEBH+yoY=G44xGk8w@mail.gmail.com>

On Fri, Mar 27, 2015 at 08:14:28PM -0400, Eric Sunshine wrote:

Show 29 quoted lines

> On Friday, March 27, 2015, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller <sbeller@google.com> wrote:
> > > I  observe that filter is going out of scope, but the
> > > implementation proposed in this patch produces just a
> > > crash instead of any helpful fix.
> > >
> > > Signed-off-by: Stefan Beller <sbeller@google.com>
> > > ---
> > > diff --git a/entry.c b/entry.c
> > > index 1eda8e9..5383001 100644
> > > --- a/entry.c
> > > +++ b/entry.c
> > > @@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,
> > >                 if (filter &&
> > >                     !streaming_write_entry(ce, path, filter,
> > >                                            state, to_tempfile,
> > > -                                          &fstat_done, &st))
> > > +                                          &fstat_done, &st)) {
> > > +                       free_stream_filter(filter);
> >
> > Aside from the crash you are seeing, this is a bogus fix anyway.
> > You're only freeing 'filter' if it was allocated _and_ if
> > streaming_write_entry() returned 0. I would guess your intention was
> > to free 'filter' regardless of the result of streaming_write_entry().
> 
> Unless streaming_write_entry() is freeing the filter for you -- there
> is a free_stream_filter() call in close_method_decl() in streaming.c
> -- in which case your new free_stream_filter() call would attempt to
> free the already-freed filter.

Yes, I think the correct fix for this leak is to make stream_blob_to_fd() always free the filter, since there's only one path out that doesn't at the moment and there's no way for the caller to figure out whether or not the filter has been freed:

-- >8 --

diff --git a/streaming.c b/streaming.c
index 2ff036a..811fcc2 100644
--- a/streaming.c
+++ b/streaming.c
@@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
 	int result = -1;
 
 	st = open_istream(sha1, &type, &sz, filter);
-	if (!st)
+	if (!st) {
+		if (filter)
+			free_stream_filter(filter);
 		return result;
+	}
 	if (type != OBJ_BLOB)
 		goto close_and_exit;
 	for (;;) {