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

Submitted by Fredrik Hubinette on March 20, 2017, 9:49 p.m.

Details

Message ID CAEVbG5rZoPsK5jTA5T0MaYRWB83WqD=KRGztNt_U_BrcgCtDiA@mail.gmail.com
State New
Headers show

Commit Message

Fredrik Hubinette March 20, 2017, 9:49 p.m.
Hopefully valid patch attached.


On Mon, Mar 20, 2017 at 2:39 PM, Fredrik Hubinette <hubbe@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@google.com>
> wrote:
>
>>
>> On Mon, Mar 20, 2017 at 1:35 PM, Michael Niedermayer <
>> michael@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@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>>
>>
>

Comments

Fredrik Hubinette March 22, 2017, 10:58 p.m.
ping?


On Mon, Mar 20, 2017 at 2:49 PM, Fredrik Hubinette <hubbe@google.com> wrote:

> Hopefully valid patch attached.
>
>
> On Mon, Mar 20, 2017 at 2:39 PM, Fredrik Hubinette <hubbe@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@google.com>
>> wrote:
>>
>>>
>>> On Mon, Mar 20, 2017 at 1:35 PM, Michael Niedermayer <
>>> michael@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@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>>
>>>
>>
>
Michael Niedermayer March 24, 2017, 1:10 a.m.
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@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@google.com>
> > wrote:
> >
> >>
> >> On Mon, Mar 20, 2017 at 1:35 PM, Michael Niedermayer <
> >> michael@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@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@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

[...]

Patch hide | download patch | download mbox

From a26c186d2ac6d013215e9707fa3508ba1ef8537a Mon Sep 17 00:00:00 2001
From: Fredrik Hubinette <hubbe@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;
+
     size1 = size;
     while (size > 0) {
         len = FFMIN(s->buf_end - s->buf_ptr, size);
-- 
2.12.0.367.g23dc2f6d3c-goog