[FFmpeg-devel] [PATCH 11/15] avformat/flacdec:Return correct error-codes on read-failure

[Michael Niedermayer]

On Wed, Oct 30, 2024 at 11:44:37AM +0100, Tomas Härdin wrote:
> 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

>  flacdec.c |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 8d27d2b37842444aad8f5df06ba02b7511ac8425  0011-avformat-flacdec-Return-correct-error-codes-on-read-.patch
> From 5f19fddd70417284f36421e67b92af673b7c6965 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/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 */

should be ok

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241103/bb33bf0c/attachment.sig>