| Message ID | 20201011190330.39991-2-andriy.gelman@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/rtspdec: set dangling pointers to NULL | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
Andriy Gelman: > From: Andriy Gelman <andriy.gelman@gmail.com> > > Fixes #6334 > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > --- > libavformat/rtspdec.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c > index b519b6f1a2..623f478585 100644 > --- a/libavformat/rtspdec.c > +++ b/libavformat/rtspdec.c > @@ -720,7 +720,7 @@ static int rtsp_read_header(AVFormatContext *s) > if (rt->rtsp_flags & RTSP_FLAG_LISTEN) { > ret = rtsp_listen(s); > if (ret) > - return ret; > + goto fail; This will add one ff_network_close() to this codepath. Is it really certain that there was a corresponding ff_network_init()? (And where is the ff_network_init() that is cancelled in rtsp_read_close() anyway?) > } else { > ret = ff_rtsp_connect(s); > if (ret) > @@ -728,22 +728,25 @@ static int rtsp_read_header(AVFormatContext *s) > > rt->real_setup_cache = !s->nb_streams ? NULL : > av_mallocz_array(s->nb_streams, 2 * sizeof(*rt->real_setup_cache)); > - if (!rt->real_setup_cache && s->nb_streams) > - return AVERROR(ENOMEM); > + if (!rt->real_setup_cache && s->nb_streams) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } With your patch, the calls to ff_network_init() and ff_network_close() cancel each other out if the above allocation fails. > rt->real_setup = rt->real_setup_cache + s->nb_streams; > > if (rt->initial_pause) { > /* do not start immediately */ > } else { > if ((ret = rtsp_read_play(s)) < 0) { > - ff_rtsp_close_streams(s); > - ff_rtsp_close_connections(s); > - return ret; > + goto fail; This will add a TEARDOWN command to this and the above error path. Is this something good? > } > } > } > - > return 0; > + > +fail: > + rtsp_read_close(s); > + return ret; > } > > int ff_rtsp_tcp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st, >
On Sun, 11. Oct 22:44, Andreas Rheinhardt wrote: > Andriy Gelman: > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > Fixes #6334 > > > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > > --- > > libavformat/rtspdec.c | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c > > index b519b6f1a2..623f478585 100644 > > --- a/libavformat/rtspdec.c > > +++ b/libavformat/rtspdec.c > > @@ -720,7 +720,7 @@ static int rtsp_read_header(AVFormatContext *s) > > if (rt->rtsp_flags & RTSP_FLAG_LISTEN) { > > ret = rtsp_listen(s); > > if (ret) > > - return ret; > > + goto fail; > > This will add one ff_network_close() to this codepath. Is it really > certain that there was a corresponding ff_network_init()? > (And where is > the ff_network_init() that is cancelled in rtsp_read_close() anyway?) Besides my patch, there is an extra ff_network_init() in ff_rtsp_connect(), which is missing from rtsp_listen(). This means there'll be an extra ff_network_close() call in listen mode (when the stream exits without errors). I think the best solution is to remove ff_network_init() from ff_rtsp_connect() and then remove ff_network_close() from rtsp_read_close() and the fail path of ff_rtsp_connect(). Calling ff_network_init() separately in ff_rtsp_connect() seems redundant because the function is called in each url_alloc_for_protocol() anyway.. and it only complicates the cleanup after init failure. Something else I noticed when debugging the code: From the ffmpeg executable, avformat_network_deinit() is not called in the error path when rtsp_read_header() fails. This means we'll have an extra ff_network_init() call after the code exits following an error rtsp_read_header(). I'm not sure if this is an issue for windows users... (i.e. if it leaves some socket resources open), or whether all the resources are automatically closed when the ffmpeg binary exits. Would it make sense to add an avformat_network_deinit() in the error path (maybe around ffmpeg_opt.c:1167?) > > > } else { > > ret = ff_rtsp_connect(s); > > if (ret) > > @@ -728,22 +728,25 @@ static int rtsp_read_header(AVFormatContext *s) > > > > rt->real_setup_cache = !s->nb_streams ? NULL : > > av_mallocz_array(s->nb_streams, 2 * sizeof(*rt->real_setup_cache)); > > - if (!rt->real_setup_cache && s->nb_streams) > > - return AVERROR(ENOMEM); > > + if (!rt->real_setup_cache && s->nb_streams) { > > + ret = AVERROR(ENOMEM); > > + goto fail; > > + } > > With your patch, the calls to ff_network_init() and ff_network_close() > cancel each other out if the above allocation fails. > Yes, if libavformat is linked to the executable. All the ff_network_init()/ff_network_close() should cancel each other out. But from the ffmpeg binary we'll have +1 call to ff_network_init(), because avformat_network_deinit() is skipped in ffmpeg's error path (but avforamt_network_init() is still called). > > rt->real_setup = rt->real_setup_cache + s->nb_streams; > > > > if (rt->initial_pause) { > > /* do not start immediately */ > > } else { > > if ((ret = rtsp_read_play(s)) < 0) { > > - ff_rtsp_close_streams(s); > > - ff_rtsp_close_connections(s); > > - return ret; > > + goto fail; > > This will add a TEARDOWN command to this and the above error path. Is > this something good? > The teardown tells the server to shutdown all the resources with this session after the error. To me this is correct behavior. Thanks,
On Sun, 11. Oct 22:04, Andriy Gelman wrote: > On Sun, 11. Oct 22:44, Andreas Rheinhardt wrote: > > Andriy Gelman: > > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > > > Fixes #6334 > > > > > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > > > --- > > > libavformat/rtspdec.c | 17 ++++++++++------- > > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c > > > index b519b6f1a2..623f478585 100644 > > > --- a/libavformat/rtspdec.c > > > +++ b/libavformat/rtspdec.c > > > @@ -720,7 +720,7 @@ static int rtsp_read_header(AVFormatContext *s) > > > if (rt->rtsp_flags & RTSP_FLAG_LISTEN) { > > > ret = rtsp_listen(s); > > > if (ret) > > > - return ret; > > > + goto fail; > > > > > This will add one ff_network_close() to this codepath. Is it really > > certain that there was a corresponding ff_network_init()? > > (And where is > > the ff_network_init() that is cancelled in rtsp_read_close() anyway?) > > Besides my patch, there is an extra ff_network_init() in ff_rtsp_connect(), > which is missing from rtsp_listen(). > > This means there'll be an extra ff_network_close() call in listen mode (when the > stream exits without errors). > > I think the best solution is to remove ff_network_init() from ff_rtsp_connect() > and then remove ff_network_close() from rtsp_read_close() and the fail path of > ff_rtsp_connect(). > > Calling ff_network_init() separately in ff_rtsp_connect() seems redundant > because the function is called in each url_alloc_for_protocol() anyway.. and it > only complicates the cleanup after init failure. > Seems ff_url_join() which is called before url_alloc_for_protocol() needs to ensure ff_network_init() has been called. The above approach will not work then.
On Sun, 11. Oct 22:04, Andriy Gelman wrote: > On Sun, 11. Oct 22:44, Andreas Rheinhardt wrote: > > Andriy Gelman: > > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > > > Fixes #6334 > > > > > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > > > --- > > > libavformat/rtspdec.c | 17 ++++++++++------- > > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c > > > index b519b6f1a2..623f478585 100644 > > > --- a/libavformat/rtspdec.c > > > +++ b/libavformat/rtspdec.c > > > @@ -720,7 +720,7 @@ static int rtsp_read_header(AVFormatContext *s) > > > if (rt->rtsp_flags & RTSP_FLAG_LISTEN) { > > > ret = rtsp_listen(s); > > > if (ret) > > > - return ret; > > > + goto fail; > > > > > This will add one ff_network_close() to this codepath. Is it really > > certain that there was a corresponding ff_network_init()? > > (And where is > > the ff_network_init() that is cancelled in rtsp_read_close() anyway?) > > Besides my patch, there is an extra ff_network_init() in ff_rtsp_connect(), > which is missing from rtsp_listen(). > > This means there'll be an extra ff_network_close() call in listen mode (when the > stream exits without errors). > > I think the best solution is to remove ff_network_init() from ff_rtsp_connect() > and then remove ff_network_close() from rtsp_read_close() and the fail path of > ff_rtsp_connect(). > > Calling ff_network_init() separately in ff_rtsp_connect() seems redundant > because the function is called in each url_alloc_for_protocol() anyway.. and it > only complicates the cleanup after init failure. > > Something else I noticed when debugging the code: > From the ffmpeg executable, > avformat_network_deinit() is not called in the error path when > rtsp_read_header() fails. This means we'll have an extra ff_network_init() call > after the code exits following an error rtsp_read_header(). > > I'm not sure if this is an issue for windows users... (i.e. if it leaves some > socket resources open), or whether all the resources are automatically closed > when the ffmpeg binary exits. > Would it make sense to add an avformat_network_deinit() in the error path (maybe > around ffmpeg_opt.c:1167?) > > > > > > } else { > > > ret = ff_rtsp_connect(s); > > > if (ret) > > > @@ -728,22 +728,25 @@ static int rtsp_read_header(AVFormatContext *s) > > > > > > rt->real_setup_cache = !s->nb_streams ? NULL : > > > av_mallocz_array(s->nb_streams, 2 * sizeof(*rt->real_setup_cache)); > > > - if (!rt->real_setup_cache && s->nb_streams) > > > - return AVERROR(ENOMEM); > > > + if (!rt->real_setup_cache && s->nb_streams) { > > > + ret = AVERROR(ENOMEM); > > > + goto fail; > > > + } > > > > > With your patch, the calls to ff_network_init() and ff_network_close() > > cancel each other out if the above allocation fails. > > > > Yes, if libavformat is linked to the executable. All the > ff_network_init()/ff_network_close() should cancel each other out. > > But from the ffmpeg binary we'll have +1 call to ff_network_init(), because > avformat_network_deinit() is skipped in ffmpeg's error path (but > avforamt_network_init() is still called). > sorry, it is called actually.. I had a mistake in the counts. Please disregard this comment.
diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c index b519b6f1a2..623f478585 100644 --- a/libavformat/rtspdec.c +++ b/libavformat/rtspdec.c @@ -720,7 +720,7 @@ static int rtsp_read_header(AVFormatContext *s) if (rt->rtsp_flags & RTSP_FLAG_LISTEN) { ret = rtsp_listen(s); if (ret) - return ret; + goto fail; } else { ret = ff_rtsp_connect(s); if (ret) @@ -728,22 +728,25 @@ static int rtsp_read_header(AVFormatContext *s) rt->real_setup_cache = !s->nb_streams ? NULL : av_mallocz_array(s->nb_streams, 2 * sizeof(*rt->real_setup_cache)); - if (!rt->real_setup_cache && s->nb_streams) - return AVERROR(ENOMEM); + if (!rt->real_setup_cache && s->nb_streams) { + ret = AVERROR(ENOMEM); + goto fail; + } rt->real_setup = rt->real_setup_cache + s->nb_streams; if (rt->initial_pause) { /* do not start immediately */ } else { if ((ret = rtsp_read_play(s)) < 0) { - ff_rtsp_close_streams(s); - ff_rtsp_close_connections(s); - return ret; + goto fail; } } } - return 0; + +fail: + rtsp_read_close(s); + return ret; } int ff_rtsp_tcp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,