| 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 | expand |
| 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 |
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.
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 --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
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(-)