diff mbox series

[FFmpeg-devel,05/14] avcodec/ffv1dec: Fix data races emanating from copying whole context

Message ID HE1PR0301MB2154567A5CB50DB0FBB82C618F449@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit 0cfc7418bbf55f7b316862598c4e59434c65b3a8
Headers show
Series [FFmpeg-devel,01/14] avcodec/ffv1dec: Remove redundant writes, fix races | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 24, 2021, 11:14 a.m. UTC 
When using frame threading, the FFV1 decoder's update_thread_context()
function copies the whole context and afterwards restores some allocated
fields with backups made earlier. Among these fields are the
ThreadFrames and the source context's ThreadFrames can change
concurrently without any synchronization, leading to data races which
are undefined behaviour even if they don't lead to problems in
practice (as the destination's own ThreadFrames are restored directly
thereafter).

Fix this by only copying the actually needed fields.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ffv1dec.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

Comments

Michael Niedermayer April 25, 2021, 12:21 p.m. UTC | #1 
On Sat, Apr 24, 2021 at 01:14:37PM +0200, Andreas Rheinhardt wrote:
> When using frame threading, the FFV1 decoder's update_thread_context()
> function copies the whole context and afterwards restores some allocated
> fields with backups made earlier. Among these fields are the
> ThreadFrames and the source context's ThreadFrames can change
> concurrently without any synchronization, leading to data races which
> are undefined behaviour even if they don't lead to problems in
> practice (as the destination's own ThreadFrames are restored directly
> thereafter).
> 
> Fix this by only copying the actually needed fields.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/ffv1dec.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)

LGTM

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index c08ec5c1b7..9a9ee10a4c 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -1031,18 +1031,12 @@  static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
     if (dst == src)
         return 0;
 
-    {
-        ThreadFrame picture = fdst->picture, last_picture = fdst->last_picture;
-        uint8_t (*initial_states[MAX_QUANT_TABLES])[32];
-        struct FFV1Context *slice_context[MAX_SLICES];
-        memcpy(initial_states, fdst->initial_states, sizeof(fdst->initial_states));
-        memcpy(slice_context,  fdst->slice_context , sizeof(fdst->slice_context));
-
-        memcpy(fdst, fsrc, sizeof(*fdst));
-        memcpy(fdst->initial_states, initial_states, sizeof(fdst->initial_states));
-        memcpy(fdst->slice_context,  slice_context , sizeof(fdst->slice_context));
-        fdst->picture      = picture;
-        fdst->last_picture = last_picture;
+    copy_fields(fdst, fsrc, fsrc);
+    fdst->use32bit     = fsrc->use32bit;
+    memcpy(fdst->state_transition, fsrc->state_transition,
+           sizeof(fdst->state_transition));
+    memcpy(fdst->quant_table, fsrc->quant_table, sizeof(fsrc->quant_table));
+
         for (i = 0; i<fdst->num_h_slices * fdst->num_v_slices; i++) {
             FFV1Context *fssrc = fsrc->slice_context[i];
             FFV1Context *fsdst = fdst->slice_context[i];
@@ -1050,7 +1044,6 @@  static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
         }
         av_assert0(!fdst->plane[0].state);
         av_assert0(!fdst->sample_buffer);
-    }
 
     av_assert1(fdst->max_slice_count == fsrc->max_slice_count);