[svn-commits] rmudgett: branch 1.6.2 r318636 - /branches/1.6.2/channels/chan_sip.c

[SVN commits to the Digium repositories]

Author: rmudgett
Date: Thu May 12 16:04:30 2011
New Revision: 318636

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=318636
Log:
Merged revision 222981 from
https://origsvn.digium.com/svn/asterisk/branches/1.8

Similar deadlock possible when running the Pickup application internally.

------------------------------------------------------------------------
r222981 | dvossel | 2009-10-08 17:04:41 -0500 (Thu, 08 Oct 2009) | 13 lines

Deadlock between ast_cel_report_event and ast_do_masquerade

chan_sip calls pbx_exec on a pvt's owner channel while only the
pvt lock is held.  Since pbx_exec calls ast_cel_report_event which
attempts to lock the channel, invalid locking order occurs.  Channels
should be locked before pvt's.

(closes issue #15512)
Reported by: lmsteffan
Patches:
      ast_cel_deadlock_15512.diff uploaded by dvossel (license 671)

Modified:
    branches/1.6.2/channels/chan_sip.c

Modified: branches/1.6.2/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.6.2/channels/chan_sip.c?view=diff&rev=318636&r1=318635&r2=318636
==============================================================================
--- branches/1.6.2/channels/chan_sip.c (original)
+++ branches/1.6.2/channels/chan_sip.c Thu May 12 16:04:30 2011
@@ -20239,6 +20239,7 @@
 	return sip_uri_params_cmp(params1, params2);
 }
 
+/* \note No channel or pvt locks should be held while calling this function. */
 static int do_magic_pickup(struct ast_channel *channel, const char *extension, const char *context)
 {
 	struct ast_str *str = ast_str_alloca(AST_MAX_EXTENSION + AST_MAX_CONTEXT + 2);
@@ -20906,12 +20907,17 @@
 			/* Do the pickup itself */
 			ast_channel_unlock(c);
 			*nounlock = 1;
+
+			/* since p->owner (c) is unlocked, we need to go ahead and unlock pvt for both
+			 * magic pickup and ast_hangup.  Both of these functions will attempt to lock
+			 * p->owner again, which can cause a deadlock if we already hold a lock on p.
+			 * Locking order is, channel then pvt.  Dead lock avoidance must be used if
+			 * called the other way around. */
+			sip_pvt_unlock(p);
 			do_magic_pickup(c, pickup.exten, pickup.context);
-
 			/* Now we're either masqueraded or we failed to pickup, in either case we... */
-			sip_pvt_unlock(p);
 			ast_hangup(c);
-			sip_pvt_lock(p);
+			sip_pvt_lock(p); /* pvt is expected to remain locked on return, so re-lock it */
 
 			res = 0;
 			goto request_invite_cleanup;