| Message ID | 20201012203609.41647-6-andriy.gelman@gmail.com |
|---|---|
| State | Accepted |
| Commit | 4fe9e2fc162a9d3258eb39dd16677970c657c122 |
| Headers | show |
| Series | [FFmpeg-devel,v3,1/6] avformat/rtspdec: add network init to listen mode | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | warning | Make failed |
On Mon, 12 Oct 2020, Andriy Gelman wrote: > From: Andriy Gelman <andriy.gelman@gmail.com> > > In sdp_read_header() some ff_network_close() calls were missed. > > Also in rtp_read_header() update comment to explain why a single > call to ff_network_close() is enough to cover all cases even if > sdp_read_header() returns an error. > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > --- > libavformat/rtsp.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > index cb9fc31166..a8c7ec4a46 100644 > --- a/libavformat/rtsp.c > +++ b/libavformat/rtsp.c > @@ -2347,11 +2347,14 @@ static int sdp_read_header(AVFormatContext *s) > /* read the whole sdp file */ > /* XXX: better loading */ > content = av_malloc(SDP_MAX_SIZE); > - if (!content) > + if (!content) { > + ff_network_close(); > return AVERROR(ENOMEM); > + } > size = avio_read(s->pb, content, SDP_MAX_SIZE - 1); > if (size <= 0) { > av_free(content); > + ff_network_close(); > return AVERROR_INVALIDDATA; > } > content[size] ='\0'; > @@ -2550,7 +2553,9 @@ static int rtp_read_header(AVFormatContext *s) > ffio_init_context(&pb, sdp.str, sdp.len, 0, NULL, NULL, NULL, NULL); > s->pb = &pb; > > - /* sdp_read_header initializes this again */ > + /* if sdp_read_header() fails then following ff_network_close() cancels out */ > + /* ff_network_init() at the start of this function. Otherwise it cancels out */ > + /* ff_network_init() inside sdp_read_header() */ > ff_network_close(); > > rt->media_type_mask = (1 << (AVMEDIA_TYPE_SUBTITLE+1)) - 1; > -- > 2.28.0 Ok Unrelatedly, as we seem to have a problem with ff_network_init()/ff_network_close() often being mismatched and not handled correctly, do you think it'd be good as a development aid, to make certain network functions (ff_url_join, the low level tcp protocol etc) error out on unix systems as well, if ff_network_init() has been missed? And same for counting the calls to init/close to make sure they're properly paired. Otherwise these issues only hit people on windows, where the code probably is tested much less. The problem, I guess, is finding the right level of obnoxity for the error. We probably don't want such an issue to break code that otherwise has run fine in production on unix platforms, even though it points out a portability problem. And as far as I know, most developers don't really build/test ffmpeg in debug mode anyway, so if the checks are hidden in such a mode, they won't help developers notice the issues either. And we don't have much fate coverage for the protocols. // Martin
Hi Martin, Thank you for reviewing the set. On Fri, 20. Nov 10:33, Martin Storsjö wrote: > On Mon, 12 Oct 2020, Andriy Gelman wrote: > > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > In sdp_read_header() some ff_network_close() calls were missed. > > > > Also in rtp_read_header() update comment to explain why a single > > call to ff_network_close() is enough to cover all cases even if > > sdp_read_header() returns an error. > > > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > > --- > > libavformat/rtsp.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > > index cb9fc31166..a8c7ec4a46 100644 > > --- a/libavformat/rtsp.c > > +++ b/libavformat/rtsp.c > > @@ -2347,11 +2347,14 @@ static int sdp_read_header(AVFormatContext *s) > > /* read the whole sdp file */ > > /* XXX: better loading */ > > content = av_malloc(SDP_MAX_SIZE); > > - if (!content) > > + if (!content) { > > + ff_network_close(); > > return AVERROR(ENOMEM); > > + } > > size = avio_read(s->pb, content, SDP_MAX_SIZE - 1); > > if (size <= 0) { > > av_free(content); > > + ff_network_close(); > > return AVERROR_INVALIDDATA; > > } > > content[size] ='\0'; > > @@ -2550,7 +2553,9 @@ static int rtp_read_header(AVFormatContext *s) > > ffio_init_context(&pb, sdp.str, sdp.len, 0, NULL, NULL, NULL, NULL); > > s->pb = &pb; > > > > - /* sdp_read_header initializes this again */ > > + /* if sdp_read_header() fails then following ff_network_close() cancels out */ > > + /* ff_network_init() at the start of this function. Otherwise it cancels out */ > > + /* ff_network_init() inside sdp_read_header() */ > > ff_network_close(); > > > > rt->media_type_mask = (1 << (AVMEDIA_TYPE_SUBTITLE+1)) - 1; > > -- > > 2.28.0 > > Ok > > Unrelatedly, as we seem to have a problem with > ff_network_init()/ff_network_close() often being mismatched and not handled > correctly, do you think it'd be good as a development aid, to make certain > network functions (ff_url_join, the low level tcp protocol etc) error out on > unix systems as well, if ff_network_init() has been missed? And same for > counting the calls to init/close to make sure they're properly paired. > Otherwise these issues only hit people on windows, where the code probably > is tested much less. > > The problem, I guess, is finding the right level of obnoxity for the error. > We probably don't want such an issue to break code that otherwise has run > fine in production on unix platforms, even though it points out a > portability problem. And as far as I know, most developers don't really > build/test ffmpeg in debug mode anyway, so if the checks are hidden in such > a mode, they won't help developers notice the issues either. And we don't > have much fate coverage for the protocols. I think it would be a useful tool. But finding the right balance seems tricky.. I'll try to think more about the problem and propose something that's not too intrusive.
diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c index cb9fc31166..a8c7ec4a46 100644 --- a/libavformat/rtsp.c +++ b/libavformat/rtsp.c @@ -2347,11 +2347,14 @@ static int sdp_read_header(AVFormatContext *s) /* read the whole sdp file */ /* XXX: better loading */ content = av_malloc(SDP_MAX_SIZE); - if (!content) + if (!content) { + ff_network_close(); return AVERROR(ENOMEM); + } size = avio_read(s->pb, content, SDP_MAX_SIZE - 1); if (size <= 0) { av_free(content); + ff_network_close(); return AVERROR_INVALIDDATA; } content[size] ='\0'; @@ -2550,7 +2553,9 @@ static int rtp_read_header(AVFormatContext *s) ffio_init_context(&pb, sdp.str, sdp.len, 0, NULL, NULL, NULL, NULL); s->pb = &pb; - /* sdp_read_header initializes this again */ + /* if sdp_read_header() fails then following ff_network_close() cancels out */ + /* ff_network_init() at the start of this function. Otherwise it cancels out */ + /* ff_network_init() inside sdp_read_header() */ ff_network_close(); rt->media_type_mask = (1 << (AVMEDIA_TYPE_SUBTITLE+1)) - 1;