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

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

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

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