[FFmpeg-cvslog] avcodec/mpegpicture: Don't copy unnecessarily, fix race

[Andreas Rheinhardt]

ffmpeg | branch: master | Andreas Rheinhardt <andreas.rheinhardt at outlook.com> | Mon Aug  8 10:29:05 2022 +0200| [b645138a34321fb1d1b7988cd0d78b897e4d65ca] | committer: Andreas Rheinhardt

avcodec/mpegpicture: Don't copy unnecessarily, fix race

mpegvideo uses an array of Pictures and when it is done with using
them, it only unreferences them incompletely: Some buffers are kept
so that they can be reused lateron if the same slot in the Picture
array is reused, making this a sort of a bufferpool.
(Basically, a Picture is considered used if the AVFrame's buf is set.)
Yet given that other pieces of the decoder may have a reference to
these buffers, they need not be writable and are made writable using
av_buffer_make_writable() when preparing a new Picture. This involves
reading the buffer's data, although the old content of the buffer
need not be retained.

Worse, this read can be racy, because the buffer can be used by another
thread at the same time. This happens for Real Video 3 and 4.

This commit fixes this race by no longer copying the data;
instead the old buffer is replaced by a new, zero-allocated buffer.

(Here are the details of what happens with three or more decoding threads
when decoding rv30.rm from the FATE-suite as happens in the rv30 test:
The first decoding thread uses the first slot of its picture array
to store its current pic; update_thread_context copies this for the
second thread that decodes a P-frame. It uses the second slot in its
Picture array to store its P-frame. This arrangement is then copied
to the third decode thread, which decodes a B-frame. It uses the third
slot in its Picture array for its current frame.
update_thread_context copies this to the next thread. It unreferences
the third slot containing the other B-frame and then it reuses this
slot for its current frame. Because the pic array slots are only
incompletely unreferenced, the buffers of the previous B-frame are
still in there and they are not writable; in fact the previous
thread is concurrently writing to them, causing races when making
the buffer writable.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>

> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=b645138a34321fb1d1b7988cd0d78b897e4d65ca
---

 libavcodec/mpegpicture.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mpegpicture.c b/libavcodec/mpegpicture.c
index c57f149752..711ce35f9d 100644
--- a/libavcodec/mpegpicture.c
+++ b/libavcodec/mpegpicture.c
@@ -47,11 +47,25 @@ static void av_noinline free_picture_tables(Picture *pic)
     }
 }
 
+static int make_table_writable(AVBufferRef **ref)
+{
+    AVBufferRef *old = *ref, *new;
+
+    if (av_buffer_is_writable(old))
+        return 0;
+    new = av_buffer_allocz(old->size);
+    if (!new)
+        return AVERROR(ENOMEM);
+    av_buffer_unref(ref);
+    *ref = new;
+    return 0;
+}
+
 static int make_tables_writable(Picture *pic)
 {
 #define MAKE_WRITABLE(table) \
 do {\
-    int ret = av_buffer_make_writable(&pic->table); \
+    int ret = make_table_writable(&pic->table); \
     if (ret < 0) \
         return ret; \
 } while (0)