[FFmpeg-devel] [PATCH] rtsp: Fix infinite loop in listen mode with UDP transport

[Martin Storsjö]

On Wed, 30 Sep 2020, Zhao Zhili wrote:

> Hi Martin,
>
> On 9/30/20 5:41 PM, Martin Storsjö wrote:
>> In listen mode with UDP transport, once the sender has sent
>> the TEARDOWN and closed the connection, poll will indicate that
>> one can read from the connection (indicating that the socket has
>> reached EOF and should be closed by the receiver as well). In this
>> case, parse_rtsp_message won't try to parse the command (because
>> it's no longer in state STREAMING), but previously just returned
>> zero.
>>
>> Prior to f6161fccf8c5720ceac1ed1df8ba60ff8fed69f5, this caused
>> udp_read_packet to return zero, which is treated as EOF by
>> read_packet. But after that commit, udp_read_packet would continue
>> if parse_rtsp_message didn't return an explicit error code.
>>
>> To keep the original behaviour from before that commit, more
>> explicitly return an error in parse_rtsp_message when in the wrong
>> state.
>>
>> Fixes: #8840
>> ---
>>   libavformat/rtsp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> index 5d8491b74b..ad12f2ae98 100644
>> --- a/libavformat/rtsp.c
>> +++ b/libavformat/rtsp.c
>> @@ -1970,7 +1970,7 @@ static int parse_rtsp_message(AVFormatContext *s)
>>                   av_log(s, AV_LOG_WARNING,
>>                          "Unable to answer to TEARDOWN\n");
>>           } else
>> -            return 0;
>> +            return AVERROR_EOF;
>
> Does the else part needs the same fix?

Which else part do you refer to? The else above (with the warning)? Yes 
that bit looks a bit odd to me as well - your patch 2/2 looks like a good 
fix for that.

> I tried a similar approach in
>
> http://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267437.html.

I'm less keen on that approach. As the problem is with listen mode, I'd 
cautiously avoid changing code that is general to both modes.

> The intended behavior (e.g., return value) of parse_rtsp_message is not 
> quite clear
>
> to me, could you help improve it, like adding some documentation?

Well I'm not sure how much there is to document. It's a static function 
that is called from one single place in the code, so the documentation of 
it is the code surrounding the single call to it. Ideally it'd follow 
common conventions like returning a negative value on error and 
zero/positive on success that one should continue from.

// Martin