<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/1303/">https://reviewboard.asterisk.org/r/1303/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yep, this will definitely fix it and the code looks great!
My only concern is that it adds more ref counting/locking complexity into the channel driver. This could be avoided with more work by converting the auth container to an ao2_container and having the sip_auth objects become ao2_objects. At that point you could get rid of all the refcounting and locking. Locking and ref counting will still occur but its within the astobj2 code during the link, unlink, and iterators. Copying an auth list would just be a matter of linking objects from one list into another. The global, peer, and dialog auth containers would always be present and only cleared when reloads happened. The only ref counting that would occur would be when dialogs and peers are destroyed, decrementing their auth containers.
Since this is a small patch I don't feel strongly one way or the other. I just wanted to point out the advantages of using all ao2 objects and containers.</pre>
<br />
<p>- David</p>
<br />
<p>On June 30th, 2011, 4:28 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, Russell Bryant and David Vossel.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated June 30, 2011, 4:28 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;">If you use the auth= parameter and do a "sip reload" while there is an ongoing call. The peer->auth data points to free'd memory.
The patch does several things:
1) Puts the authentication list into an ao2 object for reference counting to fix the reported crash during a SIP reload.
2) Converts the authentication list from open coding to AST list macros.
3) Adds display of the global authentication list in "sip show settings".</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;">Ran asterisk under valgrind and attempted to make an authenticated SIP call. Added debug statements on the test system and confirmed that the created dialog got the authentication list and that the list was used to create the challenge response.
Note: My test setup failed to connect because of how my setup was looping back the call. I got a 482 loopback detected. For this patch it was sufficient to determine that the necessary code paths were executed.</pre>
</td>
</tr>
</table>
<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-17939">ASTERISK-17939</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/channels/chan_sip.c <span style="color: grey">(325936)</span></li>
<li>/branches/1.8/channels/sip/include/sip.h <span style="color: grey">(325936)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1303/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>