[FFmpeg-devel] [PATCH 2/3] avformat/rtspdec: fix mem leaks on init fail

[Andriy Gelman]

On Sun, 11. Oct 22:44, Andreas Rheinhardt wrote:
> Andriy Gelman:
> > From: Andriy Gelman <andriy.gelman at gmail.com>
> > 
> > Fixes #6334
> > 
> > Signed-off-by: Andriy Gelman <andriy.gelman at 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,
-- 
Andriy