[FFmpeg-devel] [PATCH 11/15] avformat/flacdec:Return correct error-codes on read-failure
Tomas Härdin
git at haerdin.se
Wed Oct 30 12:44:37 EET 2024
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 <ulrikm at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-avformat-flacdec-Return-correct-error-codes-on-read-.patch
Type: text/x-patch
Size: 2175 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241030/c0ef95a7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spotify-eof-fLaC-only.flac
Type: audio/flac
Size: 4 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241030/c0ef95a7/attachment.flac>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spotify-eof-streaminfo.flac
Type: audio/flac
Size: 8 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241030/c0ef95a7/attachment-0001.flac>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spotify-short-header.flac
Type: audio/flac
Size: 7 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241030/c0ef95a7/attachment-0002.flac>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spotify-short-streaminfo.flac
Type: audio/flac
Size: 9 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241030/c0ef95a7/attachment-0003.flac>
More information about the ffmpeg-devel
mailing list