| Message ID | 20200424105437.69082-1-mattias.wadman@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v3] libavformat/flacdec: Workaround for truncated metadata picture size | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Hi again, looks ok? On Fri, Apr 24, 2020 at 12:54 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 | 35 +++++++++++++++++++++++++++-------- > libavformat/flac_picture.h | 2 +- > libavformat/flacdec.c | 2 +- > libavformat/oggparsevorbis.c | 2 +- > 4 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c > index 81ddf80465..61277e9dee 100644 > --- a/libavformat/flac_picture.c > +++ b/libavformat/flac_picture.c > @@ -27,7 +27,7 @@ > #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; > @@ -36,7 +36,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 +115,34 @@ 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) { > + // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if > + // picture size did not fit in 24 bits > + if (truncate_workaround && len > left && (len & 0xffffff) == left) { > + av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\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 detect 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 >
Sorry for nagging, something left to fix? On Mon, Apr 27, 2020 at 10:10 AM Mattias Wadman <mattias.wadman@gmail.com> wrote: > > Hi again, looks ok? > > On Fri, Apr 24, 2020 at 12:54 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 | 35 +++++++++++++++++++++++++++-------- > > libavformat/flac_picture.h | 2 +- > > libavformat/flacdec.c | 2 +- > > libavformat/oggparsevorbis.c | 2 +- > > 4 files changed, 30 insertions(+), 11 deletions(-) > > > > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c > > index 81ddf80465..61277e9dee 100644 > > --- a/libavformat/flac_picture.c > > +++ b/libavformat/flac_picture.c > > @@ -27,7 +27,7 @@ > > #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; > > @@ -36,7 +36,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 +115,34 @@ 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) { > > + // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if > > + // picture size did not fit in 24 bits > > + if (truncate_workaround && len > left && (len & 0xffffff) == left) { > > + av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\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 detect 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 > >
Hi, any chance to get this merged? On Sun, May 3, 2020 at 12:32 PM Mattias Wadman <mattias.wadman@gmail.com> wrote: > > Sorry for nagging, something left to fix? > > On Mon, Apr 27, 2020 at 10:10 AM Mattias Wadman > <mattias.wadman@gmail.com> wrote: > > > > Hi again, looks ok? > > > > On Fri, Apr 24, 2020 at 12:54 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 | 35 +++++++++++++++++++++++++++-------- > > > libavformat/flac_picture.h | 2 +- > > > libavformat/flacdec.c | 2 +- > > > libavformat/oggparsevorbis.c | 2 +- > > > 4 files changed, 30 insertions(+), 11 deletions(-) > > > > > > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c > > > index 81ddf80465..61277e9dee 100644 > > > --- a/libavformat/flac_picture.c > > > +++ b/libavformat/flac_picture.c > > > @@ -27,7 +27,7 @@ > > > #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; > > > @@ -36,7 +36,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 +115,34 @@ 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) { > > > + // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if > > > + // picture size did not fit in 24 bits > > > + if (truncate_workaround && len > left && (len & 0xffffff) == left) { > > > + av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\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 detect 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 > > >
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 | 35 +++++++++++++++++++++++++++-------- > libavformat/flac_picture.h | 2 +- > libavformat/flacdec.c | 2 +- > libavformat/oggparsevorbis.c | 2 +- > 4 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c > index 81ddf80465..61277e9dee 100644 > --- a/libavformat/flac_picture.c > +++ b/libavformat/flac_picture.c > @@ -27,7 +27,7 @@ > #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; > @@ -36,7 +36,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 +115,34 @@ 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) { > + // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if I have not found the typical lavf metadata in the file from #6333, so it seems we are not the only tool that created such out-of-spec files. This should be reflected in the comment. > + // picture size did not fit in 24 bits > + if (truncate_workaround && len > left && (len & 0xffffff) == left) { There should be a way to disable this workaround; after all, there'd be no remedy if the 32bit len field is bogus. Maybe check strict_std_compliance for being >= FF_COMPLIANCE_STRICT. > + av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\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))) { av_buffer_alloc() takes an int as its size argument. len + AV_INPUT_BUFFER_PADDING_SIZE can be anything here; it can be positive or negative. It can even be wrapped around. You need to check len for being < INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE. I wonder whether it should not be restricted even more (otherwise it would be easy to force an allocation of close to 2GiB). > 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 detect copy all data from block and read missing bytes was detected > + // 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"); >
On Sat, May 16, 2020 at 9:59 PM 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 | 35 +++++++++++++++++++++++++++-------- > > libavformat/flac_picture.h | 2 +- > > libavformat/flacdec.c | 2 +- > > libavformat/oggparsevorbis.c | 2 +- > > 4 files changed, 30 insertions(+), 11 deletions(-) > > > > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c > > index 81ddf80465..61277e9dee 100644 > > --- a/libavformat/flac_picture.c > > +++ b/libavformat/flac_picture.c > > @@ -27,7 +27,7 @@ > > #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; > > @@ -36,7 +36,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 +115,34 @@ 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) { > > + // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if > > I have not found the typical lavf metadata in the file from #6333, so it > seems we are not the only tool that created such out-of-spec files. This > should be reflected in the comment. Good point, have been clarified. > > > + // picture size did not fit in 24 bits > > + if (truncate_workaround && len > left && (len & 0xffffff) == left) { > > There should be a way to disable this workaround; after all, there'd be > no remedy if the 32bit len field is bogus. Maybe check > strict_std_compliance for being >= FF_COMPLIANCE_STRICT. Fixed. Made it fail if strict level it above normal. > > > + av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\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))) { > > av_buffer_alloc() takes an int as its size argument. len + > AV_INPUT_BUFFER_PADDING_SIZE can be anything here; it can be positive or > negative. It can even be wrapped around. You need to check len for being > < INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE. I wonder whether it should not > be restricted even more (otherwise it would be easy to force an > allocation of close to 2GiB). Fixed. Fails if >500MB or INT_MAX overflow. > > > 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 detect copy all data from block and read missing bytes > > was detected Fixed. > > > + // 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"); > > > > _______________________________________________ > 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 --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c index 81ddf80465..61277e9dee 100644 --- a/libavformat/flac_picture.c +++ b/libavformat/flac_picture.c @@ -27,7 +27,7 @@ #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; @@ -36,7 +36,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 +115,34 @@ 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) { + // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if + // picture size did not fit in 24 bits + if (truncate_workaround && len > left && (len & 0xffffff) == left) { + av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\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 detect 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");