[FFmpeg-devel] [PATCH v3 6/6] avformat/rtsp: don't forget to call ff_network_close() on error

[Martin Storsjö]

On Mon, 12 Oct 2020, Andriy Gelman wrote:

> From: Andriy Gelman <andriy.gelman at 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 at 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