[FFmpeg-devel] [PATCH] clear s->error in avio_read

[Michael Niedermayer]

On Mon, Mar 20, 2017 at 02:49:18PM -0700, Fredrik Hubinette wrote:
> Hopefully valid patch attached.
> 
> 
> On Mon, Mar 20, 2017 at 2:39 PM, Fredrik Hubinette <hubbe at google.com> wrote:
> 
> > It looks like the value in s->error also comes from an earlier call to
> > avio_read().
> > ogg_read_page tries to read 4439 bytes from the file. It has 524 bytes
> > already buffered.
> > fill_buffer() fails with an EIO, but because of the buffered bytes,
> > avio_read()
> > returns 524 and leaves the EIO in s->error.
> >
> >   /Fredrik "Hubbe" Hubinette
> >
> >
> > On Mon, Mar 20, 2017 at 1:53 PM, Fredrik Hubinette <hubbe at google.com>
> > wrote:
> >
> >>
> >> On Mon, Mar 20, 2017 at 1:35 PM, Michael Niedermayer <
> >> michael at niedermayer.cc> wrote:
> >>
> >>> On Mon, Mar 20, 2017 at 10:21:08AM -0700, Fredrik Hubinette wrote:
> >>> > In some cases (when parsing OGG) non-fatal errors can happen, which
> >>> > will cause s->error to be set. In most cases, this is not a problem
> >>> beucase
> >>> > s->error is not checked unless an actual error has occurred.  However,
> >>> > when avio_read() fails to read more bytes, it checks s->error to
> >>> decide if
> >>> > it just reached the end of the file, or an error occurred. Since
> >>> s->error is
> >>> > not modified if no error occurred, this is not reliable unless we first
> >>> > clear
> >>> > s->error before reading.
> >>> >
> >>> > ---
> >>> >  libavformat/aviobuf.c   | 2 ++
> >>> >
> >>> >  2 files changed, 7 insertions(+)
> >>>
> >>> how can the issue you describe be reproduced
> >>>
> >>>
> >> I don't currently have a simple repro. I discovered it while trying to
> >> add some buffering
> >> between the demuxer and decoder in chrome. One of our tests fails, and
> >> when
> >> looked into it, I discovered that s->error was already 5 (EIO) when
> >> entering
> >> avio_read. I have not yet tracked down where the EIO came from
> >> originally.
> >>
> >> also thats not a valid git patc
> >>>
> >>
> >> Ops, I had a chrome patch and I removed the chrome-specific parts, but
> >> I must have messed it up. I will make a better patch.
> >>
> >>
> >>> [...]
> >>> --
> >>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >>>
> >>> Those who are best at talking, realize last or never when they are wrong.
> >>>
> >>> _______________________________________________
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel at ffmpeg.org
> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>
> >>>
> >>
> >

>  aviobuf.c |    2 ++
>  1 file changed, 2 insertions(+)
> 3ef8ffcafab28750ede89a2b7390203a95afb59f  0001-clear-s-error-in-avio_read.patch
> From a26c186d2ac6d013215e9707fa3508ba1ef8537a Mon Sep 17 00:00:00 2001
> From: Fredrik Hubinette <hubbe at google.com>
> Date: Mon, 20 Mar 2017 14:47:06 -0700
> Subject: [PATCH] clear s->error in avio_read
> 
> ---
>  libavformat/aviobuf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 4ade4d0d7e..0912dfdb57 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -615,6 +615,8 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size)
>  {
>      int len, size1;
>  
> +    s->error = 0;

error should not be cleared by read so that the "user" can check for
errors after a sequence of reads without needing to check after
each individual read. Some demuxers do this that would break if
error is reset here

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170324/9d312e09/attachment.sig>