| Message ID | 20170515164710.58582-1-robertmeyers@google.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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".
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 >
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. [...]
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
>
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 [...]
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);