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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 28, 2015, 00:14 UTC
Message-ID: <CAPig+cR8zA_FLhhnUgKZPdUFpCM3yNKNfbEBH+yoY=G44xGk8w@mail.gmail.com>
In-Reply-To: <CAPig+cSHntg6KwZr3h6nhO7AK0v+FcbVAA4b_kvHdEg_=9o_eg@mail.gmail.com>

On Friday, March 27, 2015, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 23 quoted lines

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