[Home]

Summary:ASTERISK-15636: [patch] Deadlock in chan_local when obtaining locks on local_pvt->lock
Reporter:Dave Reeve (bzing2)Labels:
Date Opened:2010-02-16 11:40:27.000-0600Date Closed:2010-04-02 18:48:30
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_local
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bt-short.txt
( 1) issue_16840.rev1.diff
( 2) locks.txt
( 3) patch.txt
Description:Deadlock condition in chan_local when two brideged threads attempt to obtain
locks to eachothers local_pvt structures in the opposite order.  All
processing of SIP traffic halts when an attempt is made to hangup the call.

Thread A obtains a lock to its local_pvt->lock when in local_write.  When
in check_bridge a call is made to ast_bridged_channel, this in turn calls
local_bridgedchannel which attempts to obtain a lock on the bridged channels
local_pvt->lock.

Thread B follows a similar path.  It holds its own lock, and during a call to
ast_channel_masquerade again local_bridgedchannel is called.  This attempts to
obtain a lock to the bridged channel.

At this point either side has deadlocked.  Then a user attempts to hangup the
call and again local_bridgedchannel is called, and blocks while attempting to
get the lock.  Unfortunalty sip netlock is held and everything grinds to a
halt.
Comments:By: Dave Reeve (bzing2) 2010-02-16 11:45:40.000-0600

First attempt at a fix is to remove the check
       p->chan->_bridge != ast_bridged_channel(p->chan)
from the beginning of check_bridge.  If I understood correctly this can only be true
when LOCAL_ALREADY_MASQED is also true, which is tested first.  Unfortunatly I
know we cant trust the flag as we were already in the process of setting up
the masqurade when it deadlocked, what effect that has on local_queue_frame I
do not (as yet) know.

I belive this will work mainly because one call to check_bridge has isoutbound
true and the other is false.  In the second case case no action is preformed
whatever the result of ast_bridged_channel.
By: David Brillert (aragon) 2010-02-16 11:57:35.000-0600

Is this a duplicate of issue ASTERISK-15495 ?
If yes then this is fixed in r246547
By: Dave Reeve (bzing2) 2010-02-17 03:31:43.000-0600

Nope this is an issue during call setup; specifically when chan_local attempts to optimize itself out of the path.
By: Russell Bryant (russell) 2010-02-17 21:54:12.000-0600

Give this patch a try.  I made it against 1.6.2, but honestly have not tested it yet.  The idea is simple, though.  The whole point of check_bridge() is to see if we need to perform the optimization.  There was already code to make sure we only do the optimization coming from one direction.  The call to ast_bridged_channel() from the other direction is a total waste, so this patch insures we only hit that code in one direction.
By: Dave Reeve (bzing2) 2010-02-18 05:01:27.000-0600

Thanks for that!  Seems your thinking was aligned with mine, ie only do this in one direction.  I have put my patch into production, which in theory should achieve roughly the same result.  I will apply your patch to my code base shortly; unfortunatly I have to wait for a maintance window to deploy it, which in this case is a week Tuesday.  I will let you know the results.  Thanks for your efforts.
By: Leif Madsen (lmadsen) 2010-03-03 09:09:47.000-0600

Did you have a chance to deploy the patch by russell? What were the results? Thanks!
By: Russell Bryant (russell) 2010-03-04 09:03:55.000-0600

bzing, yes you're right, all I did was accomplish the same result as you via a slightly different means.  We should be good to go here.
By: Dave Reeve (bzing2) 2010-03-04 11:51:55.000-0600

I can confirm that my patch (and hence russell's) worked a treat.
By: Digium Subversion (svnbot) 2010-04-02 18:45:57

Repository: asterisk
Revision: 256014

U   branches/1.4/channels/chan_local.c

------------------------------------------------------------------------
r256014 | russell | 2010-04-02 18:45:57 -0500 (Fri, 02 Apr 2010) | 9 lines

Resolve a deadlock that occurs due to a pointless call to ast_bridged_channel()

(closes issue ASTERISK-15636)
Reported by: bzing2
Patches:
     patch.txt uploaded by bzing2 (license 902)
     issue_16840.rev1.diff uploaded by russell (license 2)
Tested by: bzing2, russell

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=256014
By: Digium Subversion (svnbot) 2010-04-02 18:46:46

Repository: asterisk
Revision: 256015

_U  trunk/
U   trunk/channels/chan_local.c

------------------------------------------------------------------------
r256015 | russell | 2010-04-02 18:46:46 -0500 (Fri, 02 Apr 2010) | 16 lines

Merged revisions 256014 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r256014 | russell | 2010-04-02 18:45:56 -0500 (Fri, 02 Apr 2010) | 9 lines
 
 Resolve a deadlock that occurs due to a pointless call to ast_bridged_channel()
 
 (closes issue ASTERISK-15636)
 Reported by: bzing2
 Patches:
       patch.txt uploaded by bzing2 (license 902)
       issue_16840.rev1.diff uploaded by russell (license 2)
 Tested by: bzing2, russell
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=256015
By: Digium Subversion (svnbot) 2010-04-02 18:47:17

Repository: asterisk
Revision: 256016

_U  branches/1.6.0/
U   branches/1.6.0/channels/chan_local.c

------------------------------------------------------------------------
r256016 | russell | 2010-04-02 18:47:16 -0500 (Fri, 02 Apr 2010) | 23 lines

Merged revisions 256015 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r256015 | russell | 2010-04-02 18:46:45 -0500 (Fri, 02 Apr 2010) | 16 lines
 
 Merged revisions 256014 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r256014 | russell | 2010-04-02 18:45:56 -0500 (Fri, 02 Apr 2010) | 9 lines
   
   Resolve a deadlock that occurs due to a pointless call to ast_bridged_channel()
   
   (closes issue ASTERISK-15636)
   Reported by: bzing2
   Patches:
         patch.txt uploaded by bzing2 (license 902)
         issue_16840.rev1.diff uploaded by russell (license 2)
   Tested by: bzing2, russell
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=256016
By: Digium Subversion (svnbot) 2010-04-02 18:47:58

Repository: asterisk
Revision: 256017

_U  branches/1.6.1/
U   branches/1.6.1/channels/chan_local.c

------------------------------------------------------------------------
r256017 | russell | 2010-04-02 18:47:58 -0500 (Fri, 02 Apr 2010) | 23 lines

Merged revisions 256015 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r256015 | russell | 2010-04-02 18:46:45 -0500 (Fri, 02 Apr 2010) | 16 lines
 
 Merged revisions 256014 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r256014 | russell | 2010-04-02 18:45:56 -0500 (Fri, 02 Apr 2010) | 9 lines
   
   Resolve a deadlock that occurs due to a pointless call to ast_bridged_channel()
   
   (closes issue ASTERISK-15636)
   Reported by: bzing2
   Patches:
         patch.txt uploaded by bzing2 (license 902)
         issue_16840.rev1.diff uploaded by russell (license 2)
   Tested by: bzing2, russell
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=256017
By: Digium Subversion (svnbot) 2010-04-02 18:48:29

Repository: asterisk
Revision: 256018

_U  branches/1.6.2/
U   branches/1.6.2/channels/chan_local.c

------------------------------------------------------------------------
r256018 | russell | 2010-04-02 18:48:29 -0500 (Fri, 02 Apr 2010) | 23 lines

Merged revisions 256015 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r256015 | russell | 2010-04-02 18:46:45 -0500 (Fri, 02 Apr 2010) | 16 lines
 
 Merged revisions 256014 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r256014 | russell | 2010-04-02 18:45:56 -0500 (Fri, 02 Apr 2010) | 9 lines
   
   Resolve a deadlock that occurs due to a pointless call to ast_bridged_channel()
   
   (closes issue ASTERISK-15636)
   Reported by: bzing2
   Patches:
         patch.txt uploaded by bzing2 (license 902)
         issue_16840.rev1.diff uploaded by russell (license 2)
   Tested by: bzing2, russell
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=256018