No subject
Fri Sep 2 03:59:05 CDT 2011
r to determine if the peer was already destroyed (we'll get the "b=
ad magic number" error when we try to ref it if it is already destroye=
d). I'm not sure if this gives us anything new though, as the ao2_lock=
in sip_send_mwi_to_peer() should offer the same debug information.</pre>
</blockquote>
</blockquote>
<pre style=3D"margin-left: 1em; white-space: pre-wrap; white-space: -moz-pr=
e-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-=
word;">I don't think we run the risk of ref'ing the peer here while=
it is in the destructor callback (or at least while we're in a potenti=
ally very dangerous state). They both compete for the event lock.
1. If the destructor callback wins, then it will unsubscribe from the event=
prior to mwi_event_cb being called (which is its first action)
2. If the event engine wins, then the destructor callback will block until =
after mwi_event_cb is completely done.
Your latter statement is correct - this was added to try and catch where th=
e peer went bad on us as soon as possible. I'd also note that we care =
enough about not having the peer disappear on us when using sip_send_mwi_to=
_peer in handle_request_subscribe that we ref the peer there (which is also=
probably unnecessary). Thinking through this some more, this doesn't =
really add much, and may give a false layer of security that isn't corr=
ect - so I agree with blowing it out.
In retrospect, it'd be nice if the event callback didn't have a poi=
nter to the peer in the first place, as that'd keep the ownership seman=
tics a bit clearer :-)</pre>
<br />
<p>- mjordan</p>
<br />
<p>On December 6th, 2011, 10:10 a.m., mjordan wrote:</p>
<table bgcolor=3D"#fefadf" width=3D"100%" cellspacing=3D"0" cellpadding=3D"=
8" style=3D"background-image: url('https://reviewboard.asterisk.org/media/r=
b/images/review_request_box_top_bg.png'); background-position: left top; ba=
ckground-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, David Vossel and opticron.</di=
v>
<div>By mjordan.</div>
<p style=3D"color: grey;"><i>Updated Dec. 6, 2011, 10:10 a.m.</i></p>
<h1 style=3D"color: #575012; font-size: 10pt; margin-top: 1.5em;">Descripti=
on </h1>
<table width=3D"100%" bgcolor=3D"#ffffff" cellspacing=3D"0" cellpadding=3D"=
10" style=3D"border: 1px solid #b8b5a0">
<tr>
<td>
<pre style=3D"margin: 0; padding: 0; white-space: pre-wrap; white-space:=
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap=
: break-word;">ASTERISK-18663 originally manifested as a deadlock when sett=
ing 'allowsubscribe=3Dyes', 'callercounter =3D yes' and set=
ting the subscribecontext in chan_sip. When the deadlock was resolved by r=
345063, a crash would occur in chan_sip. This would manifest when an MWI n=
otification was to be sent to a peer, but the peer had been deleted due to =
being dereferenced to a ref count of 0. The root cause of this ended up be=
ing the MWI event subscription being resubscribed to in several places, and=
orphaning the previous event subscription. When an MWI event would occur,=
all of the event subscriptions (including the orphaned subscriptions) woul=
d be notified. This didn't cause any issues until a peer was removed, =
either by pruning realtime SIP peers, unloading chan_sip, etc. When the pe=
er cleaned itself up, it only removes the subscription that it's aware =
of - the orphaned subscriptions would continue to exist and, if a new MWI e=
vent occurred, would crash Asterisk by referencing the deleted peer.
This patch does several things:
1. It resolves the issue in subscribing to the MWI event callback by first =
unsubscribing the old event subscription
2. It more aggressively holds the authpeer in handle_request_subscribe and =
removes some unneeded peer ref'ing / deref'ing. This was done more=
for clarity, as the previous location of deref'ing the authpeer ignore=
d that the relatedpeer, set to the authpeer, was still used later in the me=
thod
3. It fixes a potential bug wherein an authentication result could be posit=
ive, but all failures are assumed to be negative</pre>
</td>
</tr>
</table>
<h1 style=3D"color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing <=
/h1>
<table width=3D"100%" bgcolor=3D"#ffffff" cellspacing=3D"0" cellpadding=3D"=
10" style=3D"border: 1px solid #b8b5a0">
<tr>
<td>
<pre style=3D"margin: 0; padding: 0; white-space: pre-wrap; white-space:=
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap=
: break-word;">Testing was done extensively using 1.8 and 1.8.8.0-rc4. Thi=
s included using two SIP phones with BLF and MWI subscriptions, with multip=
le mailboxes defined for various extensions, and module unloading / reloadi=
ng chan_sip at various times (both before SUBSCRIBE messages were received =
and after multiple SUBSCRIBE messages had been recevied). The patch was al=
so confirmed to resolve the issue by the issue reporter.</pre>
</td>
</tr>
</table>
<div style=3D"margin-top: 1.5em;">
<b style=3D"color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href=3D"https://issues.asterisk.org/jira/browse/ASTERISK-18663">ASTERIS=
K-18663</a>
</div>
<h1 style=3D"color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b>=
</h1>
<ul style=3D"margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/channels/chan_sip.c <span style=3D"color: grey">(347057)=
</span></li>
<li>/branches/1.8/channels/sip/include/sip.h <span style=3D"color: grey">(=
347057)</span></li>
</ul>
<p><a href=3D"https://reviewboard.asterisk.org/r/1610/diff/" style=3D"margi=
n-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============7688674785440306915==--
More information about the asterisk-dev
mailing list