| Message ID | 20211204081800.95539-1-young_chelsea@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] fftools/cmdutils: Avoid crash when opts is empty | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | fail | Make fate failed |
| andriy/makex86 | warning | New warnings during build |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | fail | Make fate failed |
Yu Yang: > Opts is assigned by setup_find_stream_info_opts(). Could not get opts when nb_streams == 0. > It should not return NULL but print AV_LOG_ERROR. when no alloc memory for stream options, > it also need return an error to avoid crash when free. In total, setup_find_stream_info_opts() > should not return NULL. It print AV_LOG_ERROR or correct value. > > coredump backtrace info: > ==6235==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000006ba9c2f bp 0x7ffc3d5baa30 sp 0x7ffc3d5ba9a0 T0) > ==6235==The signal is caused by a READ memory access. > ==6235==Hint: address points to the zero page. > #0 0x6ba9c2f in av_dict_free /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavutil/dict.c:205:23 > #1 0x4ce5ac in open_input_file /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:1199:13 > #2 0x4c9dc0 in open_files /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3338:15 > #3 0x4c9295 in ffmpeg_parse_options /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3378:11 > #4 0x58f241 in main /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4988:11 > #5 0x7fe35197f0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16 > #6 0x42033d in _start (/home/r1/ffmpeg/ffmpeg_4.4.1+0x42033d) > > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > Signed-off-by: Yu Yang <young_chelsea@163.com> > --- > fftools/cmdutils.c | 6 ++++-- > libavformat/demux.c | 12 +++++------- > 2 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 45322f8c71..f4333d8b65 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -2182,12 +2182,14 @@ AVDictionary **setup_find_stream_info_opts(AVFormatContext *s, > AVDictionary **opts; > > if (!s->nb_streams) > - return NULL; > + av_log(NULL, AV_LOG_ERROR, > + "No stream exists, Could not get stream options.\n"); > + exit_program(1); This is completely wrong: You are unconditionally exiting the program (the indentation here is now misleading). Had you run FATE (see https://ffmpeg.org/fate.html), you would have noticed this. Exiting the program in case of no streams is wrong, too. > opts = av_calloc(s->nb_streams, sizeof(*opts)); > if (!opts) { > av_log(NULL, AV_LOG_ERROR, > "Could not alloc memory for stream options.\n"); > - return NULL; > + exit_program(1); > } > for (i = 0; i < s->nb_streams; i++) > opts[i] = filter_codec_opts(codec_opts, s->streams[i]->codecpar->codec_id, > diff --git a/libavformat/demux.c b/libavformat/demux.c > index 745dc8687c..0738ef2e73 100644 > --- a/libavformat/demux.c > +++ b/libavformat/demux.c > @@ -2434,7 +2434,7 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) > > for (unsigned i = 0; i < ic->nb_streams; i++) { > const AVCodec *codec; > - AVDictionary *thread_opt = NULL; > + > AVStream *const st = ic->streams[i]; > FFStream *const sti = ffstream(st); > AVCodecContext *const avctx = sti->avctx; > @@ -2474,26 +2474,24 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) > > /* Force thread count to 1 since the H.264 decoder will not extract > * SPS and PPS to extradata during multi-threaded decoding. */ > - av_dict_set(options ? &options[i] : &thread_opt, "threads", "1", 0); > + av_dict_set(&options[i], "threads", "1", 0); > /* Force lowres to 0. The decoder might reduce the video size by the > * lowres factor, and we don't want that propagated to the stream's > * codecpar */ > - av_dict_set(options ? &options[i] : &thread_opt, "lowres", "0", 0); > + av_dict_set(&options[i], "lowres", "0", 0); > > if (ic->codec_whitelist) > - av_dict_set(options ? &options[i] : &thread_opt, "codec_whitelist", ic->codec_whitelist, 0); > + av_dict_set(&options[i], "codec_whitelist", ic->codec_whitelist, 0); > > // Try to just open decoders, in case this is enough to get parameters. > // Also ensure that subtitle_header is properly set. > if (!has_codec_parameters(st, NULL) && sti->request_probe <= 0 || > st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) { > if (codec && !avctx->codec) > - if (avcodec_open2(avctx, codec, options ? &options[i] : &thread_opt) < 0) > + if (avcodec_open2(avctx, codec, &options[i]) < 0) > av_log(ic, AV_LOG_WARNING, > "Failed to open codec in %s\n",__FUNCTION__); > } > - if (!options) > - av_dict_free(&thread_opt); > } > > read_size = 0; > These changes to libavformat are even worse: Even if all the users in fftools always set options does not mean that all users do; it is not required. Mind if I take over and fix this? - Andreas
> 2021年12月5日 下午6:56，Andreas Rheinhardt <andreas.rheinhardt@outlook.com> 写道： > > Yu Yang: >> Opts is assigned by setup_find_stream_info_opts(). Could not get opts when nb_streams == 0. >> It should not return NULL but print AV_LOG_ERROR. when no alloc memory for stream options, >> it also need return an error to avoid crash when free. In total, setup_find_stream_info_opts() >> should not return NULL. It print AV_LOG_ERROR or correct value. >> >> coredump backtrace info: >> ==6235==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000006ba9c2f bp 0x7ffc3d5baa30 sp 0x7ffc3d5ba9a0 T0) >> ==6235==The signal is caused by a READ memory access. >> ==6235==Hint: address points to the zero page. >> #0 0x6ba9c2f in av_dict_free /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavutil/dict.c:205:23 >> #1 0x4ce5ac in open_input_file /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:1199:13 >> #2 0x4c9dc0 in open_files /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3338:15 >> #3 0x4c9295 in ffmpeg_parse_options /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3378:11 >> #4 0x58f241 in main /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4988:11 >> #5 0x7fe35197f0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16 >> #6 0x42033d in _start (/home/r1/ffmpeg/ffmpeg_4.4.1+0x42033d) >> >> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >> Signed-off-by: Yu Yang <young_chelsea@163.com> >> --- >> fftools/cmdutils.c | 6 ++++-- >> libavformat/demux.c | 12 +++++------- >> 2 files changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c >> index 45322f8c71..f4333d8b65 100644 >> --- a/fftools/cmdutils.c >> +++ b/fftools/cmdutils.c >> @@ -2182,12 +2182,14 @@ AVDictionary **setup_find_stream_info_opts(AVFormatContext *s, >> AVDictionary **opts; >> >> if (!s->nb_streams) >> - return NULL; >> + av_log(NULL, AV_LOG_ERROR, >> + "No stream exists, Could not get stream options.\n"); >> + exit_program(1); > > This is completely wrong: You are unconditionally exiting the program > (the indentation here is now misleading). Had you run FATE (see > https://ffmpeg.org/fate.html), you would have noticed this. I forgot to run 'make fate'. > Exiting the program in case of no streams is wrong, too. OK, got it. > >> opts = av_calloc(s->nb_streams, sizeof(*opts)); >> if (!opts) { >> av_log(NULL, AV_LOG_ERROR, >> "Could not alloc memory for stream options.\n"); >> - return NULL; >> + exit_program(1); >> } >> for (i = 0; i < s->nb_streams; i++) >> opts[i] = filter_codec_opts(codec_opts, s->streams[i]->codecpar->codec_id, >> diff --git a/libavformat/demux.c b/libavformat/demux.c >> index 745dc8687c..0738ef2e73 100644 >> --- a/libavformat/demux.c >> +++ b/libavformat/demux.c >> @@ -2434,7 +2434,7 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) >> >> for (unsigned i = 0; i < ic->nb_streams; i++) { >> const AVCodec *codec; >> - AVDictionary *thread_opt = NULL; >> + >> AVStream *const st = ic->streams[i]; >> FFStream *const sti = ffstream(st); >> AVCodecContext *const avctx = sti->avctx; >> @@ -2474,26 +2474,24 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) >> >> /* Force thread count to 1 since the H.264 decoder will not extract >> * SPS and PPS to extradata during multi-threaded decoding. */ >> - av_dict_set(options ? &options[i] : &thread_opt, "threads", "1", 0); >> + av_dict_set(&options[i], "threads", "1", 0); >> /* Force lowres to 0. The decoder might reduce the video size by the >> * lowres factor, and we don't want that propagated to the stream's >> * codecpar */ >> - av_dict_set(options ? &options[i] : &thread_opt, "lowres", "0", 0); >> + av_dict_set(&options[i], "lowres", "0", 0); >> >> if (ic->codec_whitelist) >> - av_dict_set(options ? &options[i] : &thread_opt, "codec_whitelist", ic->codec_whitelist, 0); >> + av_dict_set(&options[i], "codec_whitelist", ic->codec_whitelist, 0); >> >> // Try to just open decoders, in case this is enough to get parameters. >> // Also ensure that subtitle_header is properly set. >> if (!has_codec_parameters(st, NULL) && sti->request_probe <= 0 || >> st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) { >> if (codec && !avctx->codec) >> - if (avcodec_open2(avctx, codec, options ? &options[i] : &thread_opt) < 0) >> + if (avcodec_open2(avctx, codec, &options[i]) < 0) >> av_log(ic, AV_LOG_WARNING, >> "Failed to open codec in %s\n",__FUNCTION__); >> } >> - if (!options) >> - av_dict_free(&thread_opt); >> } >> >> read_size = 0; >> > > These changes to libavformat are even worse: Even if all the users in > fftools always set options does not mean that all users do; it is not > required. I understand what you mean now.I will look at this part of the code tomorrow. Thank you for your reply in the email. > Mind if I take over and fix this? I really want to fix it myself. I use ffmpeg not long time. But I think this is a good chance for me to learn it. I will try my best to fix this. Thank you very much. > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 45322f8c71..f4333d8b65 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -2182,12 +2182,14 @@ AVDictionary **setup_find_stream_info_opts(AVFormatContext *s, AVDictionary **opts; if (!s->nb_streams) - return NULL; + av_log(NULL, AV_LOG_ERROR, + "No stream exists, Could not get stream options.\n"); + exit_program(1); opts = av_calloc(s->nb_streams, sizeof(*opts)); if (!opts) { av_log(NULL, AV_LOG_ERROR, "Could not alloc memory for stream options.\n"); - return NULL; + exit_program(1); } for (i = 0; i < s->nb_streams; i++) opts[i] = filter_codec_opts(codec_opts, s->streams[i]->codecpar->codec_id, diff --git a/libavformat/demux.c b/libavformat/demux.c index 745dc8687c..0738ef2e73 100644 --- a/libavformat/demux.c +++ b/libavformat/demux.c @@ -2434,7 +2434,7 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) for (unsigned i = 0; i < ic->nb_streams; i++) { const AVCodec *codec; - AVDictionary *thread_opt = NULL; + AVStream *const st = ic->streams[i]; FFStream *const sti = ffstream(st); AVCodecContext *const avctx = sti->avctx; @@ -2474,26 +2474,24 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options) /* Force thread count to 1 since the H.264 decoder will not extract * SPS and PPS to extradata during multi-threaded decoding. */ - av_dict_set(options ? &options[i] : &thread_opt, "threads", "1", 0); + av_dict_set(&options[i], "threads", "1", 0); /* Force lowres to 0. The decoder might reduce the video size by the * lowres factor, and we don't want that propagated to the stream's * codecpar */ - av_dict_set(options ? &options[i] : &thread_opt, "lowres", "0", 0); + av_dict_set(&options[i], "lowres", "0", 0); if (ic->codec_whitelist) - av_dict_set(options ? &options[i] : &thread_opt, "codec_whitelist", ic->codec_whitelist, 0); + av_dict_set(&options[i], "codec_whitelist", ic->codec_whitelist, 0); // Try to just open decoders, in case this is enough to get parameters. // Also ensure that subtitle_header is properly set. if (!has_codec_parameters(st, NULL) && sti->request_probe <= 0 || st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) { if (codec && !avctx->codec) - if (avcodec_open2(avctx, codec, options ? &options[i] : &thread_opt) < 0) + if (avcodec_open2(avctx, codec, &options[i]) < 0) av_log(ic, AV_LOG_WARNING, "Failed to open codec in %s\n",__FUNCTION__); } - if (!options) - av_dict_free(&thread_opt); } read_size = 0;
Opts is assigned by setup_find_stream_info_opts(). Could not get opts when nb_streams == 0. It should not return NULL but print AV_LOG_ERROR. when no alloc memory for stream options, it also need return an error to avoid crash when free. In total, setup_find_stream_info_opts() should not return NULL. It print AV_LOG_ERROR or correct value. coredump backtrace info: ==6235==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000006ba9c2f bp 0x7ffc3d5baa30 sp 0x7ffc3d5ba9a0 T0) ==6235==The signal is caused by a READ memory access. ==6235==Hint: address points to the zero page. #0 0x6ba9c2f in av_dict_free /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavutil/dict.c:205:23 #1 0x4ce5ac in open_input_file /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:1199:13 #2 0x4c9dc0 in open_files /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3338:15 #3 0x4c9295 in ffmpeg_parse_options /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3378:11 #4 0x58f241 in main /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4988:11 #5 0x7fe35197f0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16 #6 0x42033d in _start (/home/r1/ffmpeg/ffmpeg_4.4.1+0x42033d) Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> Signed-off-by: Yu Yang <young_chelsea@163.com> --- fftools/cmdutils.c | 6 ++++-- libavformat/demux.c | 12 +++++------- 2 files changed, 9 insertions(+), 9 deletions(-)