diff mbox series

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

Message ID 20211201113740.79002-1-lq@chinaffmpeg.org
State New
Headers show
Series [FFmpeg-devel,v2] fftools/ffmpeg_filter: fix SEGV in choose_pix_fmts after avio_close_dyn_buf
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed
andriy/make_ppc success Make finished
andriy/make_fate_ppc fail Make fate failed

Commit Message

Steven Liu Dec. 1, 2021, 11:37 a.m. UTC
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@tsinghua.edu.cn>
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 fftools/ffmpeg_filter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Anton Khirnov Dec. 1, 2021, 11:48 a.m. UTC | #1
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@tsinghua.edu.cn>
> Signed-off-by: Steven Liu <lq@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.
Andreas Rheinhardt Dec. 1, 2021, 12:56 p.m. UTC | #2
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@tsinghua.edu.cn>
>> Signed-off-by: Steven Liu <lq@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
diff mbox series

Patch

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;
         ret[len - 1] = 0;
         return ret;
     } else