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

[Andreas Rheinhardt]

Mattias Wadman:
> On Thu, Apr 23, 2020 at 5:15 PM Mattias Wadman <mattias.wadman at gmail.com> wrote:
>>
>> lavf flacenc could previously write truncated metadata picture size if
>> the picture data did 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.
>>
>> Also only enable this workaround flac files and not ogg files with flac
>> METADATA_BLOCK_PICTURE comment.
>>
>> flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
>> before the fix a broken flac 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   | 27 ++++++++++++++++++++++++---
>>  libavformat/flac_picture.h   |  2 +-
>>  libavformat/flacdec.c        |  2 +-
>>  libavformat/oggparsevorbis.c |  2 +-
>>  4 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
>> index 81ddf80465..f59ec8843f 100644
>> --- a/libavformat/flac_picture.c
>> +++ b/libavformat/flac_picture.c
>> @@ -27,16 +27,17 @@
>>  #include "id3v2.h"
>>  #include "internal.h"
>>
>> -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)
>>  {
>>      const CodecMime *mime = ff_id3v2_mime_tags;
>>      enum AVCodecID id = AV_CODEC_ID_NONE;
>>      AVBufferRef *data = NULL;
>> -    uint8_t mimetype[64], *desc = NULL;
>> +    uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
>>      GetByteContext g;
>>      AVStream *st;
>>      int width, height, ret = 0;
>> -    unsigned int len, type;
>> +    unsigned int type;
>> +    uint32_t len, left;
>>
>>      if (buf_size < 34) {
>>          av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
>> @@ -114,6 +115,25 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
>>
>>      /* picture data */
>>      len = bytestream2_get_be32u(&g);
>> +
>> +    // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if
>> +    // picture size did not fit in 24 bits
>> +    left = bytestream2_get_bytes_left(&g);
>> +    if (truncate_workaround && len > left && (len & 0xffffff) == left) {
>> +        uint32_t lendiff;
>> +
>> +        av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\n", left, len);
>> +
>> +        picturebuf = av_malloc(len);
>> +        if (!picturebuf)
>> +            RETURN_ERROR(AVERROR(ENOMEM));
>> +        lendiff = len - left;
>> +        bytestream2_get_buffer(&g, picturebuf, left);
>> +        if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
>> +            RETURN_ERROR(AVERROR_INVALIDDATA);
>> +        bytestream2_init(&g, picturebuf, len);
>> +    }
>> +
>>      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)
>> @@ -155,6 +175,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
>>  fail:
>>      av_buffer_unref(&data);
>>      av_freep(&desc);
>> +    av_freep(&picturebuf);
>>
>>      return ret;
>>  }
>> 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
>>
> 
> Trying to find what is the convention for send patch updates. Use git
> send-email and manually edit and add version to subject?
> 
> -Mattias

git send-email accepts options for git format-patch and the latter has
an option for a version field: reroll-count.

- Andreas