[FFmpeg-devel] [PATCH 33/33] avcodec/mpeg4videodec: Move use_intra_dc_vlc to stack, fix data race

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

use_intra_dc_vlc is currently kept in sync between frame threads
in mpeg4_update_thread_context(), yet it is set when decoding
blocks, i.e. after ff_thread_finish_setup(). This is a data race
and therefore undefined behaviour.

This race can be fixed easily by moving the variable from the context
to the stack: use_intra_dc_vlc is only read in
mpeg4_decode_block() and only if one is decoding an intra block.
There are three callsites for this function: One in
mpeg4_decode_partitioned_mb() which always sets use_intra_dc_vlc
before the call and two in mpeg4_decode_mb(). One of these callsites
is for intra blocks and use_intra_dc_vlc is set before it;
the last callsite is for non-intra blocks, where use_intra_dc_vlc
is ignored. So if it is used, it always uses a new value and can
therefore be moved to the stack.

The above also explains why this data race did not lead to
FATE-test failures.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mpeg4video.h    |  1 -
 libavcodec/mpeg4videodec.c | 24 ++++++++++++++----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/libavcodec/mpeg4video.h b/libavcodec/mpeg4video.h
index 9fc79b1a22..14fc5e1396 100644
--- a/libavcodec/mpeg4video.h
+++ b/libavcodec/mpeg4video.h
@@ -94,7 +94,6 @@ typedef struct Mpeg4DecContext {
     int new_pred;
     int enhancement_type;
     int scalability;
-    int use_intra_dc_vlc;
 
     /// QP above which the ac VLC should be used for intra dc
     int intra_dc_threshold;
diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
index b8118ff2d2..2aea845580 100644
--- a/libavcodec/mpeg4videodec.c
+++ b/libavcodec/mpeg4videodec.c
@@ -1105,7 +1105,8 @@ int ff_mpeg4_decode_partitions(Mpeg4DecContext *ctx)
  * @return <0 if an error occurred
  */
 static inline int mpeg4_decode_block(Mpeg4DecContext *ctx, int16_t *block,
-                                     int n, int coded, int intra, int rvlc)
+                                     int n, int coded, int intra,
+                                     int use_intra_dc_vlc, int rvlc)
 {
     MpegEncContext *s = &ctx->m;
     int level, i, last, run, qmul, qadd;
@@ -1117,7 +1118,7 @@ static inline int mpeg4_decode_block(Mpeg4DecContext *ctx, int16_t *block,
     // Note intra & rvlc should be optimized away if this is inlined
 
     if (intra) {
-        if (ctx->use_intra_dc_vlc) {
+        if (use_intra_dc_vlc) {
             /* DC coef */
             if (s->partitioned_frame) {
                 level = s->dc_val[0][s->block_index[n]];
@@ -1357,7 +1358,7 @@ static inline int mpeg4_decode_block(Mpeg4DecContext *ctx, int16_t *block,
 
 not_coded:
     if (intra) {
-        if (!ctx->use_intra_dc_vlc) {
+        if (!use_intra_dc_vlc) {
             block[0] = ff_mpeg4_pred_dc(s, n, block[0], &dc_pred_dir, 0);
 
             i -= i >> 31;  // if (i == -1) i = 0;
@@ -1378,7 +1379,7 @@ not_coded:
 static int mpeg4_decode_partitioned_mb(MpegEncContext *s, int16_t block[6][64])
 {
     Mpeg4DecContext *ctx = s->avctx->priv_data;
-    int cbp, mb_type;
+    int cbp, mb_type, use_intra_dc_vlc;
     const int xy = s->mb_x + s->mb_y * s->mb_stride;
 
     av_assert2(s == (void*)ctx);
@@ -1386,7 +1387,7 @@ static int mpeg4_decode_partitioned_mb(MpegEncContext *s, int16_t block[6][64])
     mb_type = s->current_picture.mb_type[xy];
     cbp     = s->cbp_table[xy];
 
-    ctx->use_intra_dc_vlc = s->qscale < ctx->intra_dc_threshold;
+    use_intra_dc_vlc = s->qscale < ctx->intra_dc_threshold;
 
     if (s->current_picture.qscale_table[xy] != s->qscale)
         ff_set_qscale(s, s->current_picture.qscale_table[xy]);
@@ -1436,7 +1437,8 @@ static int mpeg4_decode_partitioned_mb(MpegEncContext *s, int16_t block[6][64])
         s->bdsp.clear_blocks(s->block[0]);
         /* decode each block */
         for (i = 0; i < 6; i++) {
-            if (mpeg4_decode_block(ctx, block[i], i, cbp & 32, s->mb_intra, ctx->rvlc) < 0) {
+            if (mpeg4_decode_block(ctx, block[i], i, cbp & 32, s->mb_intra,
+                                   use_intra_dc_vlc, ctx->rvlc) < 0) {
                 av_log(s->avctx, AV_LOG_ERROR,
                        "texture corrupted at %d %d %d\n",
                        s->mb_x, s->mb_y, s->mb_intra);
@@ -1763,6 +1765,8 @@ static int mpeg4_decode_mb(MpegEncContext *s, int16_t block[6][64])
         }
         s->current_picture.mb_type[xy] = mb_type;
     } else { /* I-Frame */
+        int use_intra_dc_vlc;
+
         do {
             cbpc = get_vlc2(&s->gb, ff_h263_intra_MCBPC_vlc.table, INTRA_MCBPC_VLC_BITS, 2);
             if (cbpc < 0) {
@@ -1790,7 +1794,7 @@ intra:
         }
         cbp = (cbpc & 3) | (cbpy << 2);
 
-        ctx->use_intra_dc_vlc = s->qscale < ctx->intra_dc_threshold;
+        use_intra_dc_vlc = s->qscale < ctx->intra_dc_threshold;
 
         if (dquant)
             ff_set_qscale(s, s->qscale + quant_tab[get_bits(&s->gb, 2)]);
@@ -1801,7 +1805,8 @@ intra:
         s->bdsp.clear_blocks(s->block[0]);
         /* decode each block */
         for (i = 0; i < 6; i++) {
-            if (mpeg4_decode_block(ctx, block[i], i, cbp & 32, 1, 0) < 0)
+            if (mpeg4_decode_block(ctx, block[i], i, cbp & 32,
+                                   1, use_intra_dc_vlc, 0) < 0)
                 return AVERROR_INVALIDDATA;
             cbp += cbp;
         }
@@ -1810,7 +1815,7 @@ intra:
 
     /* decode each block */
     for (i = 0; i < 6; i++) {
-        if (mpeg4_decode_block(ctx, block[i], i, cbp & 32, 0, 0) < 0)
+        if (mpeg4_decode_block(ctx, block[i], i, cbp & 32, 0, 0, 0) < 0)
             return AVERROR_INVALIDDATA;
         cbp += cbp;
     }
@@ -3529,7 +3534,6 @@ static int mpeg4_update_thread_context(AVCodecContext *dst,
     s->new_pred                  = s1->new_pred;
     s->enhancement_type          = s1->enhancement_type;
     s->scalability               = s1->scalability;
-    s->use_intra_dc_vlc          = s1->use_intra_dc_vlc;
     s->intra_dc_threshold        = s1->intra_dc_threshold;
     s->divx_version              = s1->divx_version;
     s->divx_build                = s1->divx_build;
-- 
2.32.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".