[asterisk-dev] [Code Review] 3657: when using FEC, asterisk considers negative sequence numbers as missing
Torrey Searle
reviewboard at asterisk.org
Mon Jun 23 13:56:12 CDT 2014
> On June 23, 2014, 4:48 p.m., Matt Jordan wrote:
> > trunk/main/udptl.c, lines 538-543
> > <https://reviewboard.asterisk.org/r/3657/diff/1/?file=59974#file59974line538>
> >
> > 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;
> > }
> >
> >
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.
- Torrey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3657/#review12273
-----------------------------------------------------------
On June 20, 2014, 8:47 a.m., Torrey Searle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3657/
> -----------------------------------------------------------
>
> (Updated June 20, 2014, 8:47 a.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: ASTERISK-23908
> https://issues.asterisk.org/jira/browse/ASTERISK-23908
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> 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
>
>
> Diffs
> -----
>
> trunk/main/udptl.c 416335
>
> Diff: https://reviewboard.asterisk.org/r/3657/diff/
>
>
> Testing
> -------
>
> Replayed the T.38 FEC callflow using a version of SIPP patched to understand m=image for pcapplay
>
>
> Thanks,
>
> Torrey Searle
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140623/242c75bb/attachment-0001.html>
More information about the asterisk-dev
mailing list