diff mbox

[FFmpeg-devel] Fix buffer resizing after probe done.

Message ID 22abb200-60c1-cb64-6c09-4558718d7fa5@gmail.com
State New
Headers show

Commit Message

Artyom Lebedev Dec. 19, 2018, 12:40 p.m. UTC 
I have this assert fired after probing done and frames are normally 
read: "av_assert0(len >= s->orig_buffer_size);". Buffer size after 
probing ended up with ~5MB, orig_buffer_size was 64K. This place looks 
like total mess, it definitely has a bug with incorrect len modification 
even if buffer was not resize (failure to do so was ignored as well). I 
did not get what is the purpose of "s->buf_ptr != dst" check as well as 
this assert itself, so I removed them, this code really needs some 
comments there.

Comments

Carl Eugen Hoyos Dec. 19, 2018, 2:33 p.m. UTC | #1 
2018-12-19 13:40 GMT+01:00, Artyom Lebedev <vagran.ast@gmail.com>:
> I have this assert fired after probing done

How can we reproduce the assertion failure?

In general, fixing the crash and (potentially) removing
the assert() should be two different things.

Carl Eugen
Artyom Lebedev Dec. 20, 2018, 9:58 a.m. UTC | #2 
On 12/19/18 4:33 PM, Carl Eugen Hoyos wrote:
> How can we reproduce the assertion failure?

I do not have a scenario for ffmpeg command line utility, I have it in a 
proprietary project based on libav*, here I have custom I/O attached to 
AVFormatContext, initial buffer was 64K, input is MPEGTS stream, only 
MPEGTS and raw h264 demuxer are compiled, probing was ended up with 5MB 
buffer and the assert occurs when the buffer is almost fully depleted 
(after avformat_open_input(), avformat_find_stream_info(), and 
sequential av_read_frame() calls).
I suppose that the bug is quite obvious from the code, len is assigned 
to orig_buffer_size even if no trimming actually occurs (that was my case).
diff mbox

Patch

From 332b0059d75953868352eedbc866b112a81d7e9f Mon Sep 17 00:00:00 2001
From: Artyom Lebedev <vagran.ast@gmail.com>
Date: Wed, 19 Dec 2018 14:28:30 +0200
Subject: [PATCH] Fix buffer resizing after probe done.
To: ffmpeg-devel@ffmpeg.org

---
 libavformat/aviobuf.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index b867fdd..48ab197 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -550,7 +550,7 @@  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->buf_end - s->buffer + max_buffer_size <= s->buffer_size ?
                           s->buf_end : s->buffer;
     int len             = s->buffer_size - (dst - s->buffer);
 
@@ -570,16 +570,16 @@  static void fill_buffer(AVIOContext *s)
     }
 
     /* make buffer smaller in case it ended up large after probing */
-    if (s->read_packet && s->orig_buffer_size && s->buffer_size > s->orig_buffer_size) {
-        if (dst == s->buffer && s->buf_ptr != dst) {
-            int ret = ffio_set_buf_size(s, s->orig_buffer_size);
-            if (ret < 0)
-                av_log(s, AV_LOG_WARNING, "Failed to decrease buffer size\n");
+    if (s->read_packet && s->orig_buffer_size && s->buffer_size > s->orig_buffer_size &&
+        dst == s->buffer) {
 
+        int ret = ffio_set_buf_size(s, s->orig_buffer_size);
+        if (ret < 0) {
+            av_log(s, AV_LOG_WARNING, "Failed to decrease buffer size\n");
+        } else {
             s->checksum_ptr = dst = s->buffer;
+            len = s->orig_buffer_size;
         }
-        av_assert0(len >= s->orig_buffer_size);
-        len = s->orig_buffer_size;
     }
 
     len = read_packet_wrapper(s, dst, len);
-- 
2.7.4