[FFmpeg-devel] libavformat aviobuf: Fixed dst pointer initialization in fill_buffer

Submitted by Rob Meyers on May 15, 2017, 4:47 p.m.

Details

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

Commit Message

Rob Meyers May 15, 2017, 4:47 p.m.
---
 libavformat/aviobuf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Hendrik Leppkes May 15, 2017, 5:41 p.m.
On Mon, May 15, 2017 at 6:47 PM, Rob Meyers
<robertmeyers-at-google.com@ffmpeg.org> wrote:
> ---
>  libavformat/aviobuf.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>

Commit messages that say "fixed <thing>" are usually not quite
informative. Can you elaborate whats the problem, how it exhibits and
can be tested and how this is fixed?

- Hendrik
Rob Meyers May 15, 2017, 5:55 p.m.
Of course.

We noticed when reading data from a named pipe the first 10 bytes would get
dropped. I traced this to the affected code in fill_buffer(). The
assignment of "dst" was always set to the beginning of the buffer, and if
it hadn't been consumed yet the data would be overwritten. We could
reproduce this by setting up a server that writes to the named pipe in two
small (6 byte) messages with a 1 second gap between. Without the gap, or if
the data is sent as one message, there's no problem. It's in the
accumulation of data between messages to fulfill a read that this bug is
triggered.

On Mon, May 15, 2017 at 10:49 AM Hendrik Leppkes <h.leppkes@gmail.com>
wrote:

> On Mon, May 15, 2017 at 6:47 PM, Rob Meyers
> <robertmeyers-at-google.com@ffmpeg.org> wrote:
> > ---
> >  libavformat/aviobuf.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
>
> Commit messages that say "fixed <thing>" are usually not quite
> informative. Can you elaborate whats the problem, how it exhibits and
> can be tested and how this is fixed?
>
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
wm4 May 16, 2017, 4:47 a.m.
On Mon, 15 May 2017 17:55:28 +0000
Rob Meyers <robertmeyers-at-google.com@ffmpeg.org> wrote:

> Of course.
> 
> We noticed when reading data from a named pipe the first 10 bytes would get
> dropped. I traced this to the affected code in fill_buffer(). The
> assignment of "dst" was always set to the beginning of the buffer, and if
> it hadn't been consumed yet the data would be overwritten. We could
> reproduce this by setting up a server that writes to the named pipe in two
> small (6 byte) messages with a 1 second gap between. Without the gap, or if
> the data is sent as one message, there's no problem. It's in the
> accumulation of data between messages to fulfill a read that this bug is
> triggered.

Well, that explanation should be in the commit message (with a short 70
char summary as subject line). Not just "fixed $thing".
Rob Meyers May 16, 2017, 3:49 p.m.
No problem. I'll resubmit the patch request.

On Mon, May 15, 2017 at 9:47 PM wm4 <nfxjfg@googlemail.com> wrote:

> On Mon, 15 May 2017 17:55:28 +0000
> Rob Meyers <robertmeyers-at-google.com@ffmpeg.org> wrote:
>
> > Of course.
> >
> > We noticed when reading data from a named pipe the first 10 bytes would
> get
> > dropped. I traced this to the affected code in fill_buffer(). The
> > assignment of "dst" was always set to the beginning of the buffer, and if
> > it hadn't been consumed yet the data would be overwritten. We could
> > reproduce this by setting up a server that writes to the named pipe in
> two
> > small (6 byte) messages with a 1 second gap between. Without the gap, or
> if
> > the data is sent as one message, there's no problem. It's in the
> > accumulation of data between messages to fulfill a read that this bug is
> > triggered.
>
> Well, that explanation should be in the commit message (with a short 70
> char summary as subject line). Not just "fixed $thing".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer May 16, 2017, 4:06 p.m.
On Mon, May 15, 2017 at 05:55:28PM +0000, Rob Meyers wrote:
> Of course.
> 
> We noticed when reading data from a named pipe the first 10 bytes would get
> dropped.

from were are bytes droped ?
this is the internal buffer, not something a user should touch
directly.


> I traced this to the affected code in fill_buffer(). The
> assignment of "dst" was always set to the beginning of the buffer, and if

The buffer pointer resets to the start if there is not enough space
available to hold another packet.
with your patch it resets either always or when the buffer is filled
to the last byte randomly truncating a read.
If the read function is incapable to fullfill the partial packet read
that will not work out i think
But i may be missing something of course



> it hadn't been consumed yet the data would be overwritten. We could
> reproduce this by setting up a server that writes to the named pipe in two
> small (6 byte) messages with a 1 second gap between. Without the gap, or if
> the data is sent as one message, there's no problem. It's in the
> accumulation of data between messages to fulfill a read that this bug is
> triggered.

[...]
Rob Meyers May 16, 2017, 4:17 p.m.
Here's a simple server we used to reproduce the problem:

#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <unistd.h>

#define INGOING "/tmp/audio.fifo"

ssize_t write_fifo(int fd, char* msg, int size) {
  ssize_t write_rc;

  write_rc = write(fd, msg, size);
  printf("msg '%s' write_rc = %ld\n", msg, write_rc);
  if (write_rc != size) {
    perror("write sent fewer bytes than expected");
  }
  if (write_rc == -1) {
    perror("write error");
  }

  return write_rc;
}

int main(int argc, char *argv[]) {
  mkfifo(INGOING, 0666);

  printf("Welcome to server.\n");
  printf("channel for reading messages to server is %s\n", INGOING);

  int in_fd = open(INGOING, O_WRONLY);
  if (in_fd == -1) {
    perror("open error");
    return 1;
  }

  int argi = 1;
  while (argi < argc) {
    char *msg = argv[argi];
    int len = strlen(msg);
    ++argi;

    write_fifo(in_fd, msg, len);
    sleep(1);
  }

  return 0;
}

--

The command line arguments are sent as individual messages. To demonstrate
the problem I run with "abcdef ghijkl" as the args. This sends two 6 byte
messages with a short gap between.

I run ffmpeg like so:
ffmpeg -ar 1000 -ac 1 -acodec pcm_s16le -f s16le -probesize 5000000 -i
 /tmp/audio.fifo -map_metadata -1 -vn -ac:a:0 1 -ar:a:0 1000 -codec:a:0
pcm_s16le -sn -f s16le -y  -

Without my patch, you won't see all 12 bytes output, just "kl". My patch
fixes this.

The buffer is in AVIOContext. The problem with the original code was that
max_buffer_size is added to the request, to see if it's larger than another
value that is also max_buffer_size. Like, "if (A + B) < B".


On Tue, May 16, 2017 at 9:06 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, May 15, 2017 at 05:55:28PM +0000, Rob Meyers wrote:
> > Of course.
> >
> > We noticed when reading data from a named pipe the first 10 bytes would
> get
> > dropped.
>
> from were are bytes droped ?
> this is the internal buffer, not something a user should touch
> directly.
>
>
> > I traced this to the affected code in fill_buffer(). The
> > assignment of "dst" was always set to the beginning of the buffer, and if
>
> The buffer pointer resets to the start if there is not enough space
> available to hold another packet.
> with your patch it resets either always or when the buffer is filled
> to the last byte randomly truncating a read.
> If the read function is incapable to fullfill the partial packet read
> that will not work out i think
> But i may be missing something of course
>
>
>
> > it hadn't been consumed yet the data would be overwritten. We could
> > reproduce this by setting up a server that writes to the named pipe in
> two
> > small (6 byte) messages with a 1 second gap between. Without the gap, or
> if
> > the data is sent as one message, there's no problem. It's in the
> > accumulation of data between messages to fulfill a read that this bug is
> > triggered.
>
> [...]
> --
> 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 May 16, 2017, 8:23 p.m.
On Tue, May 16, 2017 at 04:17:45PM +0000, Rob Meyers wrote:
> Here's a simple server we used to reproduce the problem:
> 
> #include <fcntl.h>
> #include <stdio.h>
> #include <string.h>
> #include <sys/types.h>
> #include <unistd.h>
> 
> #define INGOING "/tmp/audio.fifo"
> 
> ssize_t write_fifo(int fd, char* msg, int size) {
>   ssize_t write_rc;
> 
>   write_rc = write(fd, msg, size);
>   printf("msg '%s' write_rc = %ld\n", msg, write_rc);
>   if (write_rc != size) {
>     perror("write sent fewer bytes than expected");
>   }
>   if (write_rc == -1) {
>     perror("write error");
>   }
> 
>   return write_rc;
> }
> 
> int main(int argc, char *argv[]) {
>   mkfifo(INGOING, 0666);
> 
>   printf("Welcome to server.\n");
>   printf("channel for reading messages to server is %s\n", INGOING);
> 
>   int in_fd = open(INGOING, O_WRONLY);
>   if (in_fd == -1) {
>     perror("open error");
>     return 1;
>   }
> 
>   int argi = 1;
>   while (argi < argc) {
>     char *msg = argv[argi];
>     int len = strlen(msg);
>     ++argi;
> 
>     write_fifo(in_fd, msg, len);
>     sleep(1);
>   }
> 
>   return 0;
> }
> 
> --
> 
> The command line arguments are sent as individual messages. To demonstrate
> the problem I run with "abcdef ghijkl" as the args. This sends two 6 byte
> messages with a short gap between.
> 
> I run ffmpeg like so:
> ffmpeg -ar 1000 -ac 1 -acodec pcm_s16le -f s16le -probesize 5000000 -i
>  /tmp/audio.fifo -map_metadata -1 -vn -ac:a:0 1 -ar:a:0 1000 -codec:a:0
> pcm_s16le -sn -f s16le -y  -
> 

> Without my patch, you won't see all 12 bytes output, just "kl". My patch
> fixes this.

The patch makes it output all 12 bytes but this is more by luck than
by fixing the issue.

The issue is, well there are at least 2 issues
First the ff_id3v2_read_dict() Tries to read 10 bytes and failing to
find a id3 header seeks back.
This is buggy, it must call ffio_ensure_seekback() before it starts
any reading that it intends to seek back over.

the 2nd issue seems to be that fill_buffer() throws the larger
buffer ffio_ensure_seekback() created out too early.
I didnt investigate this deeply but something goes wrong there

and with ffio_ensure_seekback() and with the downallocation commented
the whole works and returns the 12 bytes



> 
> The buffer is in AVIOContext. The problem with the original code was that
> max_buffer_size is added to the request, to see if it's larger than another
> value that is also max_buffer_size. Like, "if (A + B) < B".

It may be that the buffer size and the maximum packet size are often
the same.  But they are not always the same, for example
ffio_ensure_seekback() makes them different

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 0a7c39eacd..4e04cb79e0 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -519,9 +519,7 @@  void avio_write_marker(AVIOContext *s, int64_t time, enum AVIODataMarkerType typ
 
 static void fill_buffer(AVIOContext *s)
 {
-    int max_buffer_size = s->max_packet_size ?
-                          s->max_packet_size : IO_BUFFER_SIZE;
-    uint8_t *dst        = s->buf_end - s->buffer + max_buffer_size < s->buffer_size ?
+    uint8_t *dst        = !s->max_packet_size && s->buf_end - s->buffer < s->buffer_size ?
                           s->buf_end : s->buffer;
     int len             = s->buffer_size - (dst - s->buffer);