| Message ID | 20201011190330.39991-1-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> > > Otherwise a double free will occur in case rtsp_read_close() is called > on error. > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > --- > libavformat/rtspdec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c > index ef084a8b2b..b519b6f1a2 100644 > --- a/libavformat/rtspdec.c > +++ b/libavformat/rtspdec.c > @@ -691,7 +691,8 @@ static int rtsp_listen(AVFormatContext *s) > } else if (methodcode == SETUP) > ret = rtsp_read_setup(s, host, uri); > if (ret) { > - ffurl_close(rt->rtsp_hd); > + ffurl_closep(&rt->rtsp_hd); > + rt->rtsp_hd_out = NULL; > return AVERROR_INVALIDDATA; > } > } > You simply set rtsp_hd_out to NULL, so I presume you have checked that it really always pointed to NULL or to rtsp_hd (i.e. that everything is consistent) before that. In this case, you don't need new code for freeing here: Remove the ffurl_close() that is already here and leave freeing to rtsp_read_close() (and ultimately to ff_rtsp_close_connections()). This advice is untested, so better test it. - Andreas PS: When I made 82bf41f3abce4a13e7c6ad1606eb708f371de87f I intentionally left rtspdec out and put it on my (pretty long) "I'll-do-it-tomorrow"-list, because of this rtsp_hd vs. rtsp_hd_out stuff. Thanks for taking care of it.
On Sun, 11. Oct 21:28, Andreas Rheinhardt wrote: > Andriy Gelman: > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > Otherwise a double free will occur in case rtsp_read_close() is called > > on error. > > > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > > --- > > libavformat/rtspdec.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c > > index ef084a8b2b..b519b6f1a2 100644 > > --- a/libavformat/rtspdec.c > > +++ b/libavformat/rtspdec.c > > @@ -691,7 +691,8 @@ static int rtsp_listen(AVFormatContext *s) > > } else if (methodcode == SETUP) > > ret = rtsp_read_setup(s, host, uri); > > if (ret) { > > - ffurl_close(rt->rtsp_hd); > > + ffurl_closep(&rt->rtsp_hd); > > + rt->rtsp_hd_out = NULL; > > return AVERROR_INVALIDDATA; > > } > > } > > > You simply set rtsp_hd_out to NULL, so I presume you have checked that > it really always pointed to NULL or to rtsp_hd (i.e. that everything is > consistent) before that. In this case, you don't need new code for > freeing here: Remove the ffurl_close() that is already here and leave > freeing to rtsp_read_close() (and ultimately to > ff_rtsp_close_connections()). This advice is untested, so better test it. Yes, in listen mode rtsp_hd = rtsp_hd_out. A separate allocation is done in connect mode. I'll add your suggestion to the second patch and drop this one. Thanks,
diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c index ef084a8b2b..b519b6f1a2 100644 --- a/libavformat/rtspdec.c +++ b/libavformat/rtspdec.c @@ -691,7 +691,8 @@ static int rtsp_listen(AVFormatContext *s) } else if (methodcode == SETUP) ret = rtsp_read_setup(s, host, uri); if (ret) { - ffurl_close(rt->rtsp_hd); + ffurl_closep(&rt->rtsp_hd); + rt->rtsp_hd_out = NULL; return AVERROR_INVALIDDATA; } }