[FFmpeg-devel] [PATCH v5] libavformat/flacdec: Workaround for truncated metadata picture size

Mattias Wadman mattias.wadman at gmail.com
Mon May 25 12:10:22 EEST 2020


Hello, ok to merge?

On Tue, May 19, 2020 at 11:27 AM Mattias Wadman
<mattias.wadman at gmail.com> wrote:
>
> Some flac muxers write truncated metadata picture size if the picture
> data do not fit in 24 bits. Detect this by truncting the size found inside
> the picture block and if it matches the block size use it and read rest
> of picture data.
>
> This workaround is only for flac files and not ogg files with flac
> METADATA_BLOCK_PICTURE comments and it can be disabled with strict level
> above normal. Currently there is a 500MB limit on truncate size to protect
> from large memory allocations.
>
> The truncation bug in lavf flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> but based on existing broken files other unknown flac muxers seems to truncate also.
> Before the fix a broken flac file for reproduction could be generated with:
> ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
>
> Fixes ticket 6333
> ---
>  libavformat/flac_picture.c   | 47 ++++++++++++++++++++++++++++++------
>  libavformat/flac_picture.h   |  2 +-
>  libavformat/flacdec.c        |  2 +-
>  libavformat/oggparsevorbis.c |  2 +-
>  4 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index 81ddf80465..53e24b28b7 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -27,7 +27,9 @@
>  #include "id3v2.h"
>  #include "internal.h"
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> +#define MAX_TRUNC_PICTURE_SIZE (500 * 1024 * 1024)
> +
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int truncate_workaround)
>  {
>      const CodecMime *mime = ff_id3v2_mime_tags;
>      enum AVCodecID id = AV_CODEC_ID_NONE;
> @@ -36,7 +38,8 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
>      GetByteContext g;
>      AVStream *st;
>      int width, height, ret = 0;
> -    unsigned int len, type;
> +    unsigned int type;
> +    uint32_t len, left, trunclen = 0;
>
>      if (buf_size < 34) {
>          av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
> @@ -114,16 +117,44 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
>
>      /* picture data */
>      len = bytestream2_get_be32u(&g);
> -    if (len <= 0 || len > bytestream2_get_bytes_left(&g)) {
> -        av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
> -        if (s->error_recognition & AV_EF_EXPLODE)
> -            ret = AVERROR_INVALIDDATA;
> -        goto fail;
> +
> +    left = bytestream2_get_bytes_left(&g);
> +    if (len <= 0 || len > left) {
> +        if (len > MAX_TRUNC_PICTURE_SIZE || len >= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) {
> +            av_log(s, AV_LOG_ERROR, "Attached picture metadata block too big %u\n", len);
> +            if (s->error_recognition & AV_EF_EXPLODE)
> +                ret = AVERROR_INVALIDDATA;
> +            goto fail;
> +        }
> +
> +        // Workaround bug for flac muxers that writs truncated metadata picture block size if
> +        // the picture size do not fit in 24 bits. lavf flacenc used to have the issue and based
> +        // on existing broken files other unknown flac muxers seems to truncate also.
> +        if (truncate_workaround &&
> +            s->strict_std_compliance <= FF_COMPLIANCE_NORMAL &&
> +            len > left && (len & 0xffffff) == left) {
> +            av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %u to %u\n", left, len);
> +            trunclen = len - left;
> +        } else {
> +            av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
> +            if (s->error_recognition & AV_EF_EXPLODE)
> +                ret = AVERROR_INVALIDDATA;
> +            goto fail;
> +        }
>      }
>      if (!(data = av_buffer_alloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) {
>          RETURN_ERROR(AVERROR(ENOMEM));
>      }
> -    bytestream2_get_bufferu(&g, data->data, len);
> +
> +    if (trunclen == 0) {
> +        bytestream2_get_bufferu(&g, data->data, len);
> +    } else {
> +        // If truncation was detected copy all data from block and read missing bytes
> +        // not included in the block size
> +        bytestream2_get_bufferu(&g, data->data, left);
> +        if (avio_read(s->pb, data->data + len - trunclen, trunclen) < trunclen)
> +            RETURN_ERROR(AVERROR_INVALIDDATA);
> +    }
>      memset(data->data + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>
>      if (AV_RB64(data->data) == PNGSIG)
> diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
> index 4374b6f4f6..61fd0c8806 100644
> --- a/libavformat/flac_picture.h
> +++ b/libavformat/flac_picture.h
> @@ -26,6 +26,6 @@
>
>  #define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
>
> -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
> +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int truncate_workaround);
>
>  #endif /* AVFORMAT_FLAC_PICTURE_H */
> diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
> index cb516fb1f3..79c05f14bf 100644
> --- a/libavformat/flacdec.c
> +++ b/libavformat/flacdec.c
> @@ -146,7 +146,7 @@ static int flac_read_header(AVFormatContext *s)
>              }
>              av_freep(&buffer);
>          } else if (metadata_type == FLAC_METADATA_TYPE_PICTURE) {
> -            ret = ff_flac_parse_picture(s, buffer, metadata_size);
> +            ret = ff_flac_parse_picture(s, buffer, metadata_size, 1);
>              av_freep(&buffer);
>              if (ret < 0) {
>                  av_log(s, AV_LOG_ERROR, "Error parsing attached picture.\n");
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index 27d2c686b6..6f15204ada 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -165,7 +165,7 @@ int ff_vorbis_comment(AVFormatContext *as, AVDictionary **m,
>                  av_freep(&tt);
>                  av_freep(&ct);
>                  if (ret > 0)
> -                    ret = ff_flac_parse_picture(as, pict, ret);
> +                    ret = ff_flac_parse_picture(as, pict, ret, 0);
>                  av_freep(&pict);
>                  if (ret < 0) {
>                      av_log(as, AV_LOG_WARNING, "Failed to parse cover art block.\n");
> --
> 2.18.0
>


More information about the ffmpeg-devel mailing list