[Asterisk-code-review] chan pjsip: Lock channel when checking for RTP changes. (asterisk[13])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged.

Change subject: chan_pjsip: Lock channel when checking for RTP changes.
......................................................................


chan_pjsip: Lock channel when checking for RTP changes.

bridge_native_rtp can call into an RTP-capable channel driver in order
for the driver to update information about who the channel is
communicating with. For SIP channel drivers, this means deactivating
RTCP and sending a reinvite so that the endpoints can communicate
directly.

bridge_native_rtp does the right thing and has the channel locked when
calling into the channel driver. chan_pjsip can't alter session
properties in this thread, though. chan_pjsip queues a task on the
session serializer in order to update properties there.

The problem is that this queued task was not locking the channel. This
meant that the queued task could attempt to deactivate RTCP at the same
time that the channel thread was attempting to process an incoming RTCP
packet. This could lead to a crash.

This patch fixes the issue by locking the channel in the queued task
when altering RTP properties.

ASTERISK-26092 #close
Reported by Niklas Larsson

Change-Id: I3464e226a3c41f6b915f97891e07fa1599e2a159
---
M channels/chan_pjsip.c
1 file changed, 9 insertions(+), 0 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index f081bd8..970fef4 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -269,6 +269,9 @@
 	return 0;
 }
 
+/*!
+ * \pre chan is locked
+ */
 static int check_for_rtp_changes(struct ast_channel *chan, struct ast_rtp_instance *rtp,
 		struct ast_sip_session_media *media, int rtcp_fd)
 {
@@ -338,6 +341,11 @@
 	int changed = 0;
 	int res = 0;
 
+	/* The channel needs to be locked when checking for RTP changes.
+	 * Otherwise, we could end up destroying an underlying RTCP structure
+	 * at the same time that the channel thread is attempting to read RTCP
+	 */
+	ast_channel_lock(cdata->chan);
 	if (pvt->media[SIP_MEDIA_AUDIO]) {
 		changed |= check_for_rtp_changes(
 			cdata->chan, cdata->rtp, pvt->media[SIP_MEDIA_AUDIO], 1);
@@ -346,6 +354,7 @@
 		changed |= check_for_rtp_changes(
 			cdata->chan, cdata->vrtp, pvt->media[SIP_MEDIA_VIDEO], 3);
 	}
+	ast_channel_unlock(cdata->chan);
 
 	if (direct_media_mitigate_glare(cdata->session)) {
 		ast_debug(4, "Disregarding setting RTP on %s: mitigating re-INVITE glare\n", ast_channel_name(cdata->chan));

-- 
To view, visit https://gerrit.asterisk.org/3003
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3464e226a3c41f6b915f97891e07fa1599e2a159
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>