when using FEC, asterisk considers negative sequence numbers as missing
Review Request #3657  Created June 20, 2014 and submitted
Torrey Searle  
ASTERISK23908  
Reviewers  

asteriskdev  
Asterisk 
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
Replayed the T.38 FEC callflow using a version of SIPP patched to understand m=image for pcapplay
Posted (June 20, 2014, 3:03 a.m.)
You may want to attach an actual diff to this review, so we have something to look at ;)
Review request changed
Updated (June 20, 2014, 3:47 a.m.)

 added Diff r1  Show changes
Posted (June 23, 2014, 11:48 a.m.)

trunk/main/udptl.c (Diff revision 1) 
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 innermost 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; }
Ship It!
Review request changed
Updated (June 26, 2014, 8:27 a.m.)
 changed from pending to submitted
Committed in revision 417380
Other reviews