[Home]

Summary:ASTERISK-16624: [patch] schedule_delivery calls ast_bridged_channel() on an unlocked channel
Reporter:Ben Winslow (rain)Labels:
Date Opened:2010-08-27 15:11:36Date Closed:2010-09-21 19:08:51
Priority:CriticalRegression?No
Status:Closed/CompleteComponents:Channels/chan_iax2
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) issue17919_v1.4.patch
( 1) issue17919_w_leak_v1.6.2.patch
( 2) issue17919_w_leak_v1.8.patch
Description:Near the beginning of schedule_delivery(), ast_bridged_channel() is called on iaxs[fr->callno]->owner; however, that channel is not locked, which can result in ast_bridged_channel() crashing should owner->tech change to a technology that doesn't implement bridged_channel.  I spoke with briefly with russellb on IRC who agreed that this usage is unsafe.

****** ADDITIONAL INFORMATION ******

My crash occurred on 1.6.2.6 (running on a multiprocessor machine), but the problem appears to exist in 1.6.2.11 as well.  Here's a backtrace:

#0  0x00000000 in ?? ()
#1  0x080a2439 in ast_bridged_channel (chan=0xb033fde0) at channel.c:4738
#2  0x07817356 in schedule_delivery (fr=0xb0659de0, updatehistory=1, fromtrunk=0, tsout=0xb6a84b14) at chan_iax2.c:4009
#3  0x0783c96b in socket_process (thread=0xb6b4c460) at chan_iax2.c:11110
#4  0x0783cf9f in iax2_process_thread (data=0xb6b4c460) at chan_iax2.c:11220
ASTERISK-1  0x0815dd84 in dummy_start (data=0xb6b41950) at utils.c:968
ASTERISK-2  0x0049b832 in start_thread () from /lib/libpthread.so.0
ASTERISK-3  0x001e1e0e in clone () from /lib/libc.so.6

(gdb) frame 1
#1  0x080a2439 in ast_bridged_channel (chan=0xb033fde0) at channel.c:4738
4738                    bridged = bridged->tech->bridged_channel(chan, bridged);
(gdb) print bridged->tech->type
$30 = 0x135870b "SIP"
(gdb) print bridged->tech->bridged_channel
$31 = (struct ast_channel *(* const)(struct ast_channel *, struct ast_channel *)) 0

Line 4737 checks that bridged and bridged->tech->bridged_channel are not NULL, so the pointer to bridged->tech must have changed between those instructions.  None of the other threads were doing anything interesting at the time of the core.
Comments:By: Digium Subversion (svnbot) 2010-09-21 18:55:59

Repository: asterisk
Revision: 288192

U   branches/1.4/channels/chan_iax2.c

------------------------------------------------------------------------
r288192 | rmudgett | 2010-09-21 18:55:58 -0500 (Tue, 21 Sep 2010) | 26 lines

In chan_iax2.c:schedule_delivery() calls ast_bridged_channel() on an unlocked channel.

Near the beginning of schedule_delivery(), ast_bridged_channel() is called
on iaxs[fr->callno]->owner.  However, the channel is not locked, which can
result in ast_bridged_channel() crashing should owner->tech change to a
technology that doesn't implement bridged_channel.

I also fixed the other calls to ast_bridged_channel() in chan_iax2.c since
the owner lock was not held there either.

Converted the existing channel deadlock avoidance to use
iax2_lock_owner().  Using the new function simplified some awkward code.

In the process of fixing the locking on ast_bridged_channel(), I also
found a memory leak in socket_process() for v1.6.2 and v1.8.  The local
struct variable ies.vars is not freed on early/abnormal function exits.

(closes issue ASTERISK-16624)
Reported by: rain
Patches:
     issue17919_v1.4.patch uploaded by rmudgett (license 664)
     issue17919_w_leak_v1.6.2.patch uploaded by rmudgett (license 664)
     issue17919_w_leak_v1.8.patch uploaded by rmudgett (license 664)

Review: https://reviewboard.asterisk.org/r/926/

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

http://svn.digium.com/view/asterisk?view=rev&revision=288192
By: Digium Subversion (svnbot) 2010-09-21 19:03:37

Repository: asterisk
Revision: 288193

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

------------------------------------------------------------------------
r288193 | rmudgett | 2010-09-21 19:03:37 -0500 (Tue, 21 Sep 2010) | 33 lines

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

........
 r288192 | rmudgett | 2010-09-21 18:55:58 -0500 (Tue, 21 Sep 2010) | 26 lines
 
 In chan_iax2.c:schedule_delivery() calls ast_bridged_channel() on an unlocked channel.
 
 Near the beginning of schedule_delivery(), ast_bridged_channel() is called
 on iaxs[fr->callno]->owner.  However, the channel is not locked, which can
 result in ast_bridged_channel() crashing should owner->tech change to a
 technology that doesn't implement bridged_channel.
 
 I also fixed the other calls to ast_bridged_channel() in chan_iax2.c since
 the owner lock was not held there either.
 
 Converted the existing channel deadlock avoidance to use
 iax2_lock_owner().  Using the new function simplified some awkward code.
 
 In the process of fixing the locking on ast_bridged_channel(), I also
 found a memory leak in socket_process() for v1.6.2 and v1.8.  The local
 struct variable ies.vars is not freed on early/abnormal function exits.
 
 (closes issue ASTERISK-16624)
 Reported by: rain
 Patches:
       issue17919_v1.4.patch uploaded by rmudgett (license 664)
       issue17919_w_leak_v1.6.2.patch uploaded by rmudgett (license 664)
       issue17919_w_leak_v1.8.patch uploaded by rmudgett (license 664)
 
 Review: https://reviewboard.asterisk.org/r/926/
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=288193
By: Digium Subversion (svnbot) 2010-09-21 19:06:22

Repository: asterisk
Revision: 288194

_U  branches/1.8/
U   branches/1.8/channels/chan_iax2.c

------------------------------------------------------------------------
r288194 | rmudgett | 2010-09-21 19:06:22 -0500 (Tue, 21 Sep 2010) | 40 lines

Merged revisions 288193 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.6.2

................
 r288193 | rmudgett | 2010-09-21 19:03:37 -0500 (Tue, 21 Sep 2010) | 33 lines
 
 Merged revisions 288192 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r288192 | rmudgett | 2010-09-21 18:55:58 -0500 (Tue, 21 Sep 2010) | 26 lines
   
   In chan_iax2.c:schedule_delivery() calls ast_bridged_channel() on an unlocked channel.
   
   Near the beginning of schedule_delivery(), ast_bridged_channel() is called
   on iaxs[fr->callno]->owner.  However, the channel is not locked, which can
   result in ast_bridged_channel() crashing should owner->tech change to a
   technology that doesn't implement bridged_channel.
   
   I also fixed the other calls to ast_bridged_channel() in chan_iax2.c since
   the owner lock was not held there either.
   
   Converted the existing channel deadlock avoidance to use
   iax2_lock_owner().  Using the new function simplified some awkward code.
   
   In the process of fixing the locking on ast_bridged_channel(), I also
   found a memory leak in socket_process() for v1.6.2 and v1.8.  The local
   struct variable ies.vars is not freed on early/abnormal function exits.
   
   (closes issue ASTERISK-16624)
   Reported by: rain
   Patches:
         issue17919_v1.4.patch uploaded by rmudgett (license 664)
         issue17919_w_leak_v1.6.2.patch uploaded by rmudgett (license 664)
         issue17919_w_leak_v1.8.patch uploaded by rmudgett (license 664)
   
   Review: https://reviewboard.asterisk.org/r/926/
 ........
................

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

http://svn.digium.com/view/asterisk?view=rev&revision=288194
By: Digium Subversion (svnbot) 2010-09-21 19:08:50

Repository: asterisk
Revision: 288195

_U  trunk/
U   trunk/channels/chan_iax2.c

------------------------------------------------------------------------
r288195 | rmudgett | 2010-09-21 19:08:50 -0500 (Tue, 21 Sep 2010) | 47 lines

Merged revisions 288194 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8

................
 r288194 | rmudgett | 2010-09-21 19:06:21 -0500 (Tue, 21 Sep 2010) | 40 lines
 
 Merged revisions 288193 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.6.2
 
 ................
   r288193 | rmudgett | 2010-09-21 19:03:37 -0500 (Tue, 21 Sep 2010) | 33 lines
   
   Merged revisions 288192 via svnmerge from
   https://origsvn.digium.com/svn/asterisk/branches/1.4
   
   ........
     r288192 | rmudgett | 2010-09-21 18:55:58 -0500 (Tue, 21 Sep 2010) | 26 lines
     
     In chan_iax2.c:schedule_delivery() calls ast_bridged_channel() on an unlocked channel.
     
     Near the beginning of schedule_delivery(), ast_bridged_channel() is called
     on iaxs[fr->callno]->owner.  However, the channel is not locked, which can
     result in ast_bridged_channel() crashing should owner->tech change to a
     technology that doesn't implement bridged_channel.
     
     I also fixed the other calls to ast_bridged_channel() in chan_iax2.c since
     the owner lock was not held there either.
     
     Converted the existing channel deadlock avoidance to use
     iax2_lock_owner().  Using the new function simplified some awkward code.
     
     In the process of fixing the locking on ast_bridged_channel(), I also
     found a memory leak in socket_process() for v1.6.2 and v1.8.  The local
     struct variable ies.vars is not freed on early/abnormal function exits.
     
     (closes issue ASTERISK-16624)
     Reported by: rain
     Patches:
           issue17919_v1.4.patch uploaded by rmudgett (license 664)
           issue17919_w_leak_v1.6.2.patch uploaded by rmudgett (license 664)
           issue17919_w_leak_v1.8.patch uploaded by rmudgett (license 664)
     
     Review: https://reviewboard.asterisk.org/r/926/
   ........
 ................
................

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

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