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

[Mattias Wadman]

On Wed, Apr 22, 2020 at 5:18 PM Andreas Rheinhardt
<andreas.rheinhardt at 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.
> >
> > flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> > ---
> >  libavformat/flac_picture.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index 81ddf80465..17be497f95 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -32,11 +32,12 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> >      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, lendiff;
> >
> >      if (buf_size < 34) {
> >          av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
> > @@ -114,6 +115,23 @@ 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 (len > left && (len & 0xffffff) == left) {
> > +        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 +173,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;
> >  }
> >
> 1. This fixes ticket 6333 (you will now no longer run into the flac
> parser bug), doesn't it? If so, you should mention it.

Yes this patch should fixat that. Didn't know there was a ticket, will
mention it.

> 2. ff_flac_parse_picture() is called from two places: The flac demuxer
> as well as the vorbis-in-ogg demuxer. In the latter case the picture
> data is base64-encoded and reading anything via the AVIOContext is a
> very bad idea (ff_vorbis_comment() is for e.g. used to parse
> VorbisComments contained in the CodecPrivate of FLAC tracks (for channel
> masks) and if such a CodecPrivate also contained an embedded picture
> that happens to trigger this scenario, you would read more data
> believing it comes from immediately after the buffer you have received,
> but that is just not true). You need to add a parameter to distinguish
> these cases.

Ok will look into that. Thanks for the speedy review!

-Mattias

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