[asterisk-dev] [Code Review] A potential fix for issue 11231

[Mark Michelson]

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

Review request for Asterisk Developers.


Summary
-------

First go read issue 11231. I'll wait.

A problem that has plagued Asterisk's SIP implementation for quite a while is the fact that we only keep track of the latest outgoing CSeq number that we used in a dialog. So if we send out several requests, we will ignore responses for all but the last of the requests we sent. I believe I have found a simple solution to the problem.

With this change, there is a new field added to the sip_pvt struct called ocseqhistory. It is a bitfield. Bit 0 represents the current ocseq. Each bit beyond represents an old CSeq we have used in outgoing responses on the dialog. Set bits represent outstanding responses.

The easiest way to explain is with an example, let's pretend that the ocseqhistory is an 8-bit field. Initially, it is all 0's
00000000
When we send our first outgoing request, we set bit 0.
00000001
When we send out each new request beyond the first, we shift the ocseqhistory by one and set bit 0 again. So if we were to send out two more requests, ocseqhistory would look like this:
00000111

Now, let's imagine that due to network oddities, we get the response to our second request first. We examine the CSeq of the response we just received and subtract it from our saved ocseq. It is 1. Since bit 1 of ocseqhistory is set, we do not ignore the response. Further, since we have now received a response, we clear the bit in ocseqhistory representing our outstanding request. Now ocseqhistory looks like this:
00000101

By doing this, we can keep track of old outgoing sequence numbers in a simple, lightweight manner. The only way this can be a problem is if someone has set up Asterisk to send out more than the number of bits in ocseqhistory at once, which is very unlikely to happen.

Potential bug:
In the current version of the patch, I unconditionally clear the bit in the ocseqhistory when receiving a response. This should really only be done if the response is a final response since we can receive multiple provisional responses potentially.


This addresses bug 11231.
    https://issues.asterisk.org/view.php?id=11231


Diffs
-----

  /branches/1.4/channels/chan_sip.c 202966 

Diff: http://reviewboard.digium.com/r/295/diff


Testing
-------

I used the following dialplan to make Asterisk rapidly send MESSAGE requests to my phone:

exten => 15,1,Answer
exten => 15,n,SendText(ABC)
exten => 15,n,SendText(ABC)
exten => 15,n,SendText(ABC)
exten => 15,n,SendText(ABC)
exten => 15,n,Wait(20)
exten => 15,n,Hangup

Unpatched, this caused a storm of retransmits. With this patch applied, Asterisk sent four MESSAGE requests, received four responses, and did not send any unnecessary retransmissions.


Thanks,

Mark