Re: [FFmpeg-devel] [PATCH] rtsp: Fix infinite loop in listen mode with UDP transport
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@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] rtsp: Fix infinite loop in listen mode with UDP transport
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... // Martin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] rtsp: Fix infinite loop in listen mode with UDP transport
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. -- Andriy ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] rtsp: Fix infinite loop in listen mode with UDP transport
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; Otherwise, I'm OK with the patch. 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] rtsp: Fix infinite loop in listen mode with UDP transport
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] rtsp: Fix infinite loop in listen mode with UDP transport
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?
I tried a similar approach in
http://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267437.html.
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?
} else {
RTSPMessageHeader reply;
ret = ff_rtsp_read_reply(s, , NULL, 0, NULL);
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] rtsp: Fix infinite loop in listen mode with UDP transport
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;
} else {
RTSPMessageHeader reply;
ret = ff_rtsp_read_reply(s, , NULL, 0, NULL);
--
2.24.3 (Apple Git-128)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
