[FFmpeg-devel] [PATCH 1/2] lavc/pngdec: fix exporting frame metadata after 5663301560

[Anton Khirnov]

Also avoid a potential race with frame threading, where a frame's
metadata could be modified concurrently with that frame being passed to
another thread.

Fixes #8972

Found-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
---
 libavcodec/pngdec.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index a5a71ef161..00fabec34c 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -57,6 +57,8 @@ typedef struct PNGDecContext {
     ThreadFrame last_picture;
     ThreadFrame picture;
 
+    AVDictionary *frame_metadata;
+
     enum PNGHeaderState hdr_state;
     enum PNGImageState pic_state;
     int width, height;
@@ -509,8 +511,7 @@ static uint8_t *iso88591_to_utf8(const uint8_t *in, size_t size_in)
     return out;
 }
 
-static int decode_text_chunk(PNGDecContext *s, uint32_t length, int compressed,
-                             AVDictionary **dict)
+static int decode_text_chunk(PNGDecContext *s, uint32_t length, int compressed)
 {
     int ret, method;
     const uint8_t *data        = s->gb.buffer;
@@ -552,7 +553,7 @@ static int decode_text_chunk(PNGDecContext *s, uint32_t length, int compressed,
         return AVERROR(ENOMEM);
     }
 
-    av_dict_set(dict, kw_utf8, txt_utf8,
+    av_dict_set(&s->frame_metadata, kw_utf8, txt_utf8,
                 AV_DICT_DONT_STRDUP_KEY | AV_DICT_DONT_STRDUP_VAL);
     return 0;
 }
@@ -710,6 +711,8 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
             s->bpp += byte_depth;
         }
 
+        av_dict_free(&s->frame_metadata);
+
         ff_thread_release_buffer(avctx, &s->picture);
         if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0)
             return ret;
@@ -1182,7 +1185,6 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
                                AVFrame *p, const AVPacket *avpkt)
 {
     const AVCRC *crc_tab = av_crc_get_table(AV_CRC_32_IEEE_LE);
-    AVDictionary **metadatap = NULL;
     uint32_t tag, length;
     int decode_next_dat = 0;
     int i, ret;
@@ -1250,7 +1252,6 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
             }
         }
 
-        metadatap = &p->metadata;
         switch (tag) {
         case MKTAG('I', 'H', 'D', 'R'):
             if ((ret = decode_ihdr_chunk(avctx, s, length)) < 0)
@@ -1292,12 +1293,12 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
                 goto skip_tag;
             break;
         case MKTAG('t', 'E', 'X', 't'):
-            if (decode_text_chunk(s, length, 0, metadatap) < 0)
+            if (decode_text_chunk(s, length, 0) < 0)
                 av_log(avctx, AV_LOG_WARNING, "Broken tEXt chunk\n");
             bytestream2_skip(&s->gb, length + 4);
             break;
         case MKTAG('z', 'T', 'X', 't'):
-            if (decode_text_chunk(s, length, 1, metadatap) < 0)
+            if (decode_text_chunk(s, length, 1) < 0)
                 av_log(avctx, AV_LOG_WARNING, "Broken zTXt chunk\n");
             bytestream2_skip(&s->gb, length + 4);
             break;
@@ -1355,7 +1356,7 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
             if (ret < 0)
                 return ret;
 
-            av_dict_set(&p->metadata, "gamma", gamma_str, AV_DICT_DONT_STRDUP_VAL);
+            av_dict_set(&s->frame_metadata, "gamma", gamma_str, AV_DICT_DONT_STRDUP_VAL);
 
             bytestream2_skip(&s->gb, 4); /* crc */
             break;
@@ -1466,6 +1467,7 @@ static int decode_frame_png(AVCodecContext *avctx,
     PNGDecContext *const s = avctx->priv_data;
     const uint8_t *buf     = avpkt->data;
     int buf_size           = avpkt->size;
+    AVFrame     *dst_frame = data;
     AVFrame *p = s->picture.f;
     int64_t sig;
     int ret;
@@ -1503,9 +1505,11 @@ static int decode_frame_png(AVCodecContext *avctx,
         goto the_end;
     }
 
-    if ((ret = av_frame_ref(data, s->picture.f)) < 0)
+    if ((ret = av_frame_ref(dst_frame, s->picture.f)) < 0)
         goto the_end;
 
+    FFSWAP(AVDictionary*, dst_frame->metadata, s->frame_metadata);
+
     if (!(avctx->active_thread_type & FF_THREAD_FRAME)) {
         ff_thread_release_buffer(avctx, &s->last_picture);
         FFSWAP(ThreadFrame, s->picture, s->last_picture);
@@ -1527,6 +1531,7 @@ static int decode_frame_apng(AVCodecContext *avctx,
                         AVPacket *avpkt)
 {
     PNGDecContext *const s = avctx->priv_data;
+    AVFrame     *dst_frame = data;
     int ret;
     AVFrame *p = s->picture.f;
 
@@ -1564,6 +1569,8 @@ static int decode_frame_apng(AVCodecContext *avctx,
     if ((ret = av_frame_ref(data, s->picture.f)) < 0)
         goto end;
 
+    FFSWAP(AVDictionary*, dst_frame->metadata, s->frame_metadata);
+
     if (!(avctx->active_thread_type & FF_THREAD_FRAME)) {
         if (s->dispose_op == APNG_DISPOSE_OP_PREVIOUS) {
             ff_thread_release_buffer(avctx, &s->picture);
@@ -1665,6 +1672,8 @@ static av_cold int png_dec_end(AVCodecContext *avctx)
     av_freep(&s->tmp_row);
     s->tmp_row_size = 0;
 
+    av_dict_free(&s->frame_metadata);
+
     return 0;
 }
 
-- 
2.30.1