tis 2024-10-29 klockan 18:42 +0100 skrev Michael Niedermayer:
> On Tue, Oct 29, 2024 at 03:50:05PM +0100, Tomas Härdin wrote:
> > Reasonable enough. Might want a sample
> >
> > Spotify comments
> > ----------------
> > Unexpected EOF is treated as invalid data
> >
> > /Tomas
>
> > flacdec.c | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> > 8cf62c7b8b7e576cc0e2a0a1e49c4f42be5fc254 0011-avformat-flacdec-
> > Return-correct-error-codes-on-read-.patch
> > From 4d803c5f5c13e194a0e52d287f021b73ec7172bd Mon Sep 17 00:00:00
> > 2001
> > From: Ulrik <ulr...@spotify.com>
> > Date: Thu, 26 Jan 2023 17:51:02 +0100
> > Subject: [PATCH 11/15] avformat/flacdec:Return correct error-codes
> > on
> > read-failure
> >
> > Forward errors from `avio_read` directly. When `avio_read` sees EOF
> > before
> > expected bytes can be read, consistently return
> > `AVERROR_INVALIDDATA`
> >
> > We used to return `AVERROR(AVERROR_INVALIDDATA)` when failing to
> > read
> > metadata block headers. `AVERROR_INVALIDDATA` is already negative,
> > so
> > wrapping in `AVERROR` leads to double-negation.
> >
> > We used to return `AVERROR(EIO)` when failing to read extended
> > metadata.
> > However, many times, the IO-layer is not at fault, the input data
> > is simply
> > corrupted (truncated), so we return `AVERROR_INVALIDDATA` here as
> > well.
> > ---
> > libavformat/flacdec.c | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
> > index 9f65c25864..be305fec82 100644
> > --- a/libavformat/flacdec.c
> > +++ b/libavformat/flacdec.c
> > @@ -81,8 +81,15 @@ static int flac_read_header(AVFormatContext *s)
> >
> > /* process metadata blocks */
> > while (!avio_feof(s->pb) && !metadata_last) {
> > - if (avio_read(s->pb, header, 4) != 4)
> > - return AVERROR_INVALIDDATA;
> > + ret = avio_read(s->pb, header, 4);
>
> > + if (ret != 4) {
> > + if (ret < 0) {
> > + goto fail;
> > + } else {
> > + return AVERROR_INVALIDDATA;
> > + }
> > + }
>
> this is wierd
> because, one path goto fails the other returns directly.Oh wait now I see what you mean. buffer isn't allocated in this hunk so it could just as well just return. The second hunk *should* goto fail however. Updated patch attached I also made some test files to demonstrate the differences in behavior. What's being addressed here is early termination of the file. One thing to bikeshed over is whether to return AVERROR_EOF or AVERROR_INVALIDDATA in that case. The attached files demonstrate a change in return value depending on how short a flac file is. It might make more sense to always return AVERROR_EOF since being truncated is fundamentally the issue with the file in these cases The original intent seems mostly to just be passing error codes along /Tomas
From 5f19fddd70417284f36421e67b92af673b7c6965 Mon Sep 17 00:00:00 2001
From: Ulrik <ulr...@spotify.com>
Date: Thu, 26 Jan 2023 17:51:02 +0100
Subject: [PATCH 11/17] avformat/flacdec:Return correct error-codes on
read-failure
Forward errors from `avio_read` directly. When `avio_read` sees EOF before
expected bytes can be read, consistently return `AVERROR_INVALIDDATA`
We used to return `AVERROR(AVERROR_INVALIDDATA)` when failing to read
metadata block headers. `AVERROR_INVALIDDATA` is already negative, so
wrapping in `AVERROR` leads to double-negation.
We used to return `AVERROR(EIO)` when failing to read extended metadata.
However, many times, the IO-layer is not at fault, the input data is simply
corrupted (truncated), so we return `AVERROR_INVALIDDATA` here as well.
---
libavformat/flacdec.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index 9f65c25864..da7fbc4dea 100644
--- a/libavformat/flacdec.c
+++ b/libavformat/flacdec.c
@@ -81,8 +81,13 @@ static int flac_read_header(AVFormatContext *s)
/* process metadata blocks */
while (!avio_feof(s->pb) && !metadata_last) {
- if (avio_read(s->pb, header, 4) != 4)
+ ret = avio_read(s->pb, header, 4);
+ if (ret < 0) {
+ return ret;
+ } else if (ret != 4) {
return AVERROR_INVALIDDATA;
+ }
+
flac_parse_block_header(header, &metadata_last, &metadata_type,
&metadata_size);
switch (metadata_type) {
@@ -96,8 +101,11 @@ static int flac_read_header(AVFormatContext *s)
if (!buffer) {
return AVERROR(ENOMEM);
}
- if (avio_read(s->pb, buffer, metadata_size) != metadata_size) {
- RETURN_ERROR(AVERROR(EIO));
+ ret = avio_read(s->pb, buffer, metadata_size);
+ if (ret < 0) {
+ RETURN_ERROR(ret);
+ } else if (ret != metadata_size) {
+ RETURN_ERROR(AVERROR_INVALIDDATA);
}
break;
/* skip metadata block for unsupported types */
--
2.39.2
spotify-eof-fLaC-only.flac
Description: audio/flac
spotify-eof-streaminfo.flac
Description: audio/flac
spotify-short-header.flac
Description: audio/flac
spotify-short-streaminfo.flac
Description: audio/flac
_______________________________________________ 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".
