diff mbox series

[FFmpeg-devel,5/5] avformat/aviobuf: Also return truncated buffer in avio_get_dyn_buf()

Message ID 20200525140801.18889-5-andreas.rheinhardt@gmail.com
State Accepted
Commit c33e56c7a6a8bef7d95e1d36eb2f35748d475695
Headers show
Series [FFmpeg-devel,1/5] avformat/aviobuf: Don't check for overflow after it happened
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt May 25, 2020, 2:08 p.m. UTC
Two kinds of errors can happen when working with dynamic buffers:
(Re)allocation errors or truncation errors (one has to truncate the
buffer to a size of INT_MAX because avio_close_dyn_buf() and
avio_get_dyn_buf() both return an int). Right now, avio_get_dyn_buf()
returns an empty buffer in either case. But given that
avio_get_dyn_buf() does not destroy the dynamic buffer, one can return
the buffer in case of truncation and let the user check the error flags
and decide for himself instead of hardcoding a single way to proceed
in case of truncation.

(This actually restores the behaviour from before commit
163bb9ac0af495a5cb95441bdb5c02170440d28c.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Unfortunately the "let the user decide" approach is not possible for
avio_close_dyn_buf(). Said function has many more deficits:

On allocation failure it returns a size of
-AV_INPUT_BUFFER_PADDING_SIZE, although the documentation does not even
allow negative return values at all.
On truncation, the padding is not really written, yet it is subtracted
from the size (or to put it another way: the padding is not zeroed in
this case).
It does not return the actual size of the buffer, but rather the last
position (which can be different in case a backward seek had been
performed). In this case, it is not the end of the buffer that is padded,
but rather the position when calling avio_close_dyn_buf().

There is no documented way to check for errors; checking whether the
returned buffer is NULL works in case of allocation failures, but not
for truncation. If we returned NULL on truncation, too, callers could
check for errors. But a real solution would involve deprecating
avio_close_dyn_buf() and maybe the rest of the current dynamic buffer API.

 libavformat/aviobuf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff mbox series

Patch

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index a13c677875..66cc8c7adc 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -1370,13 +1370,13 @@  int avio_get_dyn_buf(AVIOContext *s, uint8_t **pbuffer)
 {
     DynBuffer *d;
 
-    if (!s || s->error) {
+    if (!s) {
         *pbuffer = NULL;
         return 0;
     }
     d = s->opaque;
 
-    if (!d->size) {
+    if (!s->error && !d->size) {
         *pbuffer = d->io_buffer;
         return FFMAX(s->buf_ptr, s->buf_ptr_max) - s->buffer;
     }