[Asterisk-code-review] res rtp asterisk.c: Fix bundled SSRC handling. (asterisk[15.0])
Joshua Colp
asteriskteam at digium.com
Fri Sep 22 06:38:31 CDT 2017
Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/6562 )
Change subject: res_rtp_asterisk.c: Fix bundled SSRC handling.
......................................................................
res_rtp_asterisk.c: Fix bundled SSRC handling.
Assertions in the v15+ AST-2017-008 patches found that we were not
handling the case if the incoming SDP did not specify the required SSRC
attributes for bundled to work.
* Be strict on matching SSRC for bundled instances including the parent
instance. If the SSRC doesn't match then discard the packet. Bundled has
to tell us in the SDP signaling what SSRC to expect. Otherwise, we will
not know how to find the bundled instance structure.
Change-Id: I152830bbff71c662408909042068fada39e617f9
---
M res/res_rtp_asterisk.c
1 file changed, 58 insertions(+), 42 deletions(-)
Approvals:
Joshua Colp: Looks good to me, but someone else must approve; Approved for Submit
George Joseph: Looks good to me, approved
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 923ec84..8f66a0a 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -250,6 +250,8 @@
struct rtp_ssrc_mapping {
/*! \brief The received SSRC */
unsigned int ssrc;
+ /*! True if the SSRC is available. Otherwise, this is a placeholder mapping until the SSRC is set. */
+ unsigned int ssrc_valid;
/*! \brief The RTP instance this SSRC belongs to*/
struct ast_rtp_instance *instance;
};
@@ -3336,7 +3338,7 @@
* \return 0 if element does not match.
* \return Non-zero if element matches.
*/
-#define SSRC_MAPPING_ELEM_CMP(elem, value) (elem.instance == value)
+#define SSRC_MAPPING_ELEM_CMP(elem, value) ((elem).instance == (value))
/*! \pre instance is locked */
static int ast_rtp_destroy(struct ast_rtp_instance *instance)
@@ -4780,18 +4782,26 @@
struct ast_rtp *rtp, unsigned int ssrc)
{
int index;
- struct ast_rtp_instance *found = instance;
+ if (!AST_VECTOR_SIZE(&rtp->ssrc_mapping)) {
+ /* This instance is not bundled */
+ return instance;
+ }
+
+ /* Find the bundled child instance */
for (index = 0; index < AST_VECTOR_SIZE(&rtp->ssrc_mapping); ++index) {
struct rtp_ssrc_mapping *mapping = AST_VECTOR_GET_ADDR(&rtp->ssrc_mapping, index);
- if (mapping->ssrc == ssrc) {
- found = mapping->instance;
- break;
+ if (mapping->ssrc_valid && mapping->ssrc == ssrc) {
+ return mapping->instance;
}
}
- return found;
+ /* Does the SSRC match the bundled parent? */
+ if (rtp->themssrc_valid && rtp->themssrc == ssrc) {
+ return instance;
+ }
+ return NULL;
}
static const char *rtcp_payload_type2str(unsigned int pt)
@@ -5032,7 +5042,7 @@
/* Determine the appropriate instance for this */
if (ssrc_valid) {
child = rtp_find_instance_by_ssrc(transport, transport_rtp, ssrc);
- if (child != transport) {
+ if (child && child != transport) {
/*
* It is safe to hold the child lock while holding the parent lock.
* We guarantee that the locking order is always parent->child or
@@ -5529,6 +5539,10 @@
/* Determine the appropriate instance for this */
child = rtp_find_instance_by_ssrc(instance, rtp, ssrc);
+ if (!child) {
+ /* Neither the bundled parent nor any child has this SSRC */
+ return &ast_null_frame;
+ }
if (child != instance) {
/* It is safe to hold the child lock while holding the parent lock, we guarantee that the locking order
* is always parent->child or that the child lock is not held when acquiring the parent lock.
@@ -5711,39 +5725,42 @@
timestamp = ntohl(rtpheader[1]);
AST_LIST_HEAD_INIT_NOLOCK(&frames);
- /* Force a marker bit and change SSRC if the SSRC changes */
- if (rtp->themssrc_valid && rtp->themssrc != ssrc) {
- struct ast_frame *f, srcupdate = {
- AST_FRAME_CONTROL,
- .subclass.integer = AST_CONTROL_SRCCHANGE,
- };
- if (!mark) {
- if (rtpdebug) {
- ast_debug(1, "Forcing Marker bit, because SSRC has changed\n");
+ /* Only non-bundled instances can change/learn the remote's SSRC implicitly. */
+ if (!child && !AST_VECTOR_SIZE(&rtp->ssrc_mapping)) {
+ /* Force a marker bit and change SSRC if the SSRC changes */
+ if (rtp->themssrc_valid && rtp->themssrc != ssrc) {
+ struct ast_frame *f, srcupdate = {
+ AST_FRAME_CONTROL,
+ .subclass.integer = AST_CONTROL_SRCCHANGE,
+ };
+
+ if (!mark) {
+ if (rtpdebug) {
+ ast_debug(1, "Forcing Marker bit, because SSRC has changed\n");
+ }
+ mark = 1;
}
- mark = 1;
+
+ f = ast_frisolate(&srcupdate);
+ AST_LIST_INSERT_TAIL(&frames, f, frame_list);
+
+ rtp->seedrxseqno = 0;
+ rtp->rxcount = 0;
+ rtp->rxoctetcount = 0;
+ rtp->cycles = 0;
+ rtp->lastrxseqno = 0;
+ rtp->last_seqno = 0;
+ rtp->last_end_timestamp = 0;
+ if (rtp->rtcp) {
+ rtp->rtcp->expected_prior = 0;
+ rtp->rtcp->received_prior = 0;
+ }
}
- f = ast_frisolate(&srcupdate);
- AST_LIST_INSERT_TAIL(&frames, f, frame_list);
-
- rtp->seedrxseqno = 0;
- rtp->rxcount = 0;
- rtp->rxoctetcount = 0;
- rtp->cycles = 0;
- rtp->lastrxseqno = 0;
- rtp->last_seqno = 0;
- rtp->last_end_timestamp = 0;
- if (rtp->rtcp) {
- rtp->rtcp->expected_prior = 0;
- rtp->rtcp->received_prior = 0;
- }
+ rtp->themssrc = ssrc; /* Record their SSRC to put in future RR */
+ rtp->themssrc_valid = 1;
}
- /* Bundled children cannot change/learn their SSRC implicitly. */
- ast_assert(!child || (rtp->themssrc_valid && rtp->themssrc == ssrc));
- rtp->themssrc = ssrc; /* Record their SSRC to put in future RR */
- rtp->themssrc_valid = 1;
/* Remove any padding bytes that may be present */
if (padding) {
@@ -6481,12 +6498,13 @@
return;
}
+ rtp->themssrc = ssrc;
+ rtp->themssrc_valid = 1;
+
/* If this is bundled we need to update the SSRC mapping */
if (rtp->bundled) {
struct ast_rtp *bundled_rtp;
int index;
-
- ast_assert(rtp->themssrc_valid);
ao2_unlock(instance);
@@ -6497,8 +6515,9 @@
for (index = 0; index < AST_VECTOR_SIZE(&bundled_rtp->ssrc_mapping); ++index) {
struct rtp_ssrc_mapping *mapping = AST_VECTOR_GET_ADDR(&bundled_rtp->ssrc_mapping, index);
- if (mapping->ssrc == rtp->themssrc) {
+ if (mapping->instance == instance) {
mapping->ssrc = ssrc;
+ mapping->ssrc_valid = 1;
break;
}
}
@@ -6507,9 +6526,6 @@
ao2_lock(instance);
}
-
- rtp->themssrc = ssrc;
- rtp->themssrc_valid = 1;
}
static void ast_rtp_set_stream_num(struct ast_rtp_instance *instance, int stream_num)
@@ -6562,8 +6578,8 @@
/* Children maintain a reference to the parent to guarantee that the transport doesn't go away on them */
child_rtp->bundled = ao2_bump(parent);
- ast_assert(child_rtp->themssrc_valid);
mapping.ssrc = child_rtp->themssrc;
+ mapping.ssrc_valid = child_rtp->themssrc_valid;
mapping.instance = child;
ao2_unlock(child);
--
To view, visit https://gerrit.asterisk.org/6562
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15.0
Gerrit-MessageType: merged
Gerrit-Change-Id: I152830bbff71c662408909042068fada39e617f9
Gerrit-Change-Number: 6562
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170922/4888b73a/attachment.html>
More information about the asterisk-code-review
mailing list