[FFmpeg-devel] [PATCH] aviobuf: Write new data at s->buf_end in fill_buffer

[Martin Storsjö]

On Sun, 27 Feb 2011, Ronald S. Bultje wrote:

> On Sat, Feb 26, 2011 at 6:02 PM, Martin Storsj? <martin at martin.st> wrote:
> > In most cases, s->buf_ptr will be equal to s->buf_end when
> > fill_buffer is called, but this may not always be the case, if
> > we're seeking forward by reading (permitted by the short seek
> > threshold).
> >
> > If fill_buffer is writing to s->buf_ptr instead of s->buf_end (when
> > they aren't equal and s->buf_ptr is ahead of s->buffer), the data
> > between s->buf_ptr and s->buf_end is overwritten, leading to
> > inconsistent buffer content. This could return incorrect data if
> > later seeking back into the area before the current s->buf_ptr.
> > ---
> > ?libavformat/aviobuf.c | ? ?2 +-
> > ?1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > index 270352e..e5808b6 100644
> > --- a/libavformat/aviobuf.c
> > +++ b/libavformat/aviobuf.c
> > @@ -460,7 +460,7 @@ void put_tag(AVIOContext *s, const char *tag)
> >
> > ?static void fill_buffer(AVIOContext *s)
> > ?{
> > - ? ?uint8_t *dst= !s->max_packet_size && s->buf_end - s->buffer < s->buffer_size ? s->buf_ptr : s->buffer;
> > + ? ?uint8_t *dst= !s->max_packet_size && s->buf_end - s->buffer < s->buffer_size ? s->buf_end : s->buffer;
> 
> What is the bug you're trying to fix? I.e. what behaviour is
> inconsistent, or what sample plays back badly, or what application
> code fails / is impossible because of this?

I haven't actually stumbled upon this as a bug, but while debugging other 
aviobuf behaviour, I noticed that this didn't behave as it should.

The bug that _could_ happen, would be the following:

We've got bytes 0-200 from the source in the buffer, while buf_ptr points 
to byte 100 of this. fill_buffer() is called, and manages to read 100 
bytes more into the buffer. Since this currently, incorrect, writes this 
into buf_ptr, we've got bytes 0-100 in positions 0-100 in the buffer, and 
bytes 200-300 at position 100-200. This is inconsistent, since all the 
data in the buffer isn't sequential, as is assumed.

If the caller simply reads onwards, there won't be any problem, but _if_ 
the caller would choose to seek backwards, e.g. to position 190 in the 
source, there would be a problem. The end of the buffered data, at 
position 200 in the buffer, corresponds to byte position 300 in the 
source. This says that position 190 in the source would be at position 90 
in the buffer, and starts reading from there. This returns incorrect data, 
since byte 90 in the buffer actually is byte 90 in the source.

Thus, since all the data in the buffer should be sequential, we should 
either write from the start of the buffer (if the buffer was full before), 
or at the end of the current buffered data, buf_end, as my patch makes it 
do.

// Martin