[Home]

Summary:ASTERISK-15186: [patch] handle_incoming() incorrectly sets p->method to SIP_ACK
Reporter:Onno Molenkamp (omolenkamp)Labels:
Date Opened:2009-11-20 14:43:53.000-0600Date Closed:2009-12-22 11:10:37.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) asterisk-out-of-order-ack.patch
( 1) issue16295_v2.diff
( 2) issue16295.diff
Description:When handle_incoming() in chan_sip.c processes an ACK packet, it will set p->method to SIP_ACK (line 20567 in 1.6.1.10), just like it does for other types of packets. In case of an ACK, this doesn't seem right, as the field is supposed to contain the "SIP method that opened this dialog".

In specific cases, this can also lead to actual problems. An example of this is the following conversation for setting up a call by a device that needs to authenticate:

1. in:  INVITE (CSeq 1)
2. out: 401 Unauthorized (CSeq 1)
3. out: 401 Unauthorized (CSeq 1 - retransmission)
4. in:  ACK (CSeq 1)
5. in:  INVITE (CSeq 2)
6. out: 100 Trying (CSeq 2)
7. in:  ACK (CSeq 1)
8. out: 100 Trying (CSeq 2)
...

Both after packet 4 and 7, p->method will be changed to SIP_ACK. The first time, this doesn't cause any problems as the following packet 5 will set it to SIP_INVITE again, but after packet 7 (caused by the retransmission of packet 2) that isn't even fully processed because of its CSeq, it does:

During the transmission of the following packets (100, ..., 200) p->method will still be set to SIP_ACK, which results in the omission of some headers, some of which are mandatory. ("Contact" in the "200 OK" for example, usually added by respprep() if resp_needs_contact() tells it to)

The missing Contact-header will then cause some clients, like snom 370 phones, to send their ACK for the 200 directly to the server specified in the To-header, instead of to Asterisk. Asterisk will then close the call after a few seconds because it never receives the ACK.
Comments:By: Onno Molenkamp (omolenkamp) 2009-11-20 15:01:36.000-0600

The attached patch illustrates what could to be changed to fix the problem. I don't think this is the best possible solution however; design-wise, it's probably better to move the code that handles ACKs up a bit, from the switch at the end of the function to somewhere close to the code that handles SIP_RESPONSEs and old packets, return the function from there when an ACK is processed, and only after that set p->method.
By: David Vossel (dvossel) 2009-12-08 11:05:13.000-0600

I don't understand why we set p->method there at all. We do it in sip_alloc, why would we ever need to do it again?  The patch I uploaded is the same as your's except without the p->method being set in handle_incoming().
By: David Vossel (dvossel) 2009-12-17 16:58:21.000-0600

I uploaded a new patch.  This one restores the p->method if an ACK is not matched.  Will this work?
By: Digium Subversion (svnbot) 2009-12-22 10:58:21.000-0600

Repository: asterisk
Revision: 236062

U   branches/1.4/channels/chan_sip.c

------------------------------------------------------------------------
r236062 | dvossel | 2009-12-22 10:58:20 -0600 (Tue, 22 Dec 2009) | 11 lines

fixes issue with p->method incorrectly set to ACK

It is possible for a second ACK to come in for a retransmitted message.
If an ack does not match an unacked message in our queue, restore the previous
p->method as this ACK is completely ignored.

(closes issue ASTERISK-15186)
Reported by: omolenkamp
Patches:
     issue16295_v2.diff uploaded by dvossel (license 671)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=236062
By: Digium Subversion (svnbot) 2009-12-22 11:00:10.000-0600

Repository: asterisk
Revision: 236063

_U  trunk/
U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r236063 | dvossel | 2009-12-22 11:00:09 -0600 (Tue, 22 Dec 2009) | 18 lines

Merged revisions 236062 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r236062 | dvossel | 2009-12-22 10:58:19 -0600 (Tue, 22 Dec 2009) | 11 lines
 
 fixes issue with p->method incorrectly set to ACK
 
 It is possible for a second ACK to come in for a retransmitted message.
 If an ack does not match an unacked message in our queue, restore the previous
 p->method as this ACK is completely ignored.
 
 (closes issue ASTERISK-15186)
 Reported by: omolenkamp
 Patches:
       issue16295_v2.diff uploaded by dvossel (license 671)
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=236063
By: Digium Subversion (svnbot) 2009-12-22 11:04:39.000-0600

Repository: asterisk
Revision: 236064

_U  branches/1.6.2/
U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r236064 | dvossel | 2009-12-22 11:04:38 -0600 (Tue, 22 Dec 2009) | 25 lines

Merged revisions 236063 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r236063 | dvossel | 2009-12-22 11:00:08 -0600 (Tue, 22 Dec 2009) | 18 lines
 
 Merged revisions 236062 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r236062 | dvossel | 2009-12-22 10:58:19 -0600 (Tue, 22 Dec 2009) | 11 lines
   
   fixes issue with p->method incorrectly set to ACK
   
   It is possible for a second ACK to come in for a retransmitted message.
   If an ack does not match an unacked message in our queue, restore the previous
   p->method as this ACK is completely ignored.
   
   (closes issue ASTERISK-15186)
   Reported by: omolenkamp
   Patches:
         issue16295_v2.diff uploaded by dvossel (license 671)
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=236064
By: Digium Subversion (svnbot) 2009-12-22 11:06:48.000-0600

Repository: asterisk
Revision: 236065

_U  branches/1.6.1/
U   branches/1.6.1/channels/chan_sip.c

------------------------------------------------------------------------
r236065 | dvossel | 2009-12-22 11:06:48 -0600 (Tue, 22 Dec 2009) | 25 lines

Merged revisions 236063 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r236063 | dvossel | 2009-12-22 11:00:08 -0600 (Tue, 22 Dec 2009) | 18 lines
 
 Merged revisions 236062 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r236062 | dvossel | 2009-12-22 10:58:19 -0600 (Tue, 22 Dec 2009) | 11 lines
   
   fixes issue with p->method incorrectly set to ACK
   
   It is possible for a second ACK to come in for a retransmitted message.
   If an ack does not match an unacked message in our queue, restore the previous
   p->method as this ACK is completely ignored.
   
   (closes issue ASTERISK-15186)
   Reported by: omolenkamp
   Patches:
         issue16295_v2.diff uploaded by dvossel (license 671)
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=236065
By: Digium Subversion (svnbot) 2009-12-22 11:10:36.000-0600

Repository: asterisk
Revision: 236066

_U  branches/1.6.0/
U   branches/1.6.0/channels/chan_sip.c

------------------------------------------------------------------------
r236066 | dvossel | 2009-12-22 11:10:36 -0600 (Tue, 22 Dec 2009) | 25 lines

Merged revisions 236063 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r236063 | dvossel | 2009-12-22 11:00:08 -0600 (Tue, 22 Dec 2009) | 18 lines
 
 Merged revisions 236062 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r236062 | dvossel | 2009-12-22 10:58:19 -0600 (Tue, 22 Dec 2009) | 11 lines
   
   fixes issue with p->method incorrectly set to ACK
   
   It is possible for a second ACK to come in for a retransmitted message.
   If an ack does not match an unacked message in our queue, restore the previous
   p->method as this ACK is completely ignored.
   
   (closes issue ASTERISK-15186)
   Reported by: omolenkamp
   Patches:
         issue16295_v2.diff uploaded by dvossel (license 671)
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=236066