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

[Aman Gupta]

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);
     }
 
 #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)