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>

<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&#39;t think we run the risk of ref&#39;ing the peer here while=
 it is in the destructor callback (or at least while we&#39;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&#39;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&#39;t =
really add much, and may give a false layer of security that isn&#39;t corr=
ect - so I agree with blowing it out.

In retrospect, it&#39;d be nice if the event callback didn&#39;t have a poi=
nter to the peer in the first place, as that&#39;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('
b/images/review_request_box_top_bg.png'); background-position: left top; ba=
ckground-repeat: repeat-x; border: 1px black solid;">

<div>Review request for Asterisk Developers, David Vossel and opticron.</di=
<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">
   <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 &#39;allowsubscribe=3Dyes&#39;, &#39;callercounter =3D yes&#39; 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&#39;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&#39;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&#39;ing / deref&#39;ing.  This was done more=
 for clarity, as the previous location of deref&#39;ing the authpeer ignore=
d that the relatedpeer, set to the authpeer, was still used later in the me=
3. It fixes a potential bug wherein an authentication result could be posit=
ive, but all failures are assumed to be negative</pre>

<h1 style=3D"color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing <=
<table width=3D"100%" bgcolor=3D"#ffffff" cellspacing=3D"0" cellpadding=3D"=
10" style=3D"border: 1px solid #b8b5a0">
   <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  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>

<div style=3D"margin-top: 1.5em;">
 <b style=3D"color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>

 <a href=3D"">ASTERIS=


<h1 style=3D"color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b>=
<ul style=3D"margin-left: 3em; padding-left: 0;">

 <li>/branches/1.8/channels/chan_sip.c <span style=3D"color: grey">(347057)=

 <li>/branches/1.8/channels/sip/include/sip.h <span style=3D"color: grey">(=


<p><a href=3D"" style=3D"margi=
n-left: 3em;">View Diff</a></p>




More information about the asterisk-dev mailing list