GNOME Bugzilla – Bug 796988
rtsp-client: We allow reused channel numbers for interleaved
Last modified: 2018-08-29 11:49:39 UTC
If a (strange) client would reuse interleaved channel numbers in multiple SETUP requests, gst-rtsp-server accepts them and returns them in the 200 OK response. This leads to problems in rtsp-client.c, where the channel numbers are used for looking up stream transports (and indirectly streams) in the priv->transports hash table. Transports disappear from the table if channel numbers are reused. After a second stream has been established, the first stream gets stuck because it gets no acks as per the code added in the commit "Limit queued TCP data messages to one per stream". Also before that commit, there was a problem with received RTCP packets getting lost for the first stream. It can be seen that reused channel numbers are accepted by running ./examples/test-video and doing telnet 127.0.0.1 8554 Enter the first SETUP, followed by a blank line: SETUP rtsp://127.0.0.1:8554/test/stream=0 RTSP/1.0 CSeq: 1 Transport: RTP/AVP/TCP;unicast;interleaved=0-1 Note the session number in the response and use it in a second SETUP (blank line after): SETUP rtsp://127.0.0.1:8554/test/stream=1 RTSP/1.0 CSeq: 2 Transport: RTP/AVP/TCP;unicast;interleaved=0-1 Session: 9MZEvi9FJI3jycJL It can be seen that interleaved=0-1 is returned in the second response.
Created attachment 373395 [details] [review] rtsp-client: Add unit test of SETUP for RTSP/RTP/TCP First extend rtsp-client unit tests to test SETUP for RTSP/RTP/TCP.
Created attachment 373396 [details] [review] rtsp-client: Avoid reuse of channel numbers for interleaved Change channel numbers if they are already in use, with unit test.
Two suggested patches (building on each other) have been posted above.
Review of attachment 373396 [details] [review]: ::: gst/rtsp-server/rtsp-client.c @@ +2091,3 @@ + GINT_TO_POINTER (ct->interleaved.max))) { + gst_rtsp_session_media_alloc_channels (ctx->sessmedia, + &ct->interleaved); This would loop forever if all 256 ids are taken already. In all other cases, can this actually loop multiple times? Shouldn't the allocation of the channels always give free, valid values?
(In reply to Sebastian Dröge (slomo) from comment #4) > Review of attachment 373396 [details] [review] [review]: > > ::: gst/rtsp-server/rtsp-client.c > @@ +2091,3 @@ > + GINT_TO_POINTER (ct->interleaved.max))) { > + gst_rtsp_session_media_alloc_channels (ctx->sessmedia, > + &ct->interleaved); > > This would loop forever if all 256 ids are taken already. In all other > cases, can this actually loop multiple times? Shouldn't the allocation of > the channels always give free, valid values? It does loop twice when the client passes 0-1 for both streams. The first call to gst_rtsp_session_media_alloc_channels() gives (0,1), because in there is a counter that starts from 0, independently of the priv->transports table. Yes, there should be error handling in the case of 256 taken ids. Probably for the case where ct->interleaved.max goes past 255.
(In reply to David Svensson Fors from comment #5) > (In reply to Sebastian Dröge (slomo) from comment #4) > > Review of attachment 373396 [details] [review] [review] [review]: > > > > ::: gst/rtsp-server/rtsp-client.c > > @@ +2091,3 @@ > > + GINT_TO_POINTER (ct->interleaved.max))) { > > + gst_rtsp_session_media_alloc_channels (ctx->sessmedia, > > + &ct->interleaved); > > > > This would loop forever if all 256 ids are taken already. In all other > > cases, can this actually loop multiple times? Shouldn't the allocation of > > the channels always give free, valid values? > > It does loop twice when the client passes 0-1 for both streams. The first > call to gst_rtsp_session_media_alloc_channels() gives (0,1), because in > there is a counter that starts from 0, independently of the > priv->transports table. So if 0-1, 2-3 are used already, this would loop three times to count up to 4-5?
(In reply to Sebastian Dröge (slomo) from comment #6) > (In reply to David Svensson Fors from comment #5) > > (In reply to Sebastian Dröge (slomo) from comment #4) > > > Review of attachment 373396 [details] [review] [review] [review] [review]: > > > > > > ::: gst/rtsp-server/rtsp-client.c > > > @@ +2091,3 @@ > > > + GINT_TO_POINTER (ct->interleaved.max))) { > > > + gst_rtsp_session_media_alloc_channels (ctx->sessmedia, > > > + &ct->interleaved); > > > > > > This would loop forever if all 256 ids are taken already. In all other > > > cases, can this actually loop multiple times? Shouldn't the allocation of > > > the channels always give free, valid values? > > > > It does loop twice when the client passes 0-1 for both streams. The first > > call to gst_rtsp_session_media_alloc_channels() gives (0,1), because in > > there is a counter that starts from 0, independently of the > > priv->transports table. > > So if 0-1, 2-3 are used already, this would loop three times to count up to > 4-5? Yes. And the client could allocate channels in any pattern first, before it reuses a channel, so if we want to use the lowest avalable channel we probably need to loop.
Ok so this should loop but once 255 is reached it should error out I guess?
Created attachment 373495 [details] [review] rtsp-client: Avoid reuse of channel numbers for interleaved
(In reply to Sebastian Dröge (slomo) from comment #8) > Ok so this should loop but once 255 is reached it should error out I guess? Yes. Uploaded a new patch.
Attachment 373395 [details] pushed as 990d5dd - rtsp-client: Add unit test of SETUP for RTSP/RTP/TCP Attachment 373495 [details] pushed as a2e182c - rtsp-client: Avoid reuse of channel numbers for interleaved