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 |
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 |
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 --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);
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(-)