[FFmpeg-devel] [PATCH v3] libavformat/flacdec: Workaround for truncated metadata picture size
Mattias Wadman
mattias.wadman at gmail.com
Tue May 12 15:25:06 EEST 2020
Hi, any chance to get this merged?
On Sun, May 3, 2020 at 12:32 PM Mattias Wadman <mattias.wadman at gmail.com> wrote:
>
> Sorry for nagging, something left to fix?
>
> On Mon, Apr 27, 2020 at 10:10 AM Mattias Wadman
> <mattias.wadman at gmail.com> wrote:
> >
> > Hi again, looks ok?
> >
> > On Fri, Apr 24, 2020 at 12:54 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 | 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
> > >
More information about the ffmpeg-devel
mailing list