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 <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
> 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 <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 */
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.
signature.asc
Description: PGP signature
_______________________________________________ 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".
