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

[Reimar Döffinger]

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.