[FFmpeg-devel] [PATCH] avcodec/h264, videotoolbox: fix use-after-free of AVFrame buffer when VT decoder fails

[Aman Gupta]

On Mon, Feb 13, 2017 at 6:04 PM, Aman Gupta <ffmpeg at tmm1.net> wrote:

> From: Aman Gupta <aman at tmm1.net>
>
> The videotoolbox hwaccel returns a dummy frame with a 1-byte buffer from
> alloc_frame(). In end_frame(), this buffer is replaced with the actual
> decoded data from VTDecompressionSessionDecodeFrame(). This is hacky,
> but works well enough, as long as the VT decoder returns a valid frame on
> every end_frame() call.
>
> In the case of errors, things get more interesting. Before
> 9747219958060d8c4f697df62e7f172c2a77e6c7, the dummy 1-byte frame would
> accidentally be returned all the way up to the user. After that commit,
> the dummy frame was properly unref'd so the user received an error.
>
> However, since that commit, VT hwaccel failures started causing random
> segfaults in the h264 decoder. This happened more often on iOS where the
> VT implementation is more likely to throw errors on bitstream anomolies.
> A recent report of this issue can be see in
> http://ffmpeg.org/pipermail/libav-user/2016-November/009831.html
>
> The root cause here is that between the calls to alloc_frame() and
> end_frame(), the h264 decoder will reference the dummy 1-byte frame in
> its ref_list. When the end_frame() call fails, the dummy frame is
> unref'd but still referenced in the h264 decoder. This eventually causes
> a NULL deference and segmentation fault.
>
> This patch fixes the issue by properly discarding references to the
> dummy frame in the H264Context after the frame has been unref'd.
> ---
>  libavcodec/h264_picture.c |  3 +++
>  libavcodec/h264_refs.c    | 20 ++++++++++++++++++--
>  libavcodec/h264dec.h      |  1 +
>  3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c
> index f634d2a..702ca12 100644
> --- a/libavcodec/h264_picture.c
> +++ b/libavcodec/h264_picture.c
> @@ -177,6 +177,9 @@ int ff_h264_field_end(H264Context *h, H264SliceContext
> *sl, int in_setup)
>          if (err < 0)
>              av_log(avctx, AV_LOG_ERROR,
>                     "hardware accelerator failed to decode picture\n");
> +
> +        if (err < 0 && avctx->hwaccel->pix_fmt == AV_PIX_FMT_VIDEOTOOLBOX)
> +            ff_h264_remove_cur_pic_ref(h);
>

This patch fixed the crash I was seeing pretty often, but now another
assert() can trip (albeit less frequently):

Assertion h->cur_pic_ptr->f->buf[0] failed at
src/libavcodec/h264_slice.c:1340

I will try wm4's suggestion of leaving the dummy 1-byte frame in-place, but
ignoring it when it's time to return a frame to the user.


>      }
>
>  #if FF_API_CAP_VDPAU
> diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
> index 97bf588..9c77bc7 100644
> --- a/libavcodec/h264_refs.c
> +++ b/libavcodec/h264_refs.c
> @@ -560,6 +560,23 @@ static H264Picture *remove_long(H264Context *h, int
> i, int ref_mask)
>      return pic;
>  }
>
> +void ff_h264_remove_cur_pic_ref(H264Context *h)
> +{
> +    int j;
> +
> +    if (h->short_ref[0] == h->cur_pic_ptr) {
> +        remove_short_at_index(h, 0);
> +    }
> +
> +    if (h->cur_pic_ptr->long_ref) {
> +        for (j = 0; j < FF_ARRAY_ELEMS(h->long_ref); j++) {
> +            if (h->long_ref[j] == h->cur_pic_ptr) {
> +                remove_long(h, j, 0);
> +            }
> +        }
> +    }
> +}
> +
>  void ff_h264_remove_all_refs(H264Context *h)
>  {
>      int i;
> @@ -571,8 +588,7 @@ void ff_h264_remove_all_refs(H264Context *h)
>
>      if (h->short_ref_count && !h->last_pic_for_ec.f->data[0]) {
>          ff_h264_unref_picture(h, &h->last_pic_for_ec);
> -        if (h->short_ref[0]->f->buf[0])
> -            ff_h264_ref_picture(h, &h->last_pic_for_ec, h->short_ref[0]);
> +        ff_h264_ref_picture(h, &h->last_pic_for_ec, h->short_ref[0]);
>      }
>
>      for (i = 0; i < h->short_ref_count; i++) {
> diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h
> index 5f868b7..063afed 100644
> --- a/libavcodec/h264dec.h
> +++ b/libavcodec/h264dec.h
> @@ -569,6 +569,7 @@ int ff_h264_alloc_tables(H264Context *h);
>  int ff_h264_decode_ref_pic_list_reordering(H264SliceContext *sl, void
> *logctx);
>  int ff_h264_build_ref_list(H264Context *h, H264SliceContext *sl);
>  void ff_h264_remove_all_refs(H264Context *h);
> +void ff_h264_remove_cur_pic_ref(H264Context *h);
>
>  /**
>   * Execute the reference picture marking (memory management control
> operations).
> --
> 2.10.1 (Apple Git-78)
>
>