<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/3657/">https://reviewboard.asterisk.org/r/3657/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 23rd, 2014, 11:48 a.m. CDT, <b>Matt Jordan</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/3657/diff/1/?file=59974#file59974line538" style="color: black; font-weight: bold; text-decoration: underline;">trunk/main/udptl.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 int udptl_rx_packet(struct ast_udptl *s, uint8_t *buf, unsigned int len)</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">538</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">539</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="cm">/* only repair buffers that actually exist! */</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">540</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">seq_no</span> <span class="o"><=</span> <span class="p">(</span><span class="n">s</span><span class="o">-></span><span class="n">rx</span><span class="p">[</span><span class="n">l</span><span class="p">].</span><span class="n">fec_span</span> <span class="o">*</span> <span class="n">s</span><span class="o">-></span><span class="n">rx</span><span class="p">[</span><span class="n">l</span><span class="p">].</span><span class="n">fec_entries</span><span class="p">)</span> <span class="o">-</span> <span class="n">m</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">541</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">continue</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">542</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </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">543</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'm not very familiar with the UDPTL code here, so this is more a dump of me trying to figure out what the error in the code is and what this patch does. There's some questions at the end of this, and it is quite possible that my analysis leading up to that is incorrect (this is not the most trivial of code in the code base...)
>From a previous point in the code, we define fec_span as being an unconstrained integer (but hopefully small):
/* The span is defined as an unconstrained integer, but will never be more
than a small value. */
if (ptr + 2 > len)
return -1;
if (buf[ptr++] != 1)
return -1;
span = buf[ptr++];
s->rx[x].fec_span = span;
And we define fec_entries as being the number of entries of FEC:
/* The number of entries is defined as a length, but will only ever be a small
value. Treat it as such. */
if (ptr + 1 > len)
return -1;
entries = buf[ptr++];
if (entries > MAX_FEC_ENTRIES) {
return -1;
}
s->rx[x].fec_entries = entries;
Both of these values are passed into us via the buffer in udptl_rx_packet. Okay, so far, so good. We have some arbitrary ints we pulled out. (Interestingly, when doing most comparisons - particularly with the product of these two values - we generally only ever look at the first 4 bits. This is also true of the seq_no (where x = seq_no & UDPTL_BUF_MASK). So these values are definitely not expected to be very large, and the masking helps to keep large values from causing unexpected (and bad) calculations. Cool.)
The code in question uses an outer loop of l = seq_no (x); l != seq_no - (16 - (seq_no's fec_span * seq_no's fec_entries), decrementing each time (so working backwards through sequence numbers based on the number of spans/entries we should search backwards for). The first inner loop iterates from 0 through each of the outer loop's fec_entries.
Using your example of fec_span = 3; fec_entries = 4; seq_no = 5; we'd have something like this:
for (l = 5; l != 1; l--) {
for (m = 0; m < rx[l].fec_entries; m++) {
/* Patch goes here */
}
}
Since your patch is performing a boundary check on seq_no with respect to rx[l].fec_span/fec_entries - m, then, assuming fec_span and fec_entries for some value of l is also 3 and 4 (since that's what your issue described), we'd have the following values for (rx[l].fec_span * rx[l].fec_entries - m):
span * entries m
Iteration 0: 12 - 0 = 12
Iteration 1: 12 - 1 = 11
Iteration 2: 12 - 2 = 10
Iteration 3: 12 - 3 = 9
What happens if we didn't have the patch here?
In that case, we hit the *next* inner loop. This iterates k, with values of k = (l + m - (rx[l].fec_span * rx[l].fec_entries). However, since l + m is going to be at most 9, we will certainly calculate (for 0 <= m < 3) a negative value for k.
That means indexing into s->rx using a negative value of k, which is probably not what we want.
With your patch, since the sequence number is less than the product of span * entries, we'd skip those calculations in the inner loop.
A few questions from this admittedly convoluted line of thinking:
(1) Should use use the UDPTL_BUF_MASK on the product of fec_span and fec_entries? Likewise, should use use the value of x as opposed to seq_no, as it has already been masked with UDPTL_BUF_MASK?
(2) The negative indexing appears to actually take place within the next inner-most loop. With the patch, all calculations in that inner loop are abandoned. Should that be the case? Could this instead be written as:
for (which = -1, k = (limit - s->rx[l].fec_span * s->rx[l].fec_entries) & UDPTL_BUF_MASK; k != limit; k = (k + s->rx[l].fec_entries) & UDPTL_BUF_MASK) {
if (k < 0) {
continue;
}
if (s->rx[k].buf_len <= 0)
which = (which == -1) ? k : -2;
}
</pre>
</blockquote>
<p>On June 23rd, 2014, 1:56 p.m. CDT, <b>Torrey Searle</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;">a packet can only be recovered if all span number of packets are present so my test before traversing the span guarantees that all packets should be present in the ring buffer before checking if all but one of the packets in the span are there, the & UDPTL_BUF_MASK is a poor man's modulo 16 for a circular buffer, and as a result, k will never be negative in your revised patch as the bufmask will always nuke the signed bit.
</pre>
</blockquote>
<p>On June 23rd, 2014, 2:25 p.m. CDT, <b>Torrey Searle</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;">As and Extra explanation, the innermost loop is checking if all but one packets in the span are present. in the case of packets 0 - 4, things work because there are 2 negative non-existing packets in the span. When you hit packet 5, only the 3rd packet is negative, so it incorrectly tries to repair packet with a negative sequence number.
Adding the test to see if the circular buffer is sufficently full prevents this issue from happening.</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;">Thanks for the explanation - that works for me.</pre>
<br />
<p>- Matt</p>
<br />
<p>On June 20th, 2014, 3:47 a.m. CDT, Torrey Searle wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Torrey Searle.</div>
<p style="color: grey;"><i>Updated June 20, 2014, 3:47 a.m.</i></p>
<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-23908">ASTERISK-23908</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">When using FEC error correction with span=3 and entries=4 asterisk will try to repair in packet with sequence number 5 because it will see that packet -4 is missing. The result is that Asterisk will forward garbage packets that can possibly kill the fax.
This patch adds a check to see if the sequence number is valid before checking if the packet is missing</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;">Replayed the T.38 FEC callflow using a version of SIPP patched to understand m=image for pcapplay</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>trunk/main/udptl.c <span style="color: grey">(416335)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3657/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>