[asterisk-dev] [Code Review] 3657: when using FEC, asterisk considers negative sequence numbers as missing

[Matt Jordan]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3657/#review12273
-----------------------------------------------------------



trunk/main/udptl.c
<https://reviewboard.asterisk.org/r/3657/#comment22411>

    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;
        }
    
    


- Matt Jordan


On June 20, 2014, 3: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, 3: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/1dbc1c5e/attachment.html>