On Wed, Apr 22, 2020 at 5:18 PM Andreas Rheinhardt
<andreas.rheinha...@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
