From 605b497108999fc322cce17a1d1f0c1b0a0a5f33 Mon Sep 17 00:00:00 2001 From: Richard Fuchs Date: Fri, 7 Mar 2025 15:08:21 -0400 Subject: [PATCH] MT#55283 unconditionally demux RTCP This seems to be an acceptable and reliable way to detect RTCP multiplexed with RTP, even if `a=rtcp-mux` wasn't advertised in the SDP. Take the opportunity to clean up __streams_set_sinks() a bit by giving the variables better names. Change-Id: I0cdc5e4a544641591fc2aabca12fb11bab3453f7 --- daemon/call.c | 64 +++++++++++++++++------------------- daemon/media_socket.c | 34 ++++++------------- kernel-module/xt_RTPENGINE.c | 35 ++++++++------------ kernel-module/xt_RTPENGINE.h | 2 +- lib/rtcplib.h | 10 +++--- t/auto-daemon-tests.pl | 61 ++++++++++++++++++++++++++++++++++ 6 files changed, 121 insertions(+), 85 deletions(-) diff --git a/daemon/call.c b/daemon/call.c index 79cf1c607..14c6b53eb 100644 --- a/daemon/call.c +++ b/daemon/call.c @@ -1356,8 +1356,6 @@ static bool __init_streams(struct call_media *A, const struct stream_params *sp, __attribute__((nonnull(1, 2, 4))) static bool __streams_set_sinks(struct call_media *A, struct call_media *B, const sdp_ng_flags *flags, const struct sink_attrs *attrs) { - struct packet_stream *a, *ax, *b; - __auto_type la = A->streams.head; __auto_type lb = B->streams.head; @@ -1367,59 +1365,59 @@ static bool __streams_set_sinks(struct call_media *A, struct call_media *B, if (!lb) break; // nothing left to do - a = la->data; - b = lb->data; + __auto_type a_rtp = la->data; + __auto_type b_rtp = lb->data; /* RTP */ // reflect media - pretend reflection also for blackhole, as otherwise // we get SSRC flip-flops on the opposite side // XXX still necessary for blackhole? - if (attrs->egress && b) - __add_sink_handler(&a->rtp_mirrors, b, attrs); + if (attrs->egress && b_rtp) + __add_sink_handler(&a_rtp->rtp_mirrors, b_rtp, attrs); else if (MEDIA_ISSET(A, ECHO) || MEDIA_ISSET(A, BLACKHOLE)) - __add_sink_handler(&a->rtp_sinks, a, attrs); - else if (b && MEDIA_ISSET(B, SEND)) - __add_sink_handler(&a->rtp_sinks, b, attrs); + __add_sink_handler(&a_rtp->rtp_sinks, a_rtp, attrs); + else if (b_rtp && MEDIA_ISSET(B, SEND)) + __add_sink_handler(&a_rtp->rtp_sinks, b_rtp, attrs); - PS_CLEAR(b, ZERO_ADDR); - if (is_addr_unspecified(&a->advertised_endpoint.address) - && !(is_trickle_ice_address(&a->advertised_endpoint) + PS_CLEAR(b_rtp, ZERO_ADDR); + if (is_addr_unspecified(&a_rtp->advertised_endpoint.address) + && !(is_trickle_ice_address(&a_rtp->advertised_endpoint) && MEDIA_ISSET(A, TRICKLE_ICE)) && !(flags && flags->replace_zero_address)) - PS_SET(b, ZERO_ADDR); + PS_SET(b_rtp, ZERO_ADDR); /* RTCP */ - if (!MEDIA_ISSET(B, RTCP_MUX)) { - lb = lb->next; - assert(lb != NULL); - b = lb->data; - } - - if (MEDIA_ISSET(A, RTCP_MUX)) { - if (MEDIA_ISSET(A, ECHO) || MEDIA_ISSET(A, BLACKHOLE)) - { /* RTCP sink handler added below */ } - else if (b) - __add_sink_handler(&a->rtcp_sinks, b, attrs); - } - - ax = a; + lb = lb->next; + assert(lb != NULL); + __auto_type b_rtcp = lb->data; /* if muxing, this is the fallback RTCP port. it also contains the RTCP * crypto context */ la = la->next; assert(la != NULL); - a = la->data; + __auto_type a_rtcp = la->data; if (attrs->egress) goto no_rtcp; if (MEDIA_ISSET(A, ECHO) || MEDIA_ISSET(A, BLACKHOLE)) { - __add_sink_handler(&a->rtcp_sinks, a, attrs); - if (MEDIA_ISSET(A, RTCP_MUX)) - __add_sink_handler(&ax->rtcp_sinks, a, attrs); + if (MEDIA_ISSET(A, RTCP_MUX)) { + __add_sink_handler(&a_rtp->rtcp_sinks, a_rtp, attrs); + __add_sink_handler(&a_rtcp->rtcp_sinks, a_rtp, attrs); + } + else { + __add_sink_handler(&a_rtp->rtcp_sinks, a_rtcp, attrs); + __add_sink_handler(&a_rtcp->rtcp_sinks, a_rtcp, attrs); + } + } + else if (MEDIA_ISSET(B, RTCP_MUX)) { + __add_sink_handler(&a_rtp->rtcp_sinks, b_rtp, attrs); + __add_sink_handler(&a_rtcp->rtcp_sinks, b_rtp, attrs); + } + else if (b_rtcp) { + __add_sink_handler(&a_rtp->rtcp_sinks, b_rtcp, attrs); + __add_sink_handler(&a_rtcp->rtcp_sinks, b_rtcp, attrs); } - else if (b) - __add_sink_handler(&a->rtcp_sinks, b, attrs); no_rtcp: la = la->next; diff --git a/daemon/media_socket.c b/daemon/media_socket.c index 1863b004a..1d378d5fe 100644 --- a/daemon/media_socket.c +++ b/daemon/media_socket.c @@ -1438,13 +1438,6 @@ static void stream_fd_closed(int fd, void *p) { -/* returns: 0 = not a muxed stream, 1 = muxed, RTP, 2 = muxed, RTCP */ -static int rtcp_demux(const str *s, struct call_media *media) { - if (!MEDIA_ISSET(media, RTCP_MUX)) - return 0; - return rtcp_demux_is_rtcp(s) ? 2 : 1; -} - static int call_avp2savp_rtp(str *s, struct packet_stream *stream, struct ssrc_ctx *ssrc_ctx) { return rtp_avp2savp(s, &stream->crypto, ssrc_ctx); @@ -1584,7 +1577,6 @@ static const char *kernelize_one(struct rtpengine_target_info *reti, GQueue *out __re_address_translate_ep(&reti->local, &stream->selected_sfd->socket.local); reti->iface_stats = stream->selected_sfd->local_intf->stats; reti->stats = stream->stats_in; - reti->rtcp_mux = MEDIA_ISSET(media, RTCP_MUX); reti->rtcp = PS_ISSET(stream, RTCP); reti->dtls = MEDIA_ISSET(media, DTLS); reti->stun = media->ice_agent ? 1 : 0; @@ -2201,23 +2193,17 @@ static void media_packet_rtcp_demux(struct packet_handler_ctx *phc) { phc->in_srtp = phc->mp.stream; phc->sinks = &phc->mp.stream->rtp_sinks; + // is this RTCP? - if (PS_ISSET(phc->mp.stream, RTCP)) { - int is_rtcp = 1; - // plain RTCP or are we muxing? - if (MEDIA_ISSET(phc->mp.media, RTCP_MUX)) { - is_rtcp = 0; - int muxed_rtcp = rtcp_demux(&phc->s, phc->mp.media); - if (muxed_rtcp == 2) { - is_rtcp = 1; - if (phc->mp.stream->rtcp_sibling) - phc->in_srtp = phc->mp.stream->rtcp_sibling; // use RTCP SRTP context - } - } - if (is_rtcp) { - phc->sinks = &phc->mp.stream->rtcp_sinks; - phc->rtcp = true; - } + if (!proto_is_rtp(phc->mp.media->protocol)) + return; // no + + bool is_rtcp = rtcp_demux_is_rtcp(&phc->s); + if (is_rtcp) { + if (phc->mp.stream->rtcp_sibling) + phc->in_srtp = phc->mp.stream->rtcp_sibling; // use RTCP SRTP context + phc->sinks = &phc->mp.stream->rtcp_sinks; + phc->rtcp = true; } } // out_srtp is set to point to the SRTP context to use diff --git a/kernel-module/xt_RTPENGINE.c b/kernel-module/xt_RTPENGINE.c index 0f793f0ff..e5b6a574c 100644 --- a/kernel-module/xt_RTPENGINE.c +++ b/kernel-module/xt_RTPENGINE.c @@ -1742,8 +1742,6 @@ static int proc_list_show(struct seq_file *f, void *v) { seq_printf(f, " RTP-only"); if (g->target.rtcp) seq_printf(f, " RTCP"); - if (g->target.rtcp_mux) - seq_printf(f, " RTCP-mux"); if (g->target.dtls) seq_printf(f, " DTLS"); if (g->target.stun) @@ -5826,17 +5824,17 @@ static inline int srtcp_decrypt(struct re_crypto_context *c, } -static inline int is_muxed_rtcp(struct sk_buff *skb) { +static inline bool is_muxed_rtcp(struct sk_buff *skb) { // XXX shared code unsigned char m_pt; if (skb->len < 8) // minimum RTCP size - return 0; + return false; m_pt = skb->data[1]; if (m_pt < 194) - return 0; + return false; if (m_pt > 223) - return 0; - return 1; + return false; + return true; } static inline int is_rtcp_fb_packet(struct sk_buff *skb) { unsigned char m_pt; @@ -6270,21 +6268,8 @@ static unsigned int rtpengine46(struct sk_buff *skb, struct sk_buff *oskb, rtp.rtcp = 0; is_rtcp = NOT_RTCP; if (g->target.rtp) { - if (g->target.rtcp) { - if (g->target.rtcp_mux) { - if (is_muxed_rtcp(skb)) - is_rtcp = RTCP; - } - else - is_rtcp = RTCP; - } - - if (is_rtcp == NOT_RTCP) { - parse_rtp(&rtp, skb); - if (!rtp.ok && g->target.rtp_only) - goto out; // pass to userspace - } - else { + if (is_muxed_rtcp(skb)) { + is_rtcp = RTCP; if (g->target.rtcp_fb_fw && is_rtcp_fb_packet(skb)) ; // forward and then drop else if (g->target.rtcp_fw) @@ -6296,6 +6281,12 @@ static unsigned int rtpengine46(struct sk_buff *skb, struct sk_buff *oskb, if (!rtp.rtcp) goto out; } + else { + // not RTCP + parse_rtp(&rtp, skb); + if (!rtp.ok && g->target.rtp_only) + goto out; // pass to userspace + } } if (rtp.ok) { // RTP ok diff --git a/kernel-module/xt_RTPENGINE.h b/kernel-module/xt_RTPENGINE.h index d16996111..bb6264e20 100644 --- a/kernel-module/xt_RTPENGINE.h +++ b/kernel-module/xt_RTPENGINE.h @@ -98,7 +98,7 @@ struct rtpengine_target_info { struct interface_stats_block *iface_stats; // for ingress stats struct stream_stats *stats; // for ingress stats - unsigned int rtcp_mux:1, + unsigned int __unused1:1, dtls:1, stun:1, rtp:1, diff --git a/lib/rtcplib.h b/lib/rtcplib.h index 2f40747a8..b68416d5f 100644 --- a/lib/rtcplib.h +++ b/lib/rtcplib.h @@ -29,19 +29,19 @@ struct rtcp_packet { /* RFC 5761 section 4 */ -INLINE int rtcp_demux_is_rtcp(const str *s) { +INLINE bool rtcp_demux_is_rtcp(const str *s) { struct rtcp_packet *rtcp; if (s->len < sizeof(*rtcp)) - return 0; + return false; rtcp = (void *) s->s; if (rtcp->header.pt < 194) - return 0; + return false; if (rtcp->header.pt > 223) - return 0; - return 1; + return false; + return true; } diff --git a/t/auto-daemon-tests.pl b/t/auto-daemon-tests.pl index fcbe8ac4a..289a120f6 100755 --- a/t/auto-daemon-tests.pl +++ b/t/auto-daemon-tests.pl @@ -16630,6 +16630,67 @@ srtp_rcv($sock_a, $port_b, rtpm(0, 2000, 4000, -1, "\x00" x 160), $srtp_ctx_b); # RTCP +($sock_a, $sock_ax, $sock_b, $sock_bx) = new_call( + [qw(198.51.100.1 7430)], + [qw(198.51.100.1 7431)], + [qw(198.51.100.3 7432)], + [qw(198.51.100.3 7433)], +); + +($port_a, $port_ax) = offer('demux unannounced RTCP', { }, <