[Bug 793441] rtsp-stream: client transport is not updated for multicast clients

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=793441

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #369331|none                        |needs-work
             status|                            |

--- Comment #48 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 369331:
 --> (https://bugzilla.gnome.org/review?bug=793441&attachment=369331)

::: gst/rtsp-server/rtsp-client.c
@@ +1873,1 @@
       goto error_allocating_ports;

gst_rtsp_stream_allocate_udp_sockets() here does not actually make use of the
use_client_settings boolean that is passed to it.

There seems to be some kind of problem here. Especially if there is already
socket of the right type, nothing at all is going to happen here. But for
use_client_settings and multicast we at least need to listen on the same port
(for RTCP, etc) as the given multicast destination. It is not enough to just
send data there, i.e. not enough to just put a new destination to the
multiudpsink, but instead we might need a new socket!

@@ +1880,3 @@
+        /* FIXME: We want actually make sure at this point, that the rtp and
+         * rtcp ports are reserved with the unicast address that our rtp/rtcp
+         * sockets are bound to. */

I don't understand the FIXME comment. We're in the multicast case currently,
why do we care about the RTP/RTCP ports of the unicast address (I assume of the
unicast socket)?

You mean that we should check here what I mentioned above? That the selected
port for the socket is actually the right one? I believe we need separate
sockets here for each client if they select different ports.

@@ -1883,3 @@
-         * the one requested by the client */
-        addr = gst_rtsp_stream_reserve_address (ctx->stream, ct->destination,
-            ct->port.min, ct->port.max - ct->port.min + 1, ct->ttl);

So this was checking that a client can only ever select at most once a specific
multicast address. This is weird, at least for shared medias it should be fine
for multiple clients to select the same values here.

For non-shared medias it's not but there this does not add much. Other medias
might have different address pools, or even in different processes, and might
also allow to select the same multicast destination. It can still conflict. And
clients would have to filter this based on the SSRC then in the end.

We probably also want a different bug report here for adding some API that
allows the server to allow/disallow specific multicast destinations. Doing that
via the address pool seems weird, in any case.


For the non-client-settings case, gst_rtsp_stream_get_multicast_address()
returns the next free multicast destination that exists (or the one that was
previously selected). That seems fine. I believe the pool should only be used
for selecting server-selected address/port pairs.

@@ +1905,3 @@
         ct->port.max = addr->port + addr->n_ports - 1;
         ct->ttl = addr->ttl;
+        gst_rtsp_address_free (addr);

For the unicast case, it seems like there's also the potential of the client
maliciously redirecting the traffic to some other place. Nothing new here, that
was already the case before your changes, but it should at least get a FIXME
comment and ideally a separate bug report

::: tests/check/gst/client.c
@@ -526,3 @@
   fail_unless (session_pool != NULL);

-  fail_unless (gst_rtsp_session_pool_get_n_sessions (session_pool) == 1);

Why is this invalid now?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.