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

[Andriy Gelman]

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 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).
> 

sorry, it is called actually.. I had a mistake in the counts. Please disregard
this comment.

-- 
Andriy