Resolve race condition where channels enter dialplan application before media has...

Testsuite tests will occasionally fail because on reception of a 200 OK SIP response,
an AST_CONTROL_ANSWER frame is queued prior to when media has finished being
negotiated. This is because session supplements are called into before PJSIP's
inv_session code has told us that media has been updated. Sometimes the queued answer
frame is handled by the PBX thread before the ensuing media negotiations occur, causing
a test failure.

As it turns out, there is another place that session supplements could be called into, which is
after media has finished getting negotiated. What this commit introduces is a means for session
supplements to indicate when they wish to be called into when handling an incoming SIP response.
By default, all session supplements will be run at the same point that they were prior to this
commit. However, session supplements may indicate that they wish to be handled earlier than
normal on redirects, or they may indicate they wish to be handled after media has been negotiated.

In this changeset, two session supplements have been updated to indicate a preference for when
they should be run: res_pjsip_diversion executes before handling redirection in order to get
information from the Diversion header, and chan_pjsip now handles responses to INVITEs after
media negotiation to fix the race condition mentioned previously.

ASTERISK-24212 #close
Reported by Matt Jordan

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

Merged revisions 422536 from http://svn.asterisk.org/svn/asterisk/branches/12
........

Merged revisions 422542 from http://svn.asterisk.org/svn/asterisk/branches/13

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

diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index 7680aec..342eabb 100644 (file)
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -145,6 +145,8 @@ static struct ast_sip_session_supplement chan_pjsip_supplement = {
        .session_end = chan_pjsip_session_end,
        .incoming_request = chan_pjsip_incoming_request,
        .incoming_response = chan_pjsip_incoming_response,
+       /* It is important that this supplement runs after media has been negotiated */
+       .response_priority = AST_SIP_SESSION_AFTER_MEDIA,
 };
 
 static int chan_pjsip_incoming_ack(struct ast_sip_session *session, struct pjsip_rx_data *rdata);
@@ -2029,6 +2031,11 @@ static int call_pickup_incoming_request(struct ast_sip_session *session, pjsip_r
        struct ast_features_pickup_config *pickup_cfg = ast_get_chan_features_pickup_config(session->channel);
        struct ast_channel *chan;
 
+       /* We don't care about reinvites */
+       if (session->inv_session->state >= PJSIP_INV_STATE_CONFIRMED) {
+               return 0;
+       }
+
        if (!pickup_cfg) {
                ast_log(LOG_ERROR, "Unable to retrieve pickup configuration options. Unable to detect call pickup extension.\n");
                return 0;
@@ -2071,6 +2078,11 @@ static int pbx_start_incoming_request(struct ast_sip_session *session, pjsip_rx_
 {
        int res;
 
+       /* We don't care about reinvites */
+       if (session->inv_session->state >= PJSIP_INV_STATE_CONFIRMED) {
+               return 0;
+       }
+
        res = ast_pbx_start(session->channel);
 
        switch (res) {

diff --git a/include/asterisk/res_pjsip_session.h b/include/asterisk/res_pjsip_session.h
index 51908f9..010a74d 100644 (file)
--- a/include/asterisk/res_pjsip_session.h
+++ b/include/asterisk/res_pjsip_session.h
@@ -141,6 +141,36 @@ typedef int (*ast_sip_session_response_cb)(struct ast_sip_session *session, pjsi
 typedef int (*ast_sip_session_sdp_creation_cb)(struct ast_sip_session *session, pjmedia_sdp_session *sdp);
 
 /*!
+ * \brief Describes when a supplement should be called into on incoming responses.
+ *
+ * In most cases, session supplements will not need to worry about this because in most cases,
+ * the correct value will be automatically applied. However, there are rare circumstances
+ * when a supplement will want to specify when it should be called.
+ *
+ * The values below are listed in chronological order.
+ */
+enum ast_sip_session_response_priority {
+       /*!
+        * When processing 3XX responses, the supplement is called into before
+        * the redirecting information is processed.
+        */
+       AST_SIP_SESSION_BEFORE_REDIRECTING = (1 << 0),
+       /*!
+        * For responses to INVITE transactions, the supplement is called into
+        * before media is negotiated.
+        *
+        * This priority is applied by default to any session supplement that
+        * does not specify a response priority.
+        */
+       AST_SIP_SESSION_BEFORE_MEDIA = (1 << 1),
+       /*!
+        * For INVITE transactions, the supplement is called into after media
+        * is negotiated.
+        */
+       AST_SIP_SESSION_AFTER_MEDIA = (1 << 2),
+};
+
+/*!
  * \brief A supplement to SIP message processing
  *
  * These can be registered by any module in order to add
@@ -214,6 +244,11 @@ struct ast_sip_session_supplement {
        void (*outgoing_response)(struct ast_sip_session *session, struct pjsip_tx_data *tdata);
        /*! Next item in the list */
        AST_LIST_ENTRY(ast_sip_session_supplement) next;
+       /*!
+        * Determines when the supplement is processed when handling a response.
+        * Defaults to AST_SIP_SESSION_BEFORE_MEDIA
+        */
+       enum ast_sip_session_response_priority response_priority;
 };
 
 enum ast_sip_session_sdp_stream_defer {

diff --git a/res/res_pjsip_diversion.c b/res/res_pjsip_diversion.c
index a27b075..f8aca35 100644 (file)
--- a/res/res_pjsip_diversion.c
+++ b/res/res_pjsip_diversion.c
@@ -325,6 +325,7 @@ static struct ast_sip_session_supplement diversion_supplement = {
        .incoming_response = diversion_incoming_response,
        .outgoing_request = diversion_outgoing_request,
        .outgoing_response = diversion_outgoing_response,
+       .response_priority = AST_SIP_SESSION_BEFORE_REDIRECTING,
 };
 
 static int load_module(void)

diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index c26693f..83be0cd 100644 (file)
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -53,8 +53,10 @@
 
 /* Some forward declarations */
 static void handle_incoming_request(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type);
-static void handle_incoming_response(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type);
-static int handle_incoming(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type);
+static void handle_incoming_response(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type,
+               enum ast_sip_session_response_priority response_priority);
+static int handle_incoming(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type,
+               enum ast_sip_session_response_priority response_priority);
 static void handle_outgoing_request(struct ast_sip_session *session, pjsip_tx_data *tdata);
 static void handle_outgoing_response(struct ast_sip_session *session, pjsip_tx_data *tdata);
 static void handle_outgoing(struct ast_sip_session *session, pjsip_tx_data *tdata);
@@ -483,6 +485,10 @@ int ast_sip_session_register_supplement(struct ast_sip_session_supplement *suppl
        int inserted = 0;
        SCOPED_LOCK(lock, &session_supplements, AST_RWLIST_WRLOCK, AST_RWLIST_UNLOCK);
 
+       if (!supplement->response_priority) {
+               supplement->response_priority = AST_SIP_SESSION_BEFORE_MEDIA;
+       }
+
        AST_RWLIST_TRAVERSE_SAFE_BEGIN(&session_supplements, iter, next) {
                if (iter->priority > supplement->priority) {
                        AST_RWLIST_INSERT_BEFORE_CURRENT(supplement, next);
@@ -1795,27 +1801,27 @@ static void handle_incoming_request(struct ast_sip_session *session, pjsip_rx_da
        }
 }
 
-static void handle_incoming_response(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type)
+static void handle_incoming_response(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type,
+               enum ast_sip_session_response_priority response_priority)
 {
        struct ast_sip_session_supplement *supplement;
        struct pjsip_status_line status = rdata->msg_info.msg->line.status;
 
-       /* Squash all redirect transaction related responses as the supplements have already been invoked */
-       if (type == PJSIP_EVENT_TSX_STATE && PJSIP_IS_STATUS_IN_CLASS(status.code, 300)) {
-               return;
-       }
-
        ast_debug(3, "Response is %d %.*s\n", status.code, (int) pj_strlen(&status.reason),
                        pj_strbuf(&status.reason));
 
        AST_LIST_TRAVERSE(&session->supplements, supplement, next) {
+               if (!(supplement->response_priority & response_priority)) {
+                       continue;
+               }
                if (supplement->incoming_response && does_method_match(&rdata->msg_info.cseq->method.name, supplement->method)) {
                        supplement->incoming_response(session, rdata);
                }
        }
 }
 
-static int handle_incoming(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type)
+static int handle_incoming(struct ast_sip_session *session, pjsip_rx_data *rdata, pjsip_event_id_e type,
+               enum ast_sip_session_response_priority response_priority)
 {
        ast_debug(3, "Received %s\n", rdata->msg_info.msg->type == PJSIP_REQUEST_MSG ?
                        "request" : "response");
@@ -1823,7 +1829,7 @@ static int handle_incoming(struct ast_sip_session *session, pjsip_rx_data *rdata
        if (rdata->msg_info.msg->type == PJSIP_REQUEST_MSG) {
                handle_incoming_request(session, rdata, type);
        } else {
-               handle_incoming_response(session, rdata, type);
+               handle_incoming_response(session, rdata, type, response_priority);
        }
 
        return 0;
@@ -1913,7 +1919,8 @@ static void session_inv_on_state_changed(pjsip_inv_session *inv, pjsip_event *e)
                handle_outgoing(session, e->body.tx_msg.tdata);
                break;
        case PJSIP_EVENT_RX_MSG:
-               handle_incoming(session, e->body.rx_msg.rdata, type);
+               handle_incoming(session, e->body.rx_msg.rdata, type,
+                               AST_SIP_SESSION_BEFORE_MEDIA);
                break;
        case PJSIP_EVENT_TSX_STATE:
                ast_debug(3, "Source of transaction state change is %s\n", pjsip_event_str(e->body.tsx_state.type));
@@ -1923,7 +1930,8 @@ static void session_inv_on_state_changed(pjsip_inv_session *inv, pjsip_event *e)
                        handle_outgoing(session, e->body.tsx_state.src.tdata);
                        break;
                case PJSIP_EVENT_RX_MSG:
-                       handle_incoming(session, e->body.tsx_state.src.rdata, type);
+                       handle_incoming(session, e->body.tsx_state.src.rdata, type,
+                                       AST_SIP_SESSION_BEFORE_MEDIA);
                        break;
                case PJSIP_EVENT_TRANSPORT_ERROR:
                case PJSIP_EVENT_TIMER:
@@ -1965,6 +1973,7 @@ static void session_inv_on_tsx_state_changed(pjsip_inv_session *inv, pjsip_trans
        }
        switch (e->body.tsx_state.type) {
        case PJSIP_EVENT_TX_MSG:
+               handle_outgoing(session, e->body.tsx_state.src.tdata);
                /* When we create an outgoing request, we do not have access to the transaction that
                 * is created. Instead, We have to place transaction-specific data in the tdata. Here,
                 * we transfer the data into the transaction. This way, when we receive a response, we
@@ -1973,6 +1982,8 @@ static void session_inv_on_tsx_state_changed(pjsip_inv_session *inv, pjsip_trans
                tsx->mod_data[session_module.id] = e->body.tsx_state.src.tdata->mod_data[session_module.id];
                break;
        case PJSIP_EVENT_RX_MSG:
+               handle_incoming(session, e->body.tsx_state.src.rdata, e->type,
+                               AST_SIP_SESSION_AFTER_MEDIA);
                if (tsx->method.id == PJSIP_INVITE_METHOD) {
                        if (tsx->role == PJSIP_ROLE_UAC) {
                                if (tsx->state == PJSIP_TSX_STATE_COMPLETED) {
@@ -2004,10 +2015,6 @@ static void session_inv_on_tsx_state_changed(pjsip_inv_session *inv, pjsip_trans
                                        }
                                }
                        }
-               } else {
-                       if (tsx->role == PJSIP_ROLE_UAS && tsx->state == PJSIP_TSX_STATE_TRYING) {
-                               handle_incoming_request(session, e->body.tsx_state.src.rdata, PJSIP_EVENT_TSX_STATE);
-                       }
                }
                if ((cb = ast_sip_mod_data_get(tsx->mod_data, session_module.id,
                                               MOD_DATA_ON_RESPONSE))) {
@@ -2182,7 +2189,8 @@ static pjsip_redirect_op session_inv_on_redirected(pjsip_inv_session *inv, const
                return PJSIP_REDIRECT_STOP;
        }
 
-       handle_incoming(session, e->body.rx_msg.rdata, PJSIP_EVENT_RX_MSG);
+       handle_incoming(session, e->body.rx_msg.rdata, PJSIP_EVENT_RX_MSG,
+                       AST_SIP_SESSION_BEFORE_REDIRECTING);
 
        uri = pjsip_uri_get_uri(target);