[FFmpeg-devel] [PATCH] vf_pullup: simplify, fix double free error

[wm4]

On Tue, 25 Mar 2014 15:01:02 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Tue, Mar 25, 2014 at 01:59:58PM +0100, wm4 wrote:
> > The memory allocation for f->diffs was freed multiple times in some
> > corner cases. Simplify the code so that this doesn't happen. In fact,
> > the body of free_field_queue restores the original MPlayer code.
> > ---
> > Sorry for not providing a reproducible test case, but for me this
> > happened de to a very weird interaction with my application. I
> > suspect this can happen when initializing the filter, but not
> > filtering any frames. But I didn't attempt to confirm this.
> > ---
> >  libavfilter/vf_pullup.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/libavfilter/vf_pullup.c b/libavfilter/vf_pullup.c
> > index 58d4d7a..9a3fc05 100644
> > --- a/libavfilter/vf_pullup.c
> > +++ b/libavfilter/vf_pullup.c
> > @@ -126,20 +126,18 @@ static int alloc_metrics(PullupContext *s, PullupField *f)
> >      return 0;
> >  }
> >  
> > -static void free_field_queue(PullupField *head, PullupField **last)
> > +static void free_field_queue(PullupField *head)
> >  {
> >      PullupField *f = head;
> > -    while (f) {
> > +    do {
> > +        if (!f)
> > +            break;
> >          av_free(f->diffs);
> >          av_free(f->combs);
> >          av_free(f->vars);
> > -        if (f == *last) {
> > -            av_freep(last);
> > -            break;
> > -        }
> >          f = f->next;
> > -        av_freep(&f->prev);
> > -    };
> 
> > +        av_free(f->prev);
> 
> this will read freed data
> its a ring, theres no way to free the last element by taking a
> pointer from inside another element of the ring as there is no
> other element for the last one

Does that mean the mplayer code was broken in the first place? I guess
that's why it was different.

> > +    } while (f != head);
> >  }
> >  
> >  static PullupField *make_field_queue(PullupContext *s, int len)
> > @@ -158,14 +156,14 @@ static PullupField *make_field_queue(PullupContext *s, int len)
> >      for (; len > 0; len--) {
> >          f->next = av_mallocz(sizeof(*f->next));
> >          if (!f->next) {
> > -            free_field_queue(head, &f);
> > +            free_field_queue(head);
> 
> this will segfault as f->next is used before checking its null in
> free_field_queue
> 
> [...]

Attached another patch, mostly try&error style.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vf_pullup-simplify-fix-double-free-error.patch
Type: text/x-patch
Size: 2177 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140325/ac37f5cc/attachment.bin>