[FFmpeg-devel] [FFmpeg-cvslog] vp8: always update next_framep[] before returning from decode_frame().

[Reimar Döffinger]

On Sun, Feb 19, 2012 at 08:40:14AM +0100, Reimar Döffinger wrote:
> On 9 Feb 2012, at 22:15, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> > On Thu, Feb 09, 2012 at 11:57:28AM -0800, Ronald S. Bultje wrote:
> >> On Thu, Feb 9, 2012 at 11:49 AM, Reimar Döffinger
> >> <Reimar.Doeffinger at gmx.de> wrote:
> >>> I find that attribution a bit strange when I sent a patch for this
> >>> issue already on Dec 11.
> >> 
> >> Not to me. The issue was disclosed to me by these people, therefore
> >> they get attribution for disclosing the issue to me.
> > 
> > Sorry, I had assumed you were still reading over at least one of
> > the FFmpeg lists.
> > 
> >>> That patch also fixed a few other issues, for example that failure
> >>> to allocate a frame would not update the reference frames according
> >>> to header data.
> >>> I am not completely sure but I also think that your patch might
> >>> still leave a frame that was freed in the reference frame list.
> >> 
> >> I don't think that's possible. A free()'ed frame is either by
> >> definition not referenced (that's the condition under which it is
> >> free()'ed in vp8_decode_frame()), or it is allocated-but-not-decoded.
> >> That second, however, is not possible, since there is no return or
> >> goto err statement after vp8_alloc_frame() in vp8_decode_frame(),
> >> except for the return at the end of the function.
> > 
> > The path I see is:
> > curframe = s->framep[VP56_FRAME_CURRENT] = &s->frames[i];
> > [...]
> >    if (curframe->data[0])
> >            vp8_release_frame(s, curframe, 1, 0);
> > [...]
> >    if (!s->keyframe && (!s->framep[VP56_FRAME_PREVIOUS] ||
> >                         !s->framep[VP56_FRAME_GOLDEN] ||
> >                         !s->framep[VP56_FRAME_GOLDEN2])) {
> >        av_log(avctx, AV_LOG_WARNING, "Discarding interframe without a prior keyframe!\n");
> >        ret = AVERROR_INVALIDDATA;
> >        goto err;
> > 
> > [decode done for this frame]
> > 
> >    prev_frame = s->framep[VP56_FRAME_CURRENT];
> > [prev_frame not NULL, but freed]
> >        if (prev_frame && s->segmentation.enabled && !s->segmentation.update_map)
> >            ff_thread_await_progress(prev_frame, mb_y, 0);
> > 
> > In addition, framep not being updated on allocation error I think leads to
> > the code thinking that all reference frames are available even though
> > it is only the reference frames for the _previous_ frame are available.
> 
> Could someone please check my logic?
> Ronald stopped responding before before an explanation why this scenario can't happen that made sense to me, and this is at least a DoS bug for multithreading I think.

Ping