| Message ID | 20211207084235.42034-1-young_chelsea@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v3] fftools/opts: Avoid crash when opts could not be allocated | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Yu Yang: > If 'opts' could not be allocated, exiting the program to avoid crash when release it. > Before setup_find_stream_info_opts(), checking 'orig_nb_streams' is > 0. > If 'orig_nb_streams' == 0, it doesn't need 'opts' to getting streams info. So directly > execute avformat_find_stream_info(). > > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > Signed-off-by: Yu Yang <young_chelsea@163.com> > --- > fftools/cmdutils.c | 5 ++--- > fftools/ffmpeg_opt.c | 20 ++++++++++---------- > 2 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 3c8e5a82cd..823cc8a632 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -2181,13 +2181,12 @@ AVDictionary **setup_find_stream_info_opts(AVFormatContext *s, > int i; > AVDictionary **opts; > > - if (!s->nb_streams) > - return NULL; > 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; > + avformat_close_input(&s); > + exit_program(1); You went the easy route and exited instead of returning the error; I am not against this, but notice that you do not need to close s here, because we are exiting the program anyway; given that this function is not supposed to receive ownership of s at all, you should not free it. > } > for (i = 0; i < s->nb_streams; i++) > opts[i] = filter_codec_opts(codec_opts, s->streams[i]->codecpar->codec_id, > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c > index a703798586..453f3a21dc 100644 > --- a/fftools/ffmpeg_opt.c > +++ b/fftools/ffmpeg_opt.c > @@ -1191,17 +1191,17 @@ static int open_input_file(OptionsContext *o, const char *filename) > choose_decoder(o, ic, ic->streams[i]); > > if (find_stream_info) { > - AVDictionary **opts = setup_find_stream_info_opts(ic, o->g->codec_opts); > int orig_nb_streams = ic->nb_streams; > - > - /* If not enough info to get the stream parameters, we decode the > - first frames to get it. (used in mpeg case for example) */ > - ret = avformat_find_stream_info(ic, opts); > - > - for (i = 0; i < orig_nb_streams; i++) > - av_dict_free(&opts[i]); > - av_freep(&opts); > - > + if (orig_nb_streams > 0) { > + AVDictionary **opts = setup_find_stream_info_opts(ic, o->g->codec_opts); > + /* If not enough info to get the stream parameters, we decode the > + first frames to get it. (used in mpeg case for example) */ > + ret = avformat_find_stream_info(ic, opts); > + for (i = 0; i < orig_nb_streams; i++) > + av_dict_free(&opts[i]); > + av_freep(&opts); > + } else > + ret = avformat_find_stream_info(ic, NULL); This change makes the code way more uglier than it is. > if (ret < 0) { > av_log(NULL, AV_LOG_FATAL, "%s: could not find codec parameters\n", filename); > if (ic->nb_streams == 0) { >
> 2021年12月7日 下午5:00，Andreas Rheinhardt <andreas.rheinhardt@outlook.com> 写道： > > Yu Yang: >> If 'opts' could not be allocated, exiting the program to avoid crash when release it. >> Before setup_find_stream_info_opts(), checking 'orig_nb_streams' is > 0. >> If 'orig_nb_streams' == 0, it doesn't need 'opts' to getting streams info. So directly >> execute avformat_find_stream_info(). >> >> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >> Signed-off-by: Yu Yang <young_chelsea@163.com> >> --- >> fftools/cmdutils.c | 5 ++--- >> fftools/ffmpeg_opt.c | 20 ++++++++++---------- >> 2 files changed, 12 insertions(+), 13 deletions(-) >> >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c >> index 3c8e5a82cd..823cc8a632 100644 >> --- a/fftools/cmdutils.c >> +++ b/fftools/cmdutils.c >> @@ -2181,13 +2181,12 @@ AVDictionary **setup_find_stream_info_opts(AVFormatContext *s, >> int i; >> AVDictionary **opts; >> >> - if (!s->nb_streams) >> - return NULL; >> 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; >> + avformat_close_input(&s); >> + exit_program(1); > > You went the easy route and exited instead of returning the error; Yes. We can found log_error has been written in setup_find_stream_info_opts(). If returning the error, it need add a ‘ret’ to receive and judge. I think may be it is unnecessary. And error msg ’not alloc memory for stream options’ should be printed where the error occurred. Of course, this part of code also has ret for receive the return of avformat_find_stream_info(). Maybe it can be reused. But I think judging the value of ret twice not a good idea. May I ask your idea is to return ‘AVERROR(ENOMEM)’ ？it need to change value returned. > I am not against this, but notice that you do not need to close s here, > because we are exiting the program anyway; given that this function is > not supposed to receive ownership of s at all, you should not free it. Thx. I learn more. > >> } >> for (i = 0; i < s->nb_streams; i++) >> opts[i] = filter_codec_opts(codec_opts, s->streams[i]->codecpar->codec_id, >> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c >> index a703798586..453f3a21dc 100644 >> --- a/fftools/ffmpeg_opt.c >> +++ b/fftools/ffmpeg_opt.c >> @@ -1191,17 +1191,17 @@ static int open_input_file(OptionsContext *o, const char *filename) >> choose_decoder(o, ic, ic->streams[i]); >> >> if (find_stream_info) { >> - AVDictionary **opts = setup_find_stream_info_opts(ic, o->g->codec_opts); >> int orig_nb_streams = ic->nb_streams; >> - >> - /* If not enough info to get the stream parameters, we decode the >> - first frames to get it. (used in mpeg case for example) */ >> - ret = avformat_find_stream_info(ic, opts); >> - >> - for (i = 0; i < orig_nb_streams; i++) >> - av_dict_free(&opts[i]); >> - av_freep(&opts); >> - >> + if (orig_nb_streams > 0) { >> + AVDictionary **opts = setup_find_stream_info_opts(ic, o->g->codec_opts); >> + /* If not enough info to get the stream parameters, we decode the >> + first frames to get it. (used in mpeg case for example) */ >> + ret = avformat_find_stream_info(ic, opts); >> + for (i = 0; i < orig_nb_streams; i++) >> + av_dict_free(&opts[i]); >> + av_freep(&opts); >> + } else >> + ret = avformat_find_stream_info(ic, NULL); > > This change makes the code way more uglier than it is. Yes, I found this change is unnecessary. If orig_nb_streams == 0, it doesn’t enter for loop to free ‘ops’. > >> if (ret < 0) { >> av_log(NULL, AV_LOG_FATAL, "%s: could not find codec parameters\n", filename); >> if (ic->nb_streams == 0) { >> > > _______________________________________________ > 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 3c8e5a82cd..823cc8a632 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -2181,13 +2181,12 @@ AVDictionary **setup_find_stream_info_opts(AVFormatContext *s, int i; AVDictionary **opts; - if (!s->nb_streams) - return NULL; 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; + avformat_close_input(&s); + 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/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index a703798586..453f3a21dc 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -1191,17 +1191,17 @@ static int open_input_file(OptionsContext *o, const char *filename) choose_decoder(o, ic, ic->streams[i]); if (find_stream_info) { - AVDictionary **opts = setup_find_stream_info_opts(ic, o->g->codec_opts); int orig_nb_streams = ic->nb_streams; - - /* If not enough info to get the stream parameters, we decode the - first frames to get it. (used in mpeg case for example) */ - ret = avformat_find_stream_info(ic, opts); - - for (i = 0; i < orig_nb_streams; i++) - av_dict_free(&opts[i]); - av_freep(&opts); - + if (orig_nb_streams > 0) { + AVDictionary **opts = setup_find_stream_info_opts(ic, o->g->codec_opts); + /* If not enough info to get the stream parameters, we decode the + first frames to get it. (used in mpeg case for example) */ + ret = avformat_find_stream_info(ic, opts); + for (i = 0; i < orig_nb_streams; i++) + av_dict_free(&opts[i]); + av_freep(&opts); + } else + ret = avformat_find_stream_info(ic, NULL); if (ret < 0) { av_log(NULL, AV_LOG_FATAL, "%s: could not find codec parameters\n", filename); if (ic->nb_streams == 0) {
If 'opts' could not be allocated, exiting the program to avoid crash when release it. Before setup_find_stream_info_opts(), checking 'orig_nb_streams' is > 0. If 'orig_nb_streams' == 0, it doesn't need 'opts' to getting streams info. So directly execute avformat_find_stream_info(). Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> Signed-off-by: Yu Yang <young_chelsea@163.com> --- fftools/cmdutils.c | 5 ++--- fftools/ffmpeg_opt.c | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 13 deletions(-)