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

[Zhao Zhili]

On 10/2/20 4:48 AM, Martin Storsjö wrote:
> On Thu, 1 Oct 2020, Andriy Gelman wrote:
>
>> On Thu, 01. Oct 22:00, Zhao Zhili wrote:
>>>
>>> On 10/1/20 4:15 AM, Martin Storsjö wrote:
>>> > 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 mean the else below, especially
>>>
>>>         /* XXX: parse message */
>>>         if (rt->state != RTSP_STATE_STREAMING)
>>>             return 0;
>>
>> I did some tests with the rtsp server from:
>> https://github.com/revmischa/rtsp-server
>>
>> This point can be reached with rt->state = RTSP_STATE_IDLE when the
>> initial_pause option is set:
>> ./ffmpeg -initial_pause 1 -i rtsp://127.0.0.1/abc -f null -
>>
>> Then it seems changing the return value in the above code would lead to
>> unintended results.
>
> Thanks for testing this.
>
> Indeed, I'd rather treat that as a separate case. The listen mode is 
> not very widely used, and this issue can be traced back to a 
> regression, so that can be easily fixed in that context.
>
> For the other case you're pointing out, I don't have a concrete bug 
> (the UDP mode seems to require waiting for a long timeout at the end 
> though, but changing this return statement to return an error doesn't 
> seem to help), and the normal non-listen mode code can be used in a 
> huge variety of cases, many that aren't very easy to test (e.g. the 
> code used to support RealRTSP, but I'm not sure if there's any 
> publicly available test clips/servers for that any longer - 
> multimediawiki used to list some, but I tested them last time close to 
> a decade ago, and then there might have been zero or one of them still 
> responding). So for that code, I'd tread much more carefully...

Understood. Use cases are complex than what the simple code shows.

>
> // Martin
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".