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

Christ-Jan Wijtmans cj.wijtmans at gmail.com
Tue Mar 25 17:09:15 CET 2014


*unsubscribe cj.wijtmans at gmail.com <cj.wijtmans at gmail.com>*


Live long and prosper,

Christ-Jan Wijtmans
https://github.com/cjwijtmans
http://facebook.com/cj.wijtmans
http://twitter.com/cjwijtmans


On Tue, Mar 25, 2014 at 4:56 PM, Michael Niedermayer <michaelni at gmx.at>wrote:

> 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.
>
> probably
> i dont remembetr exactly
>
>
> >
> > > > +    } 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
> >
> > The memory allocation for f->diffs was freed multiple times in some
> > corner cases. Simplify the code so that this doesn't happen.
> > ---
> >  libavfilter/vf_pullup.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/libavfilter/vf_pullup.c b/libavfilter/vf_pullup.c
> > index 58d4d7a..bf84568 100644
> > --- a/libavfilter/vf_pullup.c
> > +++ b/libavfilter/vf_pullup.c
> > @@ -126,20 +126,20 @@ 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 {
> > +        PullupField *next;
> > +        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);
> > -    };
> > +        next = f->next;
> > +        av_free(f);
> > +        f = next;
> > +    } while (f != head);
> >  }
>
> i would suggest to set freed pointers to NULL, this should make it
> more robust
> also might mke the != head check unneeded
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I am the wisest man alive, for I know one thing, and that is that I know
> nothing. -- Socrates
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>


More information about the ffmpeg-devel mailing list