[asterisk-dev] [Code Review] IAX2 retransmit with encryption enabled fix

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/192/#review541
-----------------------------------------------------------



/trunk/channels/chan_iax2.c
<http://reviewboard.digium.com/r/192/#comment1267>

    I don't think this cast is required.



/trunk/channels/chan_iax2.c
<http://reviewboard.digium.com/r/192/#comment1268>

    trailing whitespace on this line



/trunk/channels/chan_iax2.c
<http://reviewboard.digium.com/r/192/#comment1269>

    You should remove all instances of '&' from this line.
    
    It will not change what happens with the first two arguments, but with how it is written, the value of the 3rd argument is not what you expect.
    
    This can be demonstrated with this test code:
    
    #include <stdio.h>
    
    int main()
    {
        char foo[32] = "";
    
        printf("%p (%lu) and %p (%lu) \n", foo, sizeof(foo), &foo, sizeof(&foo));
    
        return 0;
    }
    
    Output:
    
    0x7fffc9ca1ec0 (32) and 0x7fffc9ca1ec0 (8) 
    



/trunk/channels/iax2-parser.h
<http://reviewboard.digium.com/r/192/#comment1270>

    Please use doxygen style comments, even though the rest of the struct doesn't use them (yet).
    
    Also, for the "encrypt" struct member, you can make this "unsigned int encrypted:1;", to specify that you only need a bit.  Also, once you do that, you should put it with the rest of the bit fields in this struct.


- Russell


On 2009-03-11 11:06:27, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/192/
> -----------------------------------------------------------
> 
> (Updated 2009-03-11 11:06:27)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> If an iax channel is encrypted, and a retransmit frame is sent, that packet's iseqno is updated while it is encrypted.  This causes the entire frame to be corrupted.  When the corrupted frame is sent, the other side decrypts it and sends a VNAK back because the decrypted frame doesn't make any sense.  When we get the VNAK, we look through the sent queue and send the same corrupted frame causing a loop.  
> 
> To fix this, encrypted frames requiring retransmission are decrypted, updated, then re-encrypted.  Since key-rotation may change the key held by the pvt struct, the keys used for encryption/decryption are held within the iax_frame to guarantee they remain correct.
> 
> 
> This addresses bug 0014607.
>     http://bugs.digium.com/view.php?id=0014607
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/iax2-parser.h 180714 
>   /trunk/channels/chan_iax2.c 180714 
> 
> Diff: http://reviewboard.digium.com/r/192/diff
> 
> 
> Testing
> -------
> 
> tested it with and without encryption on, during packet loss of 25%.  Kept call going for several minutes.  
> 
> 
> Thanks,
> 
> David
> 
>