[PATCH] 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(-) 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; 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; } -- 2.37.2.789.g6183377224-goog
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. -- Anton Khirnov
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope
participants (5)
-
Anton Khirnov -
Dale Curtis -
James Almer -
Michael Niedermayer -
Will Cassella