| Message ID | AS8P250MB0744190D486E62CDB1C6E9EA8F522@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 44620ade251f705b1395d91a540c51768b2d9c8b |
| Headers | show |
| Series | [FFmpeg-devel,1/2] fftools/ffmpeg_mux_init: Fix attachment_filename use-after-free | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting Andreas Rheinhardt (2024-02-18 16:10:06) > The filename is freed with the OptionsContext and therefore > there will be a use-after-free when reporting the filename > in print_stream_maps(). So create a copy of the string. > > This is a regression since 8aed3911fc454e79697e183660bf30d31334a64b. > fate-lavf-mkv_attachment exhibits it (and reports a random nonsense > filename here), but this does not make the test fail (not even with > valgrind; only with ASAN, as it aborts on use-after-free). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > fftools/ffmpeg.h | 2 +- > fftools/ffmpeg_mux.c | 2 ++ > fftools/ffmpeg_mux_init.c | 10 +++++++++- > 3 files changed, 12 insertions(+), 2 deletions(-) Ok
diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h index 33750e0bb3..c394f60962 100644 --- a/fftools/ffmpeg.h +++ b/fftools/ffmpeg.h @@ -555,7 +555,7 @@ typedef struct OutputStream { AVDictionary *swr_opts; char *apad; - const char *attachment_filename; + char *attachment_filename; int keep_pix_fmt; diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c index e65fe89992..5a648c0568 100644 --- a/fftools/ffmpeg_mux.c +++ b/fftools/ffmpeg_mux.c @@ -817,6 +817,8 @@ static void ost_free(OutputStream **post) av_freep(&ost->logfile_prefix); av_freep(&ost->apad); + av_freep(&ost->attachment_filename); + #if FFMPEG_OPT_MAP_CHANNEL av_freep(&ost->audio_channels_map); ost->audio_channels_mapped = 0; diff --git a/fftools/ffmpeg_mux_init.c b/fftools/ffmpeg_mux_init.c index 0718487c53..1abbb2d945 100644 --- a/fftools/ffmpeg_mux_init.c +++ b/fftools/ffmpeg_mux_init.c @@ -1741,6 +1741,7 @@ static int of_add_attachments(Muxer *mux, const OptionsContext *o) for (int i = 0; i < o->nb_attachments; i++) { AVIOContext *pb; uint8_t *attachment; + char *attachment_filename; const char *p; int64_t len; @@ -1788,13 +1789,20 @@ read_fail: av_log(mux, AV_LOG_VERBOSE, "Creating attachment stream from file %s\n", o->attachments[i]); + attachment_filename = av_strdup(o->attachments[i]); + if (!attachment_filename) { + av_free(attachment); + return AVERROR(ENOMEM); + } + err = ost_add(mux, o, AVMEDIA_TYPE_ATTACHMENT, NULL, NULL, &ost); if (err < 0) { + av_free(attachment_filename); av_freep(&attachment); return err; } - ost->attachment_filename = o->attachments[i]; + ost->attachment_filename = attachment_filename; ost->par_in->extradata = attachment; ost->par_in->extradata_size = len;
The filename is freed with the OptionsContext and therefore there will be a use-after-free when reporting the filename in print_stream_maps(). So create a copy of the string. This is a regression since 8aed3911fc454e79697e183660bf30d31334a64b. fate-lavf-mkv_attachment exhibits it (and reports a random nonsense filename here), but this does not make the test fail (not even with valgrind; only with ASAN, as it aborts on use-after-free). Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- fftools/ffmpeg.h | 2 +- fftools/ffmpeg_mux.c | 2 ++ fftools/ffmpeg_mux_init.c | 10 +++++++++- 3 files changed, 12 insertions(+), 2 deletions(-)