<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/1542/">https://reviewboard.asterisk.org/r/1542/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 27th, 2011, 1:45 p.m., <b>wdoekes</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/1542/diff/3/?file=21429#file21429line231" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/res/res_rtp_multicast.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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 int multicast_rtp_write(struct ast_rtp_instance *instance, struct ast_frame *frame)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">231</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">put_unaligned_uint32</span><span class="p">(</span><span class="n">rtpheader</span><span class="p">,</span> <span class="n">htonl</span><span class="p">((</span><span class="mi">2</span> <span class="o"><<</span> <span class="mi">30</span><span class="p">)</span> <span class="o">|</span> <span class="p">(</span><span class="n">codec</span> <span class="o"><<</span> <span class="mi">16</span><span class="p">)</span> <span class="o">|</span> <span class="p">(</span><span class="n">multicast</span><span class="o">-></span><span class="n">seqno</span><span class="o"><span class="hl">++</span></span><span class="p">)</span> <span class="o">|</span> <span class="p">(</span><span class="mi">0</span> <span class="o"><<</span> <span class="mi">23</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">231</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">put_unaligned_uint32</span><span class="p">(</span><span class="n">rtpheader</span><span class="p">,</span> <span class="n">htonl</span><span class="p">((</span><span class="mi">2</span> <span class="o"><<</span> <span class="mi">30</span><span class="p">)</span> <span class="o">|</span> <span class="p">(</span><span class="n">codec</span> <span class="o"><<</span> <span class="mi">16</span><span class="p">)</span> <span class="o">|</span> <span class="p">(</span><span class="n">multicast</span><span class="o">-></span><span class="n">seqno</span><span class="p">)</span> <span class="o">|</span> <span class="p">(</span><span class="mi">0</span> <span class="o"><<</span> <span class="mi">23</span><span class="p">)));</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">232</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">put_unaligned_uint32</span><span class="p">(</span><span class="n">rtpheader</span> <span class="o">+</span> <span class="mi">4</span><span class="p">,</span> <span class="n">htonl</span><span class="p">(</span><span class="n">f</span><span class="o">-></span><span class="n">ts</span> <span class="o">*</span> <span class="mi">8</span><span class="p">));</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">232</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">put_unaligned_uint32</span><span class="p">(</span><span class="n">rtpheader</span> <span class="o">+</span> <span class="mi">4</span><span class="p">,</span> <span class="n">htonl</span><span class="p">(</span><span class="n">f</span><span class="o">-></span><span class="n">ts</span> <span class="o">*</span> <span class="mi">8</span><span class="p">));</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">233</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">put_unaligned_uint32</span><span class="p">(</span><span class="n">rtpheader</span> <span class="o">+</span> <span class="mi">8</span><span class="p">,</span> <span class="n">htonl</span><span class="p">(</span><span class="n">multicast</span><span class="o">-></span><span class="n">ssrc</span><span class="p">));</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">233</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">put_unaligned_uint32</span><span class="p">(</span><span class="n">rtpheader</span> <span class="o">+</span> <span class="mi">8</span><span class="p">,</span> <span class="n">htonl</span><span class="p">(</span><span class="n">multicast</span><span class="o">-></span><span class="n">ssrc</span><span class="p">));</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">234</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">234</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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">235</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* Increment sequence number and wrap to 0 if it overflows 16 bits. */</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">236</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">multicast</span><span class="o">-></span><span class="n">seqno</span> <span class="o">=</span> <span class="mh">0xFFFF</span> <span class="o">&</span> <span class="p">(</span><span class="n">multicast</span><span class="o">-></span><span class="n">seqno</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">237</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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'd rather see something like this:
put_unaligned_uint32(...(multicast->seqno & 0xFFFF)...
This clarifies that we're dealing with 16 bits in the 32 bit int.
And then simply do += 1 below, without any ANDing.
P.S. The 0 << 23 still looks odd. If you're going to keep it, consider moving it between 30 and 16.</pre>
</blockquote>
<p>On October 27th, 2011, 2:11 p.m., <b>rmudgett</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;">Doing it the way it is in the patch ensures that the seqno value is never out of range. If the seqno value is ever used elsewhere in the code, it would need to be masked to 16 bits there as well.</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;">I'll go ahead and move 0 << 23 to the left of codec << 16 before committing since the change seems pretty harmless to me and I get where you are coming from with the order of things.</pre>
<br />
<p>- jrose</p>
<br />
<p>On October 27th, 2011, 12:44 p.m., jrose 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 jrose.</div>
<p style="color: grey;"><i>Updated Oct. 27, 2011, 12:44 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;">Basically what was happening was the sequence number would go above 65535, and rather than rolling back to zero, the extra bits would overflow into the codec bits of the RTP packet. Because of this, if the sequence number goes above 65535, it'll make the codec value change and this can make calls fail.
This patch simply checks the value of the sequence number each time it is incremented and sets it back to 0 if it's above 65535. Shouldn't cause any problems.</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 checked the unpatched version with wireshark to make sure the codec shift was in fact happening and it was. I checked it again after patching and made sure the problem went away and it did. I also checked to see whether the values stayed as expected if I forcibly changed the codec value to something other than 0, and in all cases it behaved as expected.</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-18291">ASTERISK-18291</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/res/res_rtp_multicast.c <span style="color: grey">(341429)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1542/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>