[FFmpeg-devel] [PATCH v2] fftools/ffmpeg_filter: fix SEGV in choose_pix_fmts after avio_close_dyn_buf

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Steven Liu (2021-12-01 12:37:40)
>> Check avio_printf value and len from avio_close_dyn_buf, it should
>> incorrect if they are not equal each other.
>>
>> Reported-by: TOTE Robot <oslab at tsinghua.edu.cn>
>> Signed-off-by: Steven Liu <lq at chinaffmpeg.org>
>> ---
>>  fftools/ffmpeg_filter.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
>> index 452b689d62..ceb08b44f1 100644
>> --- a/fftools/ffmpeg_filter.c
>> +++ b/fftools/ffmpeg_filter.c
>> @@ -105,6 +105,7 @@ static char *choose_pix_fmts(OutputFilter *ofilter)
>>          AVIOContext *s = NULL;
>>          uint8_t *ret;
>>          int len;
>> +        int name_new_size = 0;
>>  
>>          if (avio_open_dyn_buf(&s) < 0)
>>              exit_program(1);
>> @@ -116,9 +117,11 @@ static char *choose_pix_fmts(OutputFilter *ofilter)
>>  
>>          for (; *p != AV_PIX_FMT_NONE; p++) {
>>              const char *name = av_get_pix_fmt_name(*p);
>> -            avio_printf(s, "%s|", name);
>> +            name_new_size = avio_printf(s, "%s|", name);
>>          }
>>          len = avio_close_dyn_buf(s, &ret);
>> +        if (len != name_new_size)
>> +            return NULL;
> 
> This will be wrong if there is more than one pixel format.
> 
> I'd say this should just forward errors from avio_printf(). The doxy for
> avio_close_dyn_buf() says it returns the buffer lenght, implying it
> cannot fail.
> 

1. avio_printf() (at least currently) does not return write errors of
the underlying AVIOContext, but only errors that prevent the data to be
forwarded to AVIOContext (i.e. AVBPrint allocation errors). The
philosophy seems to be that the user can check the AVIOContext errors
himself.
2. avio_close_dyn_buf() is a horrible function which does not honour its
doxy in case of errors.
a) Basically all users of the dynamic buffer API are wrong in case of
allocation error (the Matroska muxer is an (the only?) exception: Using
avio_get_dyn_buf() instead of avio_close_dyn_buf() allows to check for
errors).
b) In particular, the signature of avio_close_dyn_buf() does make it
easy to inform the user that the buffer is truncated in order to let him
decide what to do.
c) Up until a few minutes ago I thought it to be impossible to fix this
without changing the signature of avio_close_dyn_buf(). But I just had a
crazy idea, don't know whether it works at all: We remove the small
internal buffer of a dynbuf altogether and write directly into the final
buffer; every time the internal buffer is reallocated, dyn_buf_write()
reallocates the buffer and modifies the buffer pointers in the
AVIOContext. This has the downside that one looses the "small buffer"
optimization (where the data written fits completely within the current
small (1024B) buffer), but there would be no more copies for ordinary
data, promising speedups (some of which are already attainable by
setting the direct flag).
3. Anyway, this code should use an AVBPrint: It does not even need
a permanent string at all and the data fits nicely into the internal
cache. Will send a patch for that.

- Andreas