[FFmpeg-devel] [PATCH v2] fftools/cmdutils: Avoid crash when opts is empty

[Andreas Rheinhardt]

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 at tsinghua.edu.cn>
> Signed-off-by: Yu Yang <young_chelsea at 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