[asterisk-bugs] [Asterisk 0012963]: [patch] chan_iax2 will create multiple sessions when receiving retransmitted NEW

[noreply at bugs.digium.com]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=12963 
====================================================================== 
Reported By:                jpgrayson
Assigned To:                
====================================================================== 
Project:                    Asterisk
Issue ID:                   12963
Category:                   Channels/chan_iax2
Reproducibility:            always
Severity:                   major
Priority:                   normal
Status:                     new
Asterisk Version:           1.4.21 
SVN Branch (only for SVN checkouts, not tarball releases): N/A 
SVN Revision (number only!):  
Disclaimer on File?:        N/A 
Request Review:              
====================================================================== 
Date Submitted:             06-30-2008 18:48 CDT
Last Modified:              07-01-2008 18:27 CDT
====================================================================== 
Summary:                    [patch] chan_iax2 will create multiple sessions when
receiving retransmitted NEW
Description: 
When a client initiates a call with a NEW frame, frequently the client will
send a retransmitted NEW frame prior to receiving the ACCEPT frame from
asterisk. This may result in asterisk receiving duplicate NEW frames from
the same client in short succession.

As of at least 1.4.20, asterisk will ACCEPT and create new call sessions
for both of these NEW frames originating from the same client. This results
in much protocol confusion between asterisk and the client and ultimately
leads to asterisk and/or the client terminating the call.

The correct behavior would be for asterisk to recognize second (and third
and fourth) NEW frames from the same source (socket and callno) and ignore
them.

====================================================================== 

---------------------------------------------------------------------- 
 jpgrayson - 07-01-08 18:27  
---------------------------------------------------------------------- 
I've uploaded another proposed patch that obsoletes the first. This change
is more comprehensive than the first. I'll try to explain and justify the
changes:

1) Changed match() to no longer have the check_dcallno parameter. Instead,
an incoming dcallno of 0 will allow the dcallno to match.

This is symmetric with how callno is matched against cur->peercallno. This
symmetry makes it much easier to predict match()'s semantics. I also think
it makes it easier for the rest of the find_call() related code to be made
correct.

2) Remove check_dcallno parameter from __find_callno(), find_callno(), and
find_callno_locked(). This parameter was a pass-through to match(), so this
change makes sense if you buy the change to match().

Also eliminates the self-proclaimed "hack" of hooking check_dcallno to the
frames_received member of the chan_iax2_pvt struct passed to ao2_find().
Eliminating this hack is a bonus.

3) Remove match_dcallno argument from find_callno() invocation at line
7036. Here it is important to note that dcallno will necessarily equal zero
at this point in the code. In other words, these two lines would be
equivalent:

  fr->callno = find_callno(ntohs(vh->callno) &
http://bugs.digium.com/view.php?id=#0x8000, dcallno, &sin,
new, fd);
  fr->callno = find_callno(ntohs(vh->callno) &
http://bugs.digium.com/view.php?id=#0x8000,       0, &sin,
new, fd);

So the new code will give the same results as previously.

4) Remove match_dcallno argument from find_callno_locked() invocation at
line 7094. Here we see that dcallno and formerly match_dcallno were zero.
The same matching semantic holds.

5) Around line 7180. This is where it gets hairy. Some factoids to help
sort this out:

* For non-full-frames, dcallno is always 0.
* For full frames that are not (PING, LAGRQ, or NEW) if the dcallno is
zero we know there is a problem without having to call find_callno().
* For (PING, LAGRQ, and NEW) frames, dcallno needs to either be 0 or match
the call. This is actually a stronger test than what was done previously.
Previously an incoming PING or LAGRQ would be matched even if its dcallno
was non-zero and unequal to pvt->callno. I assert that this was bogus
protocol.

So what the patched code does is take all of these factors into account in
one conditional test. The only way a dcallno set to 0 will be passed to
find_callno() is if is a frame that is explicitly allowed to have a dcallno
of 0.

6) The next five changes to find_callno_locked() and find_callno() were
already explicitly not checking dcallno and this semantic is preserved.

7) The last change in pvt_cmp_cb() simply removes the now-bogus
frames_received=match_dcallno hack.

I have tested this patch against a rigged version of iaxclient that always
sends out at least one retransmitted NEW frame. Asterisk behaves as
expected in that case -- duplicate NEW frames are ignored because they
match correctly against an already created call session. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
07-01-08 18:27  jpgrayson      Note Added: 0089563                          
======================================================================