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

Michael Niedermayer michaelni at gmx.at
Tue Mar 25 16:56:47 CET 2014


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
-------------- 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/20140325/4b440c25/attachment.asc>


More information about the ffmpeg-devel mailing list