Re: [FFmpeg-devel] [PATCH 11/15] avformat/flacdec:Return correct error-codes on read-failure
ons 2024-10-30 klockan 11:44 +0100 skrev Tomas Härdin: > 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 Actually I feel like bikeshedding/philosophizing over this a bit since it is probably relevant to other parts of the code. I feel there is an important qualitative difference between a file being too short and a file containing bad data, and we'd like to within the best of our abilities tell the user what the problem is. But this isn't always so easy. Consider a length field followed by not enough data. This could happen for two reasons: either the file was terminated early or the length field was written incorrectly or somehow corrupted. Should we assume that the file was written correctly and cut short, or the opposite? In this specific case I think the answer is easy: always return EOF. Why? Because the streaminfo chunk is 34 bytes by definition, and unless the muxer is incredibly broken, an incomplete read is almost certainly due to the file being cut early We could in principle move the checks for metadata_size further up, but it's probably not worthwhile here. In a proper parsing framework this kind of stuff could be expressed in a grammar. For streaminfo it should match \x00\x00\x00\x22 exactly TL;DR: I'm going to change to returning EOF here, unless someone objects /Tomas ___ 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".
Re: [FFmpeg-devel] [PATCH 11/15] avformat/flacdec:Return correct error-codes on read-failure
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
> > > 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
> 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
Re: [FFmpeg-devel] [PATCH 11/15] avformat/flacdec:Return correct error-codes on read-failure
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
> > 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
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_INVALIDD
Re: [FFmpeg-devel] [PATCH 11/15] avformat/flacdec:Return correct error-codes on read-failure
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
> > 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.
Could be a rebase mistake. I'll take a second look at it tomorrow
/Tomas
___
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".
Re: [FFmpeg-devel] [PATCH 11/15] avformat/flacdec:Return correct error-codes on read-failure
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
> 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.
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.
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".
