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

[Michael Niedermayer]

On Tue, Mar 25, 2014 at 03:29:30PM +0100, wm4 wrote:
> 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.

>  vf_pullup.c |   25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 928e08618ac34156863971786aed2dc94408e255  0001-vf_pullup-simplify-fix-double-free-error.patch
> From ed0c5920c899752e8797ccbee84d96133be601d0 Mon Sep 17 00:00:00 2001
> From: wm4 <nfxjfg at googlemail.com>
> Date: Tue, 25 Mar 2014 13:53:11 +0100
> Subject: [PATCH] vf_pullup: simplify, fix double free error

applied

thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140326/641e7f24/attachment.asc>