<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/4178/">https://reviewboard.asterisk.org/r/4178/</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 15th, 2014, 9:22 a.m. CST, <b>Matt Jordan</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/4178/diff/1/?file=68968#file68968line274" style="color: black; font-weight: bold; text-decoration: underline;">branches/13/res/res_pjsip_outbound_publish.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">252</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ao2_callback</span><span class="p">(</span><span class="n">old_states</span><span class="p">,</span> <span class="n">OBJ_NODATA</span> <span class="o">|</span> <span class="n">OBJ_UNLINK</span><span class="p">,</span> <span class="n">destroy_old_state</span><span class="p">,</span> <span class="nb">NULL</span><span class="p">);</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">253</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ao2_ref</span><span class="p">(</span><span class="n">old_states</span><span class="p">,</span> <span class="o">-</span><span class="mi">1</span><span class="p">);</span></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;">I don't really like the pattern that we have here.
We technically have two destructor functions:
* sip_outbound_publish_client_destroy, which destroys the state object
* destroy_old_state, which must only be called on a state object once just prior to it being destroyed
That feels wrong. I would expect that destroy_old_state should just be a part of sip_outbound_publish_client_destroy.
If sip_outbound_publish_client_destroy actually is the only destructor for the object, *and* the container holds the last reference for the object (assuming it isn't being used elsewhere, in which case reference counting will still work just fine), the you should be able to just destroy the container. That will remove the container's reference to its objects, which will cause their destruction.</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;">The problem that I ran into is that pjsip_publishc object holds a reference to the state until unpublish is called. This means the state destructor won't get called until after "unpublishing". "destroy_old_state" is perhaps a bad name for this function. I'll rename it to something more appropriate like cancel_and_unpublish since that is the intent and add comments to make it more clear. Reading this part of the code will then make more sense: Old states no longer in use (the config object has been deleted for instance) are cancelled and unpublished (state ref dec held by pjsip_publishc) and then removed from the memory container (state ref dec on old_states container).
The only other way this might work is by adding a wrapper around the state object. This wrapper is stored in the ao2_container and then when it gets destroyed cancel and unpublish on its associated state gets called thus releasing the state object. Thoughts?</pre>
<br />
<p>- Kevin</p>
<br />
<p>On November 13th, 2014, 2:08 p.m. CST, Kevin Harwell wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Kevin Harwell.</div>
<p style="color: grey;"><i>Updated Nov. 13, 2014, 2:08 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-24514">ASTERISK-24514</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">When using a non-default sorcery wizard (in this instance realtime) for outbound publishes Asterisk will crash after a stack overflow occurs due to the code infinitely recursing. The fix entails removing the outbound publish state dependency from the outbound publish sorcery object and instead keeping an in memory container that can be used to lookup the state when needed.</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;">On top of running the current testsuite tests I also manually tested various configurations and scenarios using a static configuration file as well as dynamic realtime. Verified that the crash no longer occurs and the potentially affected functionality works as expected (for instance, module [re]loading).</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/13/res/res_pjsip_outbound_publish.c <span style="color: grey">(427787)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4178/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>