[asterisk-bugs] [Asterisk 0019403]: [patch] Call pickup deadlock in ast_do_masquerade

[Asterisk Bug Tracker]

A NOTE has been added to this issue. 
====================================================================== 
https://issues.asterisk.org/view.php?id=19403 
====================================================================== 
Reported By:                one47
Assigned To:                
====================================================================== 
Project:                    Asterisk
Issue ID:                   19403
Category:                   Core/Channels
Reproducibility:            always
Severity:                   block
Priority:                   normal
Status:                     new
Asterisk Version:           1.6.2.18 
JIRA:                        
Regression:                 No 
Reviewboard Link:            
SVN Branch (only for SVN checkouts, not tarball releases): N/A 
SVN Revision (number only!):  
Request Review:              
====================================================================== 
Date Submitted:             2011-06-03 04:03 CDT
Last Modified:              2011-06-03 06:40 CDT
====================================================================== 
Summary:                    [patch] Call pickup deadlock in ast_do_masquerade
Description: 
Using *8 call pickup can readily cause a deadlock where ast_do_masquerade()
has a channel locked, when calling ast_channel_free() - ast_channel_free
requests the channels (channel list) lock, which breaks locking order
requirements and caused a deadlock.
====================================================================== 

---------------------------------------------------------------------- 
 (0135673) one47 (reporter) - 2011-06-03 06:40
 https://issues.asterisk.org/view.php?id=19403#c135673 
---------------------------------------------------------------------- 
Sorry to keep being so negative, but deadlock-1.6.patch2 still has a
problem - It will be a million times better I am sure, but...

If another thread comes along and starts waiting on &channels lock, then
it has a chance to jump in between the unlock:
  AST_RWLIST_UNLOCK(&channels);
and
  ast_channel_free(clonechan);

If you are going to unlock origchan, can you just do it as per
masq_locks.diff, which actually ensures we have a legal locking order when
calling ast_channel_free().

Thoughts? I have now load-tested masq_locks.diff, and verified that
unlocking/relocking origchan should not cause any massively obvious issues.
There is only one obvious code path that has 2 channels locked when calling
ast_do_masquerade(), and that will not be masquerading a zombie channel.

The sequence that causes this deadlock to happen or not happen is as
follows:

No deadlock:
   1) SIP calls magic_pickup which sets-up a masq.
   2) Masq happens here in another thread, cannot deadlock.
   3) SIP calls hangup on the ringing channel. CDR handled, channel
freed.

With deadlock:
   1) SIP calls magic_pickup which sets-up a masq.
   2) SIP calls hangup on the ringing channel, it gets marked zombie
      because it is about to be masq'ed.
   3) Masq happens here in another thread, channel freed, might deadlock.

The above sequence also causes 2 different CDR write paths. The zombie
record never has its CDRs written, which fortunately should always be
correct! 

Issue History 
Date Modified    Username       Field                    Change               
====================================================================== 
2011-06-03 06:40 one47          Note Added: 0135673                          
======================================================================