diff mbox series

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

Message ID 20210321101518.10920-1-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/2] lavc/pngdec: fix exporting frame metadata after 5663301560
Related show

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

Anton Khirnov March 21, 2021, 10:15 a.m. UTC
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@gmail.com>
---
 libavcodec/pngdec.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

Comments

Andreas Rheinhardt March 21, 2021, 10:28 a.m. UTC | #1
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@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;
>  }
>  
> 
There is more metadata than just AVFrame.metadata, namely side data.
fate-suite/png1/lena-int_rgb24.png contains both an ICC profile as well
as mastering display metadata which are no longer exported. I don't
think your patch will fix that. I don't think one can avoid a complete
spare packet just for this stuff.

- Andreas
Carl Eugen Hoyos March 21, 2021, 10:52 a.m. UTC | #2
Am So., 21. März 2021 um 11:15 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
>
> 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

Ticket #8972 is definitely reproducible with versions older than 5663301560

Carl Eugen
diff mbox series

Patch

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;
 }