<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/2400/">https://reviewboard.asterisk.org/r/2400/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This change makes sense to me. No need to do a DNS lookup if all we want is the port information.
In 11 and trunk, we use the resolved address in attempting to determine if the UA is behind NAT and set the natdetected flag. So, I think the patch there would be slightly different from this one.</pre>
<br />
<div>
<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/2400/diff/1/?file=34745#file34745line16478" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/channels/chan_sip.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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 void check_via(struct sip_pvt *p, struct sip_request *req)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">16475</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_WARNING</span><span class="p">,</span> <span class="s">"Could not resolve socket address for '%s'</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">c</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">16476</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="p">(</span><span class="n">ast_sockaddr_split_hostport</span><span class="p">(</span><span class="n">tmp</span><span class="p">,</span> <span class="o">&</span><span class="n">tmphost</span><span class="p">,</span> <span class="o">&</span><span class="n">tmpport</span><span class="p">,</span> <span class="mi">0</span><span class="p">)</span> <span class="o">&&</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">16476</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">port</span> <span class="o">=</span> <span class="n">STANDARD_SIP_PORT</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">16477</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">tmpport</span> <span class="o">&&</span> <span class="p">(</span><span class="n">port</span> <span class="o">=</span> <span class="n">strtol</span><span class="p">(</span><span class="n">tmpport</span><span class="p">,</span> <span class="o">&</span><span class="n">tmpport2</span><span class="p">,</span> <span class="mi">10</span><span class="p">))</span> <span class="o">&&</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">16477</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span> <span class="k">else</span> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="p">(</span><span class="n">port</span> <span class="o">=</span> <span class="n">ast_sockaddr_port</span><span class="p">(</span><span class="o">&</span><span class="n">tmp</span><span class="p">)))</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">16478</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="o">*</span><span class="n">tmpport2</span> <span class="o">==</span> <span class="sc">'\0'</span> <span class="o">&&</span> <span class="n">port</span> <span class="o">></span> <span class="mi">0</span> <span class="o">&&</span> <span class="n">port</span> <span class="o"><</span> <span class="mi">65536</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Curious if we could use what is in asterisk code for checking if ports are valid, although it is pretty clear as it is.
You could call ast_sockaddr_split_hostport with the flag PARSE_PORT_REQUIRED which would fail immediately without any extra checks needed.
if (!ast_sockaddr_split_hostport(tmp, &tmphost, &tmpport, PARSE_PORT_REQUIRED)) {
ast_log(LOG_WARNING, "Could not split host/port for '%s'\n", c);
}
Upon success of the above, you could also check that the port returned is valid by using ast_parse_arg().
else if (ast_parse_arg(&tmpport, PARSE_UINT32|PARSE_IN_RANGE, &port, 1024, 65535)) {
ast_log(LOG_WARNING, "Invalid port number, '%s', in VIA header\n", tmpport);
port = STANDARD_SIP_PORT;
}
In other parts of the code, when we check for valid ports we are using the range 1024-65535, not 0-65535.</pre>
</div>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">These were just some ideas that I had when looking at this review.</pre>
<p>- elguero</p>
<br />
<p>On March 18th, 2013, 9:16 a.m., wdoekes 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.</div>
<div>By wdoekes.</div>
<p style="color: grey;"><i>Updated March 18, 2013, 9:16 a.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;">The sent-by-addr address in the Via is supposed to get ignored and the sent-by-port is only used if we're not doing force_rport.
Nevertheless did check_via do hostname lookups even though it only wanted to get at the port.
I've changed the code around so it uses ast_sockaddr_split_hostport() instead of ast_sockaddr_resolve_first(). This also removes an ERROR notice which isn't worthy of such severity.
Before, when someone sent something resolvable in the Via:
chan_sip did an DNS A lookup, but ignored the results.
Now:
chan_sip just looks at the port.
Before, when someone sent a bad via header:
ERROR[29769]: netsock2.c:269 ast_sockaddr_resolve: getaddrinfo("127.0.1.1", "5061branch=z9hG4bK-11063-1-0", ...): Servname not supported for ai_socktype
WARNING[29769]: chan_sip.c:16921 check_via: Could not resolve socket address for '127.0.1.1:5061branch=z9hG4bK-11063-1-0'
Now:
WARNING[10229]: chan_sip.c:16928 check_via: Could not split host/port for '127.0.1.1:5061branch=z9hG4bK-11049-1-0'
</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;">57 SIP tests ran successfully with and without the patch.</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/1.8/channels/chan_sip.c <span style="color: grey">(383308)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2400/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>