[svn-commits] rmudgett: trunk r326145 - in /trunk: ./ channels/chan_sip.c

[SVN commits to the Digium repositories]

Author: rmudgett
Date: Fri Jul  1 16:11:34 2011
New Revision: 326145

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=326145
Log:
Merged revisions 326144 via svnmerge from 
https://origsvn.digium.com/svn/asterisk/branches/1.8

........
  r326144 | rmudgett | 2011-07-01 16:07:22 -0500 (Fri, 01 Jul 2011) | 16 lines
  
  Better way to get chan and pvt lock for issue ASTERISK-17431.
  
  Redoes -r308945 for issue ASTERISK-17431 deadlock fix for
  sip_set_udptl_peer() and sip_set_rtp_peer().
  
  * Lock the channels in the defined order and avoid the need for a deadlock
  avoidance loop.
  
  * Lock the channel before getting the pointer to the private structure to
  be sure that the pointer will not change due to a masquerade or channel
  hangup.
  
  * To preserve sanity, check that chan and p->owner are the same.  (Pointer
  rearangements should not happen without the protection of locks because
  bad things tend to happen otherwise.)
........

Modified:
    trunk/   (props changed)
    trunk/channels/chan_sip.c

Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-1.8-merged' - no diff available.

Modified: trunk/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/trunk/channels/chan_sip.c?view=diff&rev=326145&r1=326144&r2=326145
==============================================================================
--- trunk/channels/chan_sip.c (original)
+++ trunk/channels/chan_sip.c Fri Jul  1 16:11:34 2011
@@ -28775,24 +28775,22 @@
 {
 	struct sip_pvt *p;
 
+	/* Lock the channel and the private safely. */
+	ast_channel_lock(chan);
 	p = chan->tech_pvt;
 	if (!p) {
+		ast_channel_unlock(chan);
 		return -1;
 	}
-	/*
-	 * Lock both the pvt and it's owner safely.
-	 */
 	sip_pvt_lock(p);
-	while (p->owner && ast_channel_trylock(p->owner)) {
+	if (p->owner != chan) {
+		/* I suppose it could be argued that if this happens it is a bug. */
+		ast_debug(1, "The private is not owned by channel %s anymore.\n", chan->name);
 		sip_pvt_unlock(p);
-		usleep(1);
-		sip_pvt_lock(p);
-	}
-
-	if (!p->owner) {
-		sip_pvt_unlock(p);
+		ast_channel_unlock(chan);
 		return 0;
 	}
+
 	if (udptl) {
 		ast_udptl_get_peer(udptl, &p->udptlredirip);
 	} else {
@@ -28811,8 +28809,8 @@
 	}
 	/* Reset lastrtprx timer */
 	p->lastrtprx = p->lastrtptx = time(NULL);
-	ast_channel_unlock(p->owner);
 	sip_pvt_unlock(p);
+	ast_channel_unlock(chan);
 	return 0;
 }
 
@@ -28919,9 +28917,20 @@
 	struct sip_pvt *p;
 	int changed = 0;
 
+	/* Lock the channel and the private safely. */
+	ast_channel_lock(chan);
 	p = chan->tech_pvt;
 	if (!p) {
+		ast_channel_unlock(chan);
 		return -1;
+	}
+	sip_pvt_lock(p);
+	if (p->owner != chan) {
+		/* I suppose it could be argued that if this happens it is a bug. */
+		ast_debug(1, "The private is not owned by channel %s anymore.\n", chan->name);
+		sip_pvt_unlock(p);
+		ast_channel_unlock(chan);
+		return 0;
 	}
 
 	/* Disable early RTP bridge  */
@@ -28931,25 +28940,10 @@
 			return 0;
 	}
 
-	/*
-	 * Lock both the pvt and it's owner safely.
-	 */
-	sip_pvt_lock(p);
-	while (p->owner && ast_channel_trylock(p->owner)) {
-		sip_pvt_unlock(p);
-		usleep(1);
-		sip_pvt_lock(p);
-	}
-
-	if (!p->owner) {
-		sip_pvt_unlock(p);
-		return 0;
-	}
-
 	if (p->alreadygone) {
 		/* If we're destroyed, don't bother */
-		ast_channel_unlock(p->owner);
 		sip_pvt_unlock(p);
+		ast_channel_unlock(chan);
 		return 0;
 	}
 
@@ -28957,8 +28951,8 @@
 	   that are known to be behind a NAT, then stop the process now
 	*/
 	if (nat_active && !ast_test_flag(&p->flags[0], SIP_DIRECT_MEDIA_NAT)) {
-		ast_channel_unlock(p->owner);
 		sip_pvt_unlock(p);
+		ast_channel_unlock(chan);
 		return 0;
 	}
 
@@ -29000,8 +28994,8 @@
 	}
 	/* Reset lastrtprx timer */
 	p->lastrtprx = p->lastrtptx = time(NULL);
-	ast_channel_unlock(p->owner);
 	sip_pvt_unlock(p);
+	ast_channel_unlock(chan);
 	return 0;
 }