[FFmpeg-devel] libavformat/flac_picture: Don't return AVERROR_INVALIDDATA for errors with flac picture mimetype

In the case where the FLAC picture MIME type is not understood, fail to
parse the picture silently rather than return AVERROR_INVALIDDATA.

This originated from a bug reported in Chromium: https://crbug.com/1052821

Signed-off-by: Will Cassella <cassew@google.com>
---
 libavformat/flac_picture.c | 4 ----
 1 file changed, 4 deletions(-)

     if (len + 24 > bytestream2_get_bytes_left(&g)) {
@@ -91,8 +89,6 @@ int ff_flac_parse_picture(AVFormatContext *s,
uint8_t **bufp, int buf_size,
     if (id == AV_CODEC_ID_NONE) {
         av_log(s, AV_LOG_ERROR, "Unknown attached picture mimetype: %s.\n",
                mimetype);
-        if (s->error_recognition & AV_EF_EXPLODE)
-            return AVERROR_INVALIDDATA;
         return 0;
     }

On 9/9/2022 7:44 PM, Will Cassella wrote:
> In the case where the FLAC picture MIME type is not understood, fail to
> parse the picture silently rather than return AVERROR_INVALIDDATA.
> 
> This originated from a bug reported in Chromium: https://crbug.com/1052821
> 
> Signed-off-by: Will Cassella <cassew@google.com>
> ---
>   libavformat/flac_picture.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index b33fee75b4..1acad9b251 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -68,8 +68,6 @@ int ff_flac_parse_picture(AVFormatContext *s,
> uint8_t **bufp, int buf_size,
>       if (len <= 0 || len >= sizeof(mimetype)) {
>           av_log(s, AV_LOG_ERROR, "Could not read mimetype from an attached "
>                  "picture.\n");
> -        if (s->error_recognition & AV_EF_EXPLODE)
> -            return AVERROR_INVALIDDATA;

If you don't want to error out, then don't enable explode mode, which is 
meant to abort on the slightest issue?

>           return 0;
>       }
>       if (len + 24 > bytestream2_get_bytes_left(&g)) {
> @@ -91,8 +89,6 @@ int ff_flac_parse_picture(AVFormatContext *s,
> uint8_t **bufp, int buf_size,
>       if (id == AV_CODEC_ID_NONE) {
>           av_log(s, AV_LOG_ERROR, "Unknown attached picture mimetype: %s.\n",
>                  mimetype);
> -        if (s->error_recognition & AV_EF_EXPLODE)
> -            return AVERROR_INVALIDDATA;
>           return 0;
>       }
>

Sorry for the delay, didn't see your reply!

The issue is that we do want to error out most of the time on invalid
input, but for our needs the mime-type of the attached picture being
empty or unknown isn't really relevant. Could there be a way to
classify this error differently so that it could be ignored, while
keeping AV_EF_EXPLODE enabled for everything else?

Thanks,
Will

On Fri, Sep 9, 2022 at 6:16 PM James Almer <jamrial@gmail.com> wrote:
>
> On 9/9/2022 7:44 PM, Will Cassella wrote:
> > In the case where the FLAC picture MIME type is not understood, fail to
> > parse the picture silently rather than return AVERROR_INVALIDDATA.
> >
> > This originated from a bug reported in Chromium: https://crbug.com/1052821
> >
> > Signed-off-by: Will Cassella <cassew@google.com>
> > ---
> >   libavformat/flac_picture.c | 4 ----
> >   1 file changed, 4 deletions(-)
> >
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index b33fee75b4..1acad9b251 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -68,8 +68,6 @@ int ff_flac_parse_picture(AVFormatContext *s,
> > uint8_t **bufp, int buf_size,
> >       if (len <= 0 || len >= sizeof(mimetype)) {
> >           av_log(s, AV_LOG_ERROR, "Could not read mimetype from an attached "
> >                  "picture.\n");
> > -        if (s->error_recognition & AV_EF_EXPLODE)
> > -            return AVERROR_INVALIDDATA;
>
> If you don't want to error out, then don't enable explode mode, which is
> meant to abort on the slightest issue?
>
> >           return 0;
> >       }
> >       if (len + 24 > bytestream2_get_bytes_left(&g)) {
> > @@ -91,8 +89,6 @@ int ff_flac_parse_picture(AVFormatContext *s,
> > uint8_t **bufp, int buf_size,
> >       if (id == AV_CODEC_ID_NONE) {
> >           av_log(s, AV_LOG_ERROR, "Unknown attached picture mimetype: %s.\n",
> >                  mimetype);
> > -        if (s->error_recognition & AV_EF_EXPLODE)
> > -            return AVERROR_INVALIDDATA;
> >           return 0;
> >       }
> >
> _______________________________________________
> 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".

Quoting James Almer (2022-09-10 03:16:32)
> On 9/9/2022 7:44 PM, Will Cassella wrote:
> > In the case where the FLAC picture MIME type is not understood, fail to
> > parse the picture silently rather than return AVERROR_INVALIDDATA.
> > 
> > This originated from a bug reported in Chromium: https://crbug.com/1052821
> > 
> > Signed-off-by: Will Cassella <cassew@google.com>
> > ---
> >   libavformat/flac_picture.c | 4 ----
> >   1 file changed, 4 deletions(-)
> > 
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index b33fee75b4..1acad9b251 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -68,8 +68,6 @@ int ff_flac_parse_picture(AVFormatContext *s,
> > uint8_t **bufp, int buf_size,
> >       if (len <= 0 || len >= sizeof(mimetype)) {
> >           av_log(s, AV_LOG_ERROR, "Could not read mimetype from an attached "
> >                  "picture.\n");
> > -        if (s->error_recognition & AV_EF_EXPLODE)
> > -            return AVERROR_INVALIDDATA;
> 
> If you don't want to error out, then don't enable explode mode, which is 
> meant to abort on the slightest issue?

IMO failing to recognize the MIME type is a lavf error rather than a
file error and so should not fail, much less with AVERROR_INVALIDDATA
(should be ENOSYS if anything).

THe other error should stay though - then the file actually is broken.

Will left Google, but I've updated the patch to only skip errors when the
type is unrecognized.

On Wed, Sep 28, 2022 at 7:17 AM Anton Khirnov <anton@khirnov.net> wrote:

> IMO failing to recognize the MIME type is a lavf error rather than a
> file error and so should not fail, much less with AVERROR_INVALIDDATA
> (should be ENOSYS if anything).
>

Bump on this patch -- it still applies cleanly. We're still seeing files in
the wild like this.

- dale

On Mon, Apr 10, 2023 at 10:54 AM Dale Curtis <dalecurtis@chromium.org>
wrote:

> Will left Google, but I've updated the patch to only skip errors when the
> type is unrecognized.
>
> On Wed, Sep 28, 2022 at 7:17 AM Anton Khirnov <anton@khirnov.net> wrote:
>
>> IMO failing to recognize the MIME type is a lavf error rather than a
>> file error and so should not fail, much less with AVERROR_INVALIDDATA
>> (should be ENOSYS if anything).
>>
>

On Wed, Mar 27, 2024 at 01:05:52PM -0700, Dale Curtis wrote:
> Bump on this patch -- it still applies cleanly. We're still seeing files in
> the wild like this.

will apply

thx

[...]