diff mbox series

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

Message ID 20200423151522.59081-1-mattias.wadman@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] libavformat/flacdec: Workaround for truncated metadata picture size | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Mattias Wadman April 23, 2020, 3:15 p.m. UTC 
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(-)

Comments

Mattias Wadman April 23, 2020, 3:18 p.m. UTC | #1 
On Thu, Apr 23, 2020 at 5:15 PM Mattias Wadman <mattias.wadman@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
Andreas Rheinhardt April 23, 2020, 4:18 p.m. UTC | #2 
Mattias Wadman:
> On Thu, Apr 23, 2020 at 5:15 PM Mattias Wadman <mattias.wadman@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
Andreas Rheinhardt April 24, 2020, 5:29 a.m. UTC | #3 
Mattias Wadman:
> 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);

I don't like that you are allocating a very big buffer and read data
into this buffer just to copy it into another buffer (that is not given
in advance, but allocated by this function, too).

- Andreas

>  
>      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");
>
Mattias Wadman April 24, 2020, 8:18 a.m. UTC | #4 
On Fri, Apr 24, 2020 at 7:29 AM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Mattias Wadman:
> > 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);
>
> I don't like that you are allocating a very big buffer and read data
> into this buffer just to copy it into another buffer (that is not given
> in advance, but allocated by this function, too).

Ok thanks, yeap not so nice. Guess there are some alternatives i come up with:

1) Probe for the issue in flac_read_header and if detected realloc and
read more then call unmodified ff_flac_parse_picture
2) Make ff_flac_parse_picture able to say it needs more data
3) Make ff_flac_parse_picture able to realloc the passed buf, so
change buf arg to uint8_t **buf

-Mattias

>
> - Andreas
>
> >
> >      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");
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt April 24, 2020, 9:26 a.m. UTC | #5 
Mattias Wadman:
> On Fri, Apr 24, 2020 at 7:29 AM Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com> wrote:
>>
>> Mattias Wadman:
>>> 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);
>>
>> I don't like that you are allocating a very big buffer and read data
>> into this buffer just to copy it into another buffer (that is not given
>> in advance, but allocated by this function, too).
> 
> Ok thanks, yeap not so nice. Guess there are some alternatives i come up with:
> 
> 1) Probe for the issue in flac_read_header and if detected realloc and
> read more then call unmodified ff_flac_parse_picture
> 2) Make ff_flac_parse_picture able to say it needs more data
> 3) Make ff_flac_parse_picture able to realloc the passed buf, so
> change buf arg to uint8_t **buf
> 
This would be way to intrusive. I actually had something else in mind:
You directly allocate a buffer of the right size (with padding at the
end), copy the needed part of the data that is given to you via buf into
it and read the stuff you need to read into the new buffer (at the right
offset). This does not introduce a new allocation.

- Andreas
Mattias Wadman April 24, 2020, 10:33 a.m. UTC | #6 
On Fri, Apr 24, 2020 at 11:26 AM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Mattias Wadman:
> > On Fri, Apr 24, 2020 at 7:29 AM Andreas Rheinhardt
> > <andreas.rheinhardt@gmail.com> wrote:
> >>
> >> Mattias Wadman:
> >>> 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);
> >>
> >> I don't like that you are allocating a very big buffer and read data
> >> into this buffer just to copy it into another buffer (that is not given
> >> in advance, but allocated by this function, too).
> >
> > Ok thanks, yeap not so nice. Guess there are some alternatives i come up with:
> >
> > 1) Probe for the issue in flac_read_header and if detected realloc and
> > read more then call unmodified ff_flac_parse_picture
> > 2) Make ff_flac_parse_picture able to say it needs more data
> > 3) Make ff_flac_parse_picture able to realloc the passed buf, so
> > change buf arg to uint8_t **buf
> >
> This would be way to intrusive. I actually had something else in mind:
> You directly allocate a buffer of the right size (with padding at the
> end), copy the needed part of the data that is given to you via buf into
> it and read the stuff you need to read into the new buffer (at the right
> offset). This does not introduce a new allocation.

Yes, much better. Didn't see that if we don't fail on too short we would already
allocate the correct size and just need to read some more.

Patch update coming in a minute.

>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

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");