<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1557/">https://reviewboard.asterisk.org/r/1557/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 8th, 2011, 11:50 a.m., <b>David Vossel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1557/diff/5/?file=21655#file21655line11738" style="color: black; font-weight: bold; text-decoration: underline;">/branches/10/channels/chan_sip.c</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static enum sip_result add_sdp(struct sip_request *resp, struct sip_pvt *p, int oldsdp, int add_audio, int add_t38)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11725</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* we unlink this dialog and link again into the dialogs_rtpcheck container to doesnt add it twice */</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11726</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/*</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11727</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * We unlink this dialog and link again into the</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11728</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * dialogs_rtpcheck container so its not in there twice.</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11729</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> */</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11730</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_lock</span><span class="p">(</span><span class="n">dialogs_rtpcheck</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11726</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_t_unlink</span><span class="p">(</span><span class="n">dialogs_rtpcheck</span><span class="p">,</span> <span class="n">p</span><span class="p">,</span> <span class="s">"unlink pvt into dialogs_rtpcheck container"</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11731</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_t_unlink</span><span class="p">(</span><span class="n">dialogs_rtpcheck</span><span class="p">,</span> <span class="n">p</span><span class="p">,</span> <span class="s">"unlink pvt into dialogs_rtpcheck container"</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11727</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_t_link</span><span class="p">(</span><span class="n">dialogs_rtpcheck</span><span class="p">,</span> <span class="n">p</span><span class="p">,</span> <span class="s">"link pvt into dialogs_rtpcheck container"</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11732</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_t_link</span><span class="p">(</span><span class="n">dialogs_rtpcheck</span><span class="p">,</span> <span class="n">p</span><span class="p">,</span> <span class="s">"link pvt into dialogs_rtpcheck container"</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11733</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_unlock</span><span class="p">(</span><span class="n">dialogs_rtpcheck</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">11728</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11734</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Is p locked during these links/unlinks. If so this is a problem.</pre>
</blockquote>
<p>On November 8th, 2011, 1:07 p.m., <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I see what you mean. Deadlock between dialogs_rtpcheck container lock and the dialog p lock. Actually, p is not always locked here which is itself not good.
The dialogs_rtpcheck container does not care about the p lock. The use of the dialogs_rtpcheck container is as a subset of the dialogs container. There are two places where locking order could have been a problem:
1) In dialog_checkrtp_cb() but that function does a trylock instead of a lock.
2) In change_callid_pvt().
Looking at the change_callid_pvt() function will potentially violate locking order between dialogs and dialog/pvt. It is sometimes called with pvt locked. If it is not called with pvt locked then it is changing the dialog callid without the lock. The change_callid_pvt() routine was created from repeated code sequences so it would not be a new deadlock potential.
Locking in chan_sip seems to be really messy. :)</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I see why we get away with it. We do a try lock in check rtp callback on the sip pvt while the dialogs_rtpcheck container is locked.</pre>
<br />
<p>- David</p>
<br />
<p>On November 8th, 2011, 1:42 p.m., rmudgett wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, David Vossel and schmidts.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated Nov. 8, 2011, 1:42 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="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;">Timing between dialog destruction and a MWI event sending a message could result in a deadlock.
Order of events causing deadlock:
1a) The event subscription system calls the registered callbacks with its list RWLOCK held.
1b) The SIP monitor checks for dialogs needing destruction. It does an ao2_callback that holds the dialogs container lock while searching for dialogs to destroy.
2a) The event subscription SIP callback needs to create a temporary dialog to send out the MWI notification. That temporary dialog needs to be inserted in the dialogs container so it must wait.
2b) The dialog search finds a dialog to destroy and as a result releases the last reference for a peer. The peer destructor attempts to get the subscription RWLOCK but must wait.
3) deadlock
Residual changes for Asterisk v10 branch after https://reviewboard.asterisk.org/r/1564/ commit
and associated dialogs callid hash key change fix.
NOTE THE CHANGE FROM v1.8 to v10 BRANCH.
* Make check_rtp_timeout() return CMP_MATCH if need to delete dialog from
dialogs_rtpcheck. This is an optimization to avoid an unneeded
lock/unlock and object search when using ao2_unlink.
* Prevent crash in check_rtp_timeout() if dialog->rtp is NULL.
* Make pvt_set_needdestroy() protect from possible double entries in
dialogs_needdestroy.
Schmidts please note that change_callid_pvt() is different between v1.8
and v10 for your unleas-the-beast branch.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="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;">It compiles. :)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/10/channels/chan_sip.c <span style="color: grey">(343951)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1557/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>