Fix a problem where chan_sip would ignore "old" but valid responses.

chan_sip has had a problem for quite a long time that would manifest when
Asterisk would send multiple SIP responses on the same dialog before receiving
a response. The problem occurred because chan_sip only kept track of the highest
outgoing sequence number used on the dialog. If Asterisk sent two requests out,
and a response arrived for the first request sent, then Asterisk would ignore
the response. The result was that Asterisk would continue retransmitting the
requests and ignoring the responses until the maximum number of retransmissions
had been reached.

The fix here is to rearrange the code a bit so that instead of simply comparing
the sequence number of the response to our latest outgoing sequence number, we
walk our list of outstanding packets and determine if there is a match. If there is,
we continue. If not, then we ignore the response.

In doing this, I found a few completely useless variables that I have now removed.

(closes issue #11231)
Reported by: flefoll

Review: https://reviewboard.asterisk.org/r/298

git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.4@204243 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index ce92f2f35f96538e0b35508ffdff37f7dcc49ad4..175d5510cf61dcba2fbe9809e45e0c02eb73cb00 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -813,7 +813,6 @@ struct sip_auth {
 #define SIP_PKT_DEBUG          (1 << 0)        /*!< Debug this packet */
 #define SIP_PKT_WITH_TOTAG     (1 << 1)        /*!< This packet has a to-tag */
 #define SIP_PKT_IGNORE                 (1 << 2)        /*!< This is a re-transmit, ignore it */
-#define SIP_PKT_IGNORE_RESP    (1 << 3)        /*!< Resp ignore - ??? */
 #define SIP_PKT_IGNORE_REQ     (1 << 4)        /*!< Req ignore - ??? */
 
 /* T.38 set of flags */
@@ -960,7 +959,6 @@ static struct sip_pvt {
        ast_group_t callgroup;                  /*!< Call group */
        ast_group_t pickupgroup;                /*!< Pickup group */
        int lastinvite;                         /*!< Last Cseq of invite */
-       int lastnoninvite;                      /*!< Last Cseq of non-invite */
        struct ast_flags flags[2];              /*!< SIP_ flags */
        int timer_t1;                           /*!< SIP timer T1, ms rtt */
        unsigned int sipoptions;                /*!< Supported SIP options on the other end */
@@ -1300,7 +1298,7 @@ static void sip_scheddestroy(struct sip_pvt *p, int ms);
 static int sip_cancel_destroy(struct sip_pvt *p);
 static void sip_destroy(struct sip_pvt *p);
 static int __sip_destroy(struct sip_pvt *p, int lockowner);
-static void __sip_ack(struct sip_pvt *p, int seqno, int resp, int sipmethod);
+static int __sip_ack(struct sip_pvt *p, int seqno, int resp, int sipmethod);
 static void __sip_pretend_ack(struct sip_pvt *p);
 static int __sip_semi_ack(struct sip_pvt *p, int seqno, int resp, int sipmethod);
 static int auto_congest(const void *nothing);
@@ -1557,8 +1555,8 @@ static int local_attended_transfer(struct sip_pvt *transferer, struct sip_dual *
 /*------Response handling functions */
 static void handle_response_invite(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int seqno);
 static void handle_response_refer(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int seqno);
-static int handle_response_register(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int ignore, int seqno);
-static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int ignore, int seqno);
+static int handle_response_register(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int seqno);
+static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int seqno);
 
 /*----- RTP interface functions */
 static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp *rtp, struct ast_rtp *vrtp, int codecs, int nat_active);
@@ -2185,7 +2183,7 @@ static int sip_cancel_destroy(struct sip_pvt *p)
 
 /*! \brief Acknowledges receipt of a packet and stops retransmission 
  * called with p locked*/
-static void __sip_ack(struct sip_pvt *p, int seqno, int resp, int sipmethod)
+static int __sip_ack(struct sip_pvt *p, int seqno, int resp, int sipmethod)
 {
        struct sip_pkt *cur, *prev = NULL;
 
@@ -2234,6 +2232,7 @@ static void __sip_ack(struct sip_pvt *p, int seqno, int resp, int sipmethod)
        }
        if (option_debug)
                ast_log(LOG_DEBUG, "Stopping retransmission on '%s' of %s %d: Match %s\n", p->callid, resp ? "Response" : "Request", seqno, res == FALSE ? "Not Found" : "Found");
+       return res;
 }
 
 /*! \brief Pretend to ack all packets
@@ -2258,7 +2257,7 @@ static void __sip_pretend_ack(struct sip_pvt *p)
 static int __sip_semi_ack(struct sip_pvt *p, int seqno, int resp, int sipmethod)
 {
        struct sip_pkt *cur;
-       int res = -1;
+       int res = FALSE;
 
        for (cur = p->packets; cur; cur = cur->next) {
                if (cur->seqno == seqno && ast_test_flag(cur, FLAG_RESPONSE) == resp &&
@@ -2269,7 +2268,7 @@ static int __sip_semi_ack(struct sip_pvt *p, int seqno, int resp, int sipmethod)
                                        ast_log(LOG_DEBUG, "*** SIP TIMER: Cancelling retransmission #%d - %s (got response)\n", cur->retransid, sip_methods[sipmethod].text);
                        }
                        AST_SCHED_DEL(sched, cur->retransid);
-                       res = 0;
+                       res = TRUE;
                        break;
                }
        }
@@ -7698,8 +7697,6 @@ static int transmit_notify_with_sipfrag(struct sip_pvt *p, int cseq, char *messa
        if (!p->initreq.headers)
                initialize_initreq(p, &req);
 
-       p->lastnoninvite = p->ocseq;
-
        return send_request(p, &req, XMIT_RELIABLE, p->ocseq);
 }
 
@@ -12922,7 +12919,7 @@ static void handle_response_refer(struct sip_pvt *p, int resp, char *rest, struc
 }
 
 /*! \brief Handle responses on REGISTER to services */
-static int handle_response_register(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int ignore, int seqno)
+static int handle_response_register(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int seqno)
 {
        int expires, expires_ms;
        struct sip_registry *r;
@@ -13122,11 +13119,12 @@ static void stop_media_flows(struct sip_pvt *p)
 
 /*! \brief Handle SIP response in dialogue */
 /* XXX only called by handle_request */
-static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int ignore, int seqno)
+static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_request *req, int seqno)
 {
        struct ast_channel *owner;
        int sipmethod;
        int res = 1;
+       int ack_res;
        const char *c = get_header(req, "Cseq");
        /* GCC 4.2 complains if I try to cast c as a char * when passing it to ast_skip_nonblanks, so make a copy of it */
        char *c_copy = ast_strdupa(c);
@@ -13143,10 +13141,16 @@ static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_
                owner->hangupcause = hangup_sip2cause(resp);
 
        /* Acknowledge whatever it is destined for */
-       if ((resp >= 100) && (resp <= 199))
-               __sip_semi_ack(p, seqno, 0, sipmethod);
-       else
-               __sip_ack(p, seqno, 0, sipmethod);
+       if ((resp >= 100) && (resp <= 199)) {
+               ack_res = __sip_semi_ack(p, seqno, 0, sipmethod)
+       } else {
+               ack_res = __sip_ack(p, seqno, 0, sipmethod);
+       }
+
+       if (ack_res == FALSE) {
+               append_history(p, "Ignore", "Ignoring this retransmit\n");
+               return;
+       }
 
        /* If this is a NOTIFY for a subscription clear the flag that indicates that we have a NOTIFY pending */
        if (!p->owner && sipmethod == SIP_NOTIFY && p->pendinginvite) 
@@ -13223,7 +13227,7 @@ static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_
                                        }
                                }
                        } else if (sipmethod == SIP_REGISTER) 
-                               res = handle_response_register(p, resp, rest, req, ignore, seqno);
+                               res = handle_response_register(p, resp, rest, req, seqno);
                        else if (sipmethod == SIP_BYE) {                /* Ok, we're ready to go */
                                ast_set_flag(&p->flags[0], SIP_NEEDDESTROY);
                                ast_clear_flag(&p->flags[1], SIP_PAGE2_DIALOG_ESTABLISHED);
@@ -13240,7 +13244,7 @@ static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_
                        else if (sipmethod == SIP_REFER)
                                handle_response_refer(p, resp, rest, req, seqno);
                        else if (p->registry && sipmethod == SIP_REGISTER)
-                               res = handle_response_register(p, resp, rest, req, ignore, seqno);
+                               res = handle_response_register(p, resp, rest, req, seqno);
                        else if (sipmethod == SIP_BYE) {
                                if (ast_strlen_zero(p->authname)) {
                                        ast_log(LOG_WARNING, "Asked to authenticate %s, to %s:%d but we have no matching peer!\n",
@@ -13261,7 +13265,7 @@ static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_
                        if (sipmethod == SIP_INVITE)
                                handle_response_invite(p, resp, rest, req, seqno);
                        else if (p->registry && sipmethod == SIP_REGISTER) 
-                               res = handle_response_register(p, resp, rest, req, ignore, seqno);
+                               res = handle_response_register(p, resp, rest, req, seqno);
                        else {
                                ast_log(LOG_WARNING, "Forbidden - maybe wrong password on authentication for %s\n", msg);
                                ast_set_flag(&p->flags[0], SIP_NEEDDESTROY);    
@@ -13269,7 +13273,7 @@ static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_
                        break;
                case 404: /* Not found */
                        if (p->registry && sipmethod == SIP_REGISTER)
-                               res = handle_response_register(p, resp, rest, req, ignore, seqno);
+                               res = handle_response_register(p, resp, rest, req, seqno);
                        else if (sipmethod == SIP_INVITE)
                                handle_response_invite(p, resp, rest, req, seqno);
                        else if (owner)
@@ -13281,7 +13285,7 @@ static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_
                        else if (sipmethod == SIP_REFER)
                                handle_response_refer(p, resp, rest, req, seqno);
                        else if (p->registry && sipmethod == SIP_REGISTER)
-                               res = handle_response_register(p, resp, rest, req, ignore, seqno);
+                               res = handle_response_register(p, resp, rest, req, seqno);
                        else if (sipmethod == SIP_BYE) {
                                if (ast_strlen_zero(p->authname)) {
                                        ast_log(LOG_WARNING, "Asked to authenticate %s, to %s:%d but we have no matching peer!\n",
@@ -13299,7 +13303,7 @@ static void handle_response(struct sip_pvt *p, int resp, char *rest, struct sip_
                        if (sipmethod == SIP_INVITE)
                                handle_response_invite(p, resp, rest, req, seqno);
                        else if (sipmethod == SIP_REGISTER) 
-                               res = handle_response_register(p, resp, rest, req, ignore, seqno);
+                               res = handle_response_register(p, resp, rest, req, seqno);
                        else if (sipmethod == SIP_BYE) {
                                ast_set_flag(&p->flags[0], SIP_NEEDDESTROY); 
                                if (option_debug)
@@ -16098,7 +16102,7 @@ static int handle_request(struct sip_pvt *p, struct sip_request *req, struct soc
        /* Get the command XXX */
 
        cmd = req->rlPart1;
-       e = req->rlPart2;
+       e = ast_skip_blanks(req->rlPart2);
 
        /* Save useragent of the client */
        useragent = get_header(req, "User-Agent");
@@ -16108,36 +16112,32 @@ static int handle_request(struct sip_pvt *p, struct sip_request *req, struct soc
        /* Find out SIP method for incoming request */
        if (req->method == SIP_RESPONSE) {      /* Response to our request */
                /* Response to our request -- Do some sanity checks */  
+               if (ast_strlen_zero(e)) {
+                       return 0;
+               }
+               if (sscanf(e, "%d %n", &respid, &len) != 1) {
+                       ast_log(LOG_WARNING, "Invalid response: '%s'\n", e);
+                       return 0;
+               }
+               if (respid <= 0) {
+                       ast_log(LOG_WARNING, "Invalid SIP response code: '%d'\n", respid);
+                       return 0;
+               }
                if (!p->initreq.headers) {
                        if (option_debug)
                                ast_log(LOG_DEBUG, "That's odd...  Got a response on a call we dont know about. Cseq %d Cmd %s\n", seqno, cmd);
                        ast_set_flag(&p->flags[0], SIP_NEEDDESTROY);
                        return 0;
-               } else if (p->ocseq && (p->ocseq < seqno) && (seqno != p->lastnoninvite)) {
+               }
+               if (p->ocseq && (p->ocseq < seqno)) {
                        if (option_debug)
                                ast_log(LOG_DEBUG, "Ignoring out of order response %d (expecting %d)\n", seqno, p->ocseq);
                        return -1;
-               } else if (p->ocseq && (p->ocseq != seqno) && (seqno != p->lastnoninvite)) {
-                       /* ignore means "don't do anything with it" but still have to 
-                          respond appropriately  */
-                       ignore = TRUE;
-                       ast_set_flag(req, SIP_PKT_IGNORE);
-                       ast_set_flag(req, SIP_PKT_IGNORE_RESP);
-                       append_history(p, "Ignore", "Ignoring this retransmit\n");
-               } else if (e) {
-                       e = ast_skip_blanks(e);
-                       if (sscanf(e, "%d %n", &respid, &len) != 1) {
-                               ast_log(LOG_WARNING, "Invalid response: '%s'\n", e);
-                       } else {
-                               if (respid <= 0) {
-                                       ast_log(LOG_WARNING, "Invalid SIP response code: '%d'\n", respid);
-                                       return 0;
-                               }
-                               /* More SIP ridiculousness, we have to ignore bogus contacts in 100 etc responses */
-                               if ((respid == 200) || ((respid >= 300) && (respid <= 399)))
-                                       extract_uri(p, req);
-                               handle_response(p, respid, e + len, req, ignore, seqno);
+               } else {
+                       if ((respid == 200) || ((respid >= 300) && (respid <= 399))) {
+                               extract_uri(p, req);
                        }
+                       handle_response(p, respid, e + len, req, seqno);
                }
                return 0;
        }