Summary:	ASTERISK-09898: Incorrect handling of VNAK retransmission
Reporter:	mihai (mihai)	Labels:
Date Opened:	2007-07-18 09:49:27	Date Closed:	2007-07-18 16:06:00
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_iax2
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:
Description:	In vnak_retransmit you can find the following code: if ((f->callno == callno) && iaxs[f->callno] && (f->oseqno >= last)) { send_packet(f); oseqno and last are 8-bit values that wrap around, so if a VNAK retransmission is requested right after a wraparound, this code will behave incorrectly. Most probable effect would be that asterisk would not retransmitting the right frames and the other endpoint would keep asking for them, generating a VNAK storm ****** ADDITIONAL INFORMATION ****** This was discovered in 1.4.8, but similar code exists in 1.2 and trunk
Comments:	By: Russell Bryant (russell) 2007-07-18 10:18:35 I'm saving this little tidbit here ... 10:08 < _mihai_> russellb: btw, here's the way I handle this in iaxclient: 10:09 < _mihai_> if ( (unsigned char)(fh->iseqno - session->rseqno) <= (unsigned char)(sch->frame->oseqno - session->rseqno) ) 10:09 < _mihai_> session->rseqno is the last acknowledged sequence number By: Russell Bryant (russell) 2007-07-18 15:19:16 I'm digging through the VNAK code, looking for things that can go wrong.  If I make any commits related to it, even if not for this specific issue you have reported, I will reference this issue number so that the commits will show up here. By: mihai (mihai) 2007-07-18 15:33:40 Russell, thanks for looking into this. One simple way of fixing this particular issue would be to use the following code in vnak_retransmit if ((f->callno == callno) && iaxs[f->callno] && ((unsigned char)(f->oseqno - last) < 128)) { send_packet(f); } It is pretty much equivalent to the old code, but it takes wraparounds into account By: Russell Bryant (russell) 2007-07-18 15:45:13 In the future, please put any code in attachments marked as code so that they are automatically handled by our license system.  In this case, I think the change is small enough that's it's not a big deal. I had to think about it for a few minutes, but I think that change looks right.  I'm going to commit that. After that, should we use another issue to track remaining VNAK storm problems? By: Digium Subversion (svnbot) 2007-07-18 15:52:14 Repository: asterisk Revision: 75757 ------------------------------------------------------------------------ r75757 | russell | 2007-07-18 15:52:13 -0500 (Wed, 18 Jul 2007) | 5 lines When traversing the queue of frames for possible retransmission after receiving a VNAK, handle sequence number wraparound so that all frames that should be retransmitted actually do get retransmitted. (issue ASTERISK-9898, reported and patched by mihai) ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-07-18 15:52:47 Repository: asterisk Revision: 75759 ------------------------------------------------------------------------ r75759 | russell | 2007-07-18 15:52:47 -0500 (Wed, 18 Jul 2007) | 13 lines Merged revisions 75757 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.2 ........ r75757 | russell | 2007-07-18 16:09:13 -0500 (Wed, 18 Jul 2007) | 5 lines When traversing the queue of frames for possible retransmission after receiving a VNAK, handle sequence number wraparound so that all frames that should be retransmitted actually do get retransmitted. (issue ASTERISK-9898, reported and patched by mihai) ........ ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-07-18 15:53:30 Repository: asterisk Revision: 75761 ------------------------------------------------------------------------ r75761 | russell | 2007-07-18 15:53:29 -0500 (Wed, 18 Jul 2007) | 21 lines Merged revisions 75759 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ................ r75759 | russell | 2007-07-18 16:09:46 -0500 (Wed, 18 Jul 2007) | 13 lines Merged revisions 75757 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.2 ........ r75757 | russell | 2007-07-18 16:09:13 -0500 (Wed, 18 Jul 2007) | 5 lines When traversing the queue of frames for possible retransmission after receiving a VNAK, handle sequence number wraparound so that all frames that should be retransmitted actually do get retransmitted. (issue ASTERISK-9898, reported and patched by mihai) ........ ................ ------------------------------------------------------------------------ By: Russell Bryant (russell) 2007-07-18 16:06:00 I'm going to mark this one as fixed for now as the specific issue here is resolved.  But please let me know if you still have a problem after using this change.  I have a bad feeling that it's not all fixed, but we will see.