<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/1214/">https://reviewboard.asterisk.org/r/1214/</a>
</td>
</tr>
</table>
<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/1214/diff/1/?file=16464#file16464line14769" 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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int get_rdnis(struct sip_pvt *p, struct sip_request *oreq, char **name, char **number, int *reason)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">searches:</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">14769</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* If we reach the end of the road, we want to search for in the uri and start over if we find them. */</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;">I usually don't nitpick comment but you accidentally a very important word here.</pre>
</div>
<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/1214/diff/1/?file=16464#file16464line14773" 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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int get_rdnis(struct sip_pvt *p, struct sip_request *oreq, char **name, char **number, int *reason)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">searches:</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">14773</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">((</span><span class="n">term_point</span> <span class="o">=</span> <span class="n">strchr</span><span class="p">(</span><span class="n">uri</span><span class="p">,</span> <span class="sc">';'</span><span class="p">)))</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">14774</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="o">*</span><span class="n">term_point</span><span class="o">++</span> <span class="o">=</span> <span class="sc">'\0'</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">14775</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">repeat_extension</span> <span class="o">=</span> <span class="mi">1</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">14776</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_string_field_set</span><span class="p">(</span><span class="n">p</span><span class="p">,</span> <span class="n">uri_user_parameters</span><span class="p">,</span> <span class="n">term_point</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">14777</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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">14778</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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">14779</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">((</span><span class="n">term_point</span> <span class="o">=</span> <span class="n">strchr</span><span class="p">(</span><span class="n">decoded_uri</span><span class="p">,</span> <span class="sc">';'</span><span class="p">)))</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">14780</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="o">*</span><span class="n">term_point</span><span class="o">++</span> <span class="o">=</span> <span class="sc">'\0'</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">14781</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">repeat_extension</span> <span class="o">=</span> <span class="mi">1</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">14782</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_string_field_set</span><span class="p">(</span><span class="n">p</span><span class="p">,</span> <span class="n">uri_user_parameters</span><span class="p">,</span> <span class="n">term_point</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">14783</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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;">You could reduce some code duplication here by just doing:
if ((term_point = strchr(uri, ';')) || (term_point = strchr(decoded_uri, ';'))) {
*term_point++ = '\0';
repeat_extension = 1;
ast_string_field_set(p, uri_user_parameters, term_point);
}
This would also have the benefit of not trying to find a ';' in the decoded_uri if you've already found one in the (potentially) non-decoded uri.</pre>
</div>
<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/1214/diff/1/?file=16464#file16464line14787" 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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int get_rdnis(struct sip_pvt *p, struct sip_request *oreq, char **name, char **number, int *reason)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">searches:</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">14787</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_debug</span><span class="p">(</span><span class="mi">3</span><span class="p">,</span> <span class="s">"uri contained ';' and could not match. Searching again ignoring post-semicolon.</span><span class="se">\n</span><span class="s">"</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;">Instead of "uri" you should specify the user section of the uri.</pre>
</div>
<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/1214/diff/1/?file=16464#file16464line14788" 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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int get_rdnis(struct sip_pvt *p, struct sip_request *oreq, char **name, char **number, int *reason)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">searches:</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">14788</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">goto</span> <span class="n">searches</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;">This is a situation where I think the use of a goto is more confusing than anything. My initial inclination was to rewrite this as a do-while loop, but I realized that the loop would always run once and at most twice.
I think a better idea would be to place everything below the "searches" label into a function. That way, you can call the function once, check its return value, and then call the function again with the altered user value if necessary.</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On May 11th, 2011, 9:46 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, Mark Michelson, and David Vossel.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated 2011-05-11 09:46:56</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;">This is sort of a natural follow-up to https://reviewboard.asterisk.org/r/1188/
Reading over RFC3261 and talking to mmichelson made the following things clear to me:
1. semi-colons are allowed in the user field of the uri. In fact, the very data I was trying to parse out is described as belonging in the user field by pages 156-158.
2. Theoretically, a user could make an extension something like 2005;bunch-of-uri-user-parameters
3. Since that's the case, simply parsing out this data before it ever gets used by anything based on semicolons could interfere with current behavior.
What I did to address it:
1. In the case of the user field in the uri from the bug...
"4254883646;phone-context=+1;npdi=yes" -- Asterisk needs to match 4254883646. Since this can't be safely removed in parsing, there is now a fail-triggered semi-colon delimited removal
performed in chan_sip's get_destination. If no extensions are made before it reaches the end, we check to see if there was a semicolon in the uri/decoded uri and if there was, we start the searches
over again with the uri terminated at the first semicolon.
2. If one of those uri semicolons is found, we preserve that data within a string field which can be accessed from the channel function.
3. A test was added to check that teluris that are converted into sip uris are read like it appears they should according to RFC3261.</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;">I tested to make sure it didn't interfere with regular dialing behavior of sip phones by using one of my own sip phones.
I tested the basic behavior by forcing the following sip message using sipp:
INVITE sip:2005;phone-context=+1;npdi=yes@127.0.0.1:5060 SIP/2.0
Via: SIP/2.0/UDP 127.0.1.1:5062;branch=z9hG4bK-23024-1-0
From: "Lrrrr Schmrrr" <sip:sipp@127.0.1.1:5062>;tag=1
To: Asterisk <sip:2005:whoop;phone-context=+1;npdi=yes@127.0.0.1:5060>
Call-ID: 1-23024@127.0.1.1
CSeq: 1 OPTIONS
Contact: sip:sipp@127.0.1.1:5062
Max-Forwards: 70
Subject: Asterisk Testsuite
Content-Length: 0
And using this dialplan setup I'd check to make sure values were what I expected:
[sipp]
exten => 2005,1,Answer()
exten => 2005,n,Background(tt-weasels)
exten => 2005,n,NoOp(callerid => ${CALLERID(all)})
exten => 2005,n,NoOp(exten => ${EXTEN})
exten => 2005,n,NoOp(uri => ${CHANNEL(uri)})
exten => 2005,n,NoOp(user_options => ${CHANNEL(uri_user_parameters)})
exten => 2005,n,Wait(1)
exten => 2005,n,HangUp()
yielding these results:
-- Executing [2005@sipp:1] Answer("SIP/sipp-00000000", "") in new stack
-- Executing [2005@sipp:2] BackGround("SIP/sipp-00000000", "tt-weasels") in new stack
-- <SIP/sipp-00000000> Playing 'tt-weasels.gsm' (language 'en')
-- Executing [2005@sipp:3] NoOp("SIP/sipp-00000000", "callerid => "Lrrrr Schmrrr" <sipp>") in new stack
-- Executing [2005@sipp:4] NoOp("SIP/sipp-00000000", "exten => 2005") in new stack
-- Executing [2005@sipp:5] NoOp("SIP/sipp-00000000", "uri => sip:sipp@127.0.1.1:5062") in new stack
-- Executing [2005@sipp:6] NoOp("SIP/sipp-00000000", "user_options => phone-context=+1;npdi=yes") in new stack
-- Executing [2005@sipp:7] Wait("SIP/sipp-00000000", "1") in new stack
-- Executing [2005@sipp:8] Hangup("SIP/sipp-00000000", "") in new stack
I was paranoid that my goto statement would potentially cause a memory leak, so I also compared "memory show summary chan_sip.c" before and after the channel lived for both the case of the normal call and the case where it has to do the repetition with goto. I repeated the process with pbx.c and likewise saw no memory leak (though I thought I did at first when 4 allocations were made to pbx.c, but it turned out to be unrelated since I performed the test while creating hundreds of similar sip channels and saw no additional increases) Still, it's something worth looking at since I'm still rather likely to miss that sort of thing.</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=18344">18344</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">(318337)</span></li>
<li>/branches/1.8/channels/sip/dialplan_functions.c <span style="color: grey">(318337)</span></li>
<li>/branches/1.8/channels/sip/include/sip.h <span style="color: grey">(318337)</span></li>
<li>/branches/1.8/channels/sip/reqresp_parser.c <span style="color: grey">(318337)</span></li>
<li>/branches/1.8/funcs/func_channel.c <span style="color: grey">(318337)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1214/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>