<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/1225/">https://reviewboard.asterisk.org/r/1225/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 23rd, 2011, 10:05 a.m., <b>wdoekes</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 could be wrong, but the way I read parse_uri (in reqresp_parser), the only time that domain comes out NULL, is when ast_strlen_zero(uri).
So .. I'd move the return -1 to the if-check that already exists above.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ah.. but I missed Davids comments. Yes.. a supplementary ast_strlen_zero(domain)-check is even better.</pre>
<br />
<p>- wdoekes</p>
<br />
<p>On May 23rd, 2011, 9:01 a.m., jrose wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.orgrb/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 jrose.</div>
<p style="color: grey;"><i>Updated 2011-05-23 09:01:59</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;">According to the backtrace, __set_address_from_contact is invoking ast_sockaddr_resolve (via a couple of wrappers) in a way that is causing a segfault when it tries to perform ast_strdup on the str value this function receives. At the same time, it is known that the SIP URI is failing to parse properly. It appears that the improper URI is parsed, lacks the appropriate data, and fails to fill in an expected field (domain) sent in from __set_address_from_contact (chan_sip.c). Then this domain is used as the str value for ast_sockaddr_resolve, which ultimately causes the segfault.
The submitted patch's approach was to simply return a failure when this str value was null from ast_sockaddr_resolve, but I feel that probably obfuscates the problem. For now, I've opted to deal with it at the the level of the invoking function. ast_sockaddr_resolve should never be invoked with a null value for str.
I noticed some peculiar comments around where I ended up putting the patch in though, so a little caution is probably a good thing. I don't see anyway for this to mess things up, but I could be wrong.</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;">Unfortunately, I don't know a proper test setup for this situation. I was thinking sipp with a deliberately botched message, but I don't know what sort of dialog invokes __set_address_from_contact.
I'm going to ask the reporter to test the patch though and see if he ever receives the warning message I've added.</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/view.php?id=18857">18857</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">(319995)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1225/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>