[FFmpeg-devel] avio_read data loss with multiple calls to fill_buffer

Submitted by Rob Meyers on May 18, 2017, 8:12 p.m.

Details

Message ID 20170518201249.87904-1-robertmeyers@google.com
State New
Headers show

Commit Message

Rob Meyers May 18, 2017, 8:12 p.m.
avio_read may make multiple calls to fill_buffer to accumulate bytes
to fulfill a request. fill_buffer will overwrite previously read data
if it is not prepped. Added a call to 'ffio_ensure_seekback' to adjust
the s->buffer to fill_buffer's liking. This isn't necessary for the
very first call to fill_buffer, thus the "size1 != size" check.
---
 libavformat/aviobuf.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Michael Niedermayer May 18, 2017, 10:53 p.m.
On Thu, May 18, 2017 at 01:12:49PM -0700, Rob Meyers wrote:
> avio_read may make multiple calls to fill_buffer to accumulate bytes
> to fulfill a request. fill_buffer will overwrite previously read data
> if it is not prepped. Added a call to 'ffio_ensure_seekback' to adjust
> the s->buffer to fill_buffer's liking. This isn't necessary for the
> very first call to fill_buffer, thus the "size1 != size" check.
> ---
>  libavformat/aviobuf.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 0a7c39eacd..8e9594cab8 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -648,6 +648,11 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size)
>                      s->buf_end = s->buffer/* + len*/;
>                  }
>              } else {
> +                if (size1 != size)
> +                    /* this call will ensure fill_buffer will not overwrite previously
> +                    read data. */
> +                    ffio_ensure_seekback(s, size1);

ffio_ensure_seekback() should be called when theres a potential
seek back in the following code.

There is no seekback in avio_read(), avio_read only moves forward.
Some callers may do a avio_read() and or other function calls and
then seek back. A caller might do 3 avio_read() and seek back over all

ffio_ensure_seekback() generally should be called by the code that
later initiates teh seek back

ffio_ensure_seekback() is not free it should only be used when it is
needed.



[...]
Rob Meyers May 18, 2017, 10:58 p.m.
avio_read does multiple calls to fill_buffer(). Adjusting things in
fill_buffer was frowned upon; how would you suggest I approach this? The
id3v2 seek doesn't come into play; the return from avio_read is the "full"
read.

The call I added in avio_read is only done when there was accumulated data,
so avoid reallocating the buffer the first time through.

On Thu, May 18, 2017 at 3:53 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Thu, May 18, 2017 at 01:12:49PM -0700, Rob Meyers wrote:
> > avio_read may make multiple calls to fill_buffer to accumulate bytes
> > to fulfill a request. fill_buffer will overwrite previously read data
> > if it is not prepped. Added a call to 'ffio_ensure_seekback' to adjust
> > the s->buffer to fill_buffer's liking. This isn't necessary for the
> > very first call to fill_buffer, thus the "size1 != size" check.
> > ---
> >  libavformat/aviobuf.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > index 0a7c39eacd..8e9594cab8 100644
> > --- a/libavformat/aviobuf.c
> > +++ b/libavformat/aviobuf.c
> > @@ -648,6 +648,11 @@ int avio_read(AVIOContext *s, unsigned char *buf,
> int size)
> >                      s->buf_end = s->buffer/* + len*/;
> >                  }
> >              } else {
> > +                if (size1 != size)
> > +                    /* this call will ensure fill_buffer will not
> overwrite previously
> > +                    read data. */
> > +                    ffio_ensure_seekback(s, size1);
>
> ffio_ensure_seekback() should be called when theres a potential
> seek back in the following code.
>
> There is no seekback in avio_read(), avio_read only moves forward.
> Some callers may do a avio_read() and or other function calls and
> then seek back. A caller might do 3 avio_read() and seek back over all
>
> ffio_ensure_seekback() generally should be called by the code that
> later initiates teh seek back
>
> ffio_ensure_seekback() is not free it should only be used when it is
> needed.
>
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Those who are too smart to engage in politics are punished by being
> governed by those who are dumber. -- Plato
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer May 19, 2017, 6:28 p.m.
hi

On Thu, May 18, 2017 at 10:58:39PM +0000, Rob Meyers wrote:
> avio_read does multiple calls to fill_buffer(). Adjusting things in
> fill_buffer was frowned upon; how would you suggest I approach this?

Its not frowned upon to fix a bug in fill_buffer(), its frowned
upon to make a random change that fixes a testcase without fixing
the underlaing bug. (there is a bug somewhere)


> The
> id3v2 seek doesn't come into play; the return from avio_read is the "full"
> read.

Just tried, if i comment
ff_id3v2_read_dict(s->pb, &s->internal->id3v2_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta);
out from avformat_open_input()
your testcase works


> 
> The call I added in avio_read is only done when there was accumulated data,
> so avoid reallocating the buffer the first time through.

> 
> On Thu, May 18, 2017 at 3:53 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Thu, May 18, 2017 at 01:12:49PM -0700, Rob Meyers wrote:
> > > avio_read may make multiple calls to fill_buffer to accumulate bytes
> > > to fulfill a request. fill_buffer will overwrite previously read data
> > > if it is not prepped. Added a call to 'ffio_ensure_seekback' to adjust
> > > the s->buffer to fill_buffer's liking. This isn't necessary for the
> > > very first call to fill_buffer, thus the "size1 != size" check.
> > > ---
> > >  libavformat/aviobuf.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > > index 0a7c39eacd..8e9594cab8 100644
> > > --- a/libavformat/aviobuf.c
> > > +++ b/libavformat/aviobuf.c
> > > @@ -648,6 +648,11 @@ int avio_read(AVIOContext *s, unsigned char *buf,
> > int size)
> > >                      s->buf_end = s->buffer/* + len*/;
> > >                  }
> > >              } else {
> > > +                if (size1 != size)
> > > +                    /* this call will ensure fill_buffer will not
> > overwrite previously
> > > +                    read data. */
> > > +                    ffio_ensure_seekback(s, size1);
> >
> > ffio_ensure_seekback() should be called when theres a potential
> > seek back in the following code.
> >
> > There is no seekback in avio_read(), avio_read only moves forward.
> > Some callers may do a avio_read() and or other function calls and
> > then seek back. A caller might do 3 avio_read() and seek back over all
> >
> > ffio_ensure_seekback() generally should be called by the code that
> > later initiates teh seek back
> >
> > ffio_ensure_seekback() is not free it should only be used when it is
> > needed.
> >
> >
> >
> > [...]
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Those who are too smart to engage in politics are punished by being
> > governed by those who are dumber. -- Plato
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer May 19, 2017, 7:16 p.m.
On Fri, May 19, 2017 at 08:28:23PM +0200, Michael Niedermayer wrote:
> hi
> 
> On Thu, May 18, 2017 at 10:58:39PM +0000, Rob Meyers wrote:
> > avio_read does multiple calls to fill_buffer(). Adjusting things in
> > fill_buffer was frowned upon; how would you suggest I approach this?
> 
> Its not frowned upon to fix a bug in fill_buffer(), its frowned
> upon to make a random change that fixes a testcase without fixing
> the underlaing bug.

> (there is a bug somewhere)

should be fixed

[...]
Rob Meyers May 19, 2017, 9:02 p.m.
I can confirm the current source tree has resolved our issue.

On Fri, May 19, 2017 at 12:16 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, May 19, 2017 at 08:28:23PM +0200, Michael Niedermayer wrote:
> > hi
> >
> > On Thu, May 18, 2017 at 10:58:39PM +0000, Rob Meyers wrote:
> > > avio_read does multiple calls to fill_buffer(). Adjusting things in
> > > fill_buffer was frowned upon; how would you suggest I approach this?
> >
> > Its not frowned upon to fix a bug in fill_buffer(), its frowned
> > upon to make a random change that fixes a testcase without fixing
> > the underlaing bug.
>
> > (there is a bug somewhere)
>
> should be fixed
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The real ebay dictionary, page 1
> "Used only once"    - "Some unspecified defect prevented a second use"
> "In good condition" - "Can be repaird by experienced expert"
> "As is" - "You wouldnt want it even if you were payed for it, if you knew
> ..."
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 0a7c39eacd..8e9594cab8 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -648,6 +648,11 @@  int avio_read(AVIOContext *s, unsigned char *buf, int size)
                     s->buf_end = s->buffer/* + len*/;
                 }
             } else {
+                if (size1 != size)
+                    /* this call will ensure fill_buffer will not overwrite previously
+                    read data. */
+                    ffio_ensure_seekback(s, size1);
+
                 fill_buffer(s);
                 len = s->buf_end - s->buf_ptr;
                 if (len == 0)