> 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; > > } > > > > > > Torrey Searle wrote: > 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. >
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. - 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
