chan_sip.c: Fix session timers deadlock potential.

This patch is part of a series to resolve deadlocks in chan_sip.c.

Stopping a scheduled event can result in a deadlock if the scheduled event
is running when you try to stop the event.  If you hold a lock needed by
the scheduled event while trying to stop the scheduled event then a
deadlock can happen.  The general strategy for resolving the deadlock
potential is to push the actual starting and stopping of the scheduled
events off onto the scheduler/do_monitor() thread by scheduling an
immediate one shot scheduled event.  Some restructuring may be needed
because the code may assume that the start/stop of the scheduled events is
immediate.

ASTERISK-25023

Change-Id: I6d65269151ba95e0d8fe4e9e611881cde2ab4900

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index edcd66e4bd5431df6341e7122b80a3c5012095a0..792090a5abe8e33fc9b0ddfa96b60ff23c8e2dba 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1488,7 +1488,6 @@ static void change_t38_state(struct sip_pvt *p, int state);
 
 /*------ Session-Timers functions --------- */
 static void proc_422_rsp(struct sip_pvt *p, struct sip_request *rsp);
-static int  proc_session_timer(const void *vp);
 static void stop_session_timer(struct sip_pvt *p);
 static void start_session_timer(struct sip_pvt *p);
 static void restart_session_timer(struct sip_pvt *p);
@@ -1512,6 +1511,7 @@ static int __sip_subscribe_mwi_do(struct sip_subscription_mwi *mwi);
 
 /* Scheduler id start/stop/reschedule functions. */
 static void stop_provisional_keepalive(struct sip_pvt *pvt);
+static void do_stop_session_timer(struct sip_pvt *pvt);
 
 /*! \brief Definition of this channel for PBX channel registration */
 struct ast_channel_tech sip_tech = {
@@ -3288,7 +3288,8 @@ static void do_dialog_unlink_sched_items(struct sip_pvt *dialog)
                dialog_unref(dialog, "Stop scheduled t38id"));
 
        if (dialog->stimer) {
-               stop_session_timer(dialog);
+               dialog->stimer->st_active = FALSE;
+               do_stop_session_timer(dialog);
        }
 }
 
@@ -6529,12 +6530,8 @@ static void sip_pvt_dtor(void *vdoomed)
        ast_debug(3, "Destroying SIP dialog %s\n", p->callid);
 
        /* Destroy Session-Timers if allocated */
-       if (p->stimer) {
-               p->stimer->quit_flag = 1;
-               stop_session_timer(p);
-               ast_free(p->stimer);
-               p->stimer = NULL;
-       }
+       ast_free(p->stimer);
+       p->stimer = NULL;
 
        if (sip_debug_test_pvt(p))
                ast_verbose("Really destroying SIP dialog '%s' Method: %s\n", p->callid, sip_methods[p->method].text);
@@ -7166,7 +7163,7 @@ static int sip_hangup(struct ast_channel *ast)
                                p->invitestate = INV_TERMINATED;
                        }
                } else {        /* Call is in UP state, send BYE */
-                       if (p->stimer->st_active == TRUE) {
+                       if (p->stimer) {
                                stop_session_timer(p);
                        }
 
@@ -7339,8 +7336,8 @@ static int sip_answer(struct ast_channel *ast)
                ast_set_flag(&p->flags[1], SIP_PAGE2_DIALOG_ESTABLISHED);
                /* RFC says the session timer starts counting on 200,
                 * not on INVITE. */
-               if (p->stimer->st_active == TRUE) {
-                       start_session_timer(p);
+               if (p->stimer) {
+                       restart_session_timer(p);
                }
        }
        sip_pvt_unlock(p);
@@ -8732,13 +8729,13 @@ static struct sip_st_dlg* sip_st_alloc(struct sip_pvt *const p)
                return p->stimer;
        }
 
-       if (!(stp = ast_calloc(1, sizeof(struct sip_st_dlg))))
+       if (!(stp = ast_calloc(1, sizeof(struct sip_st_dlg)))) {
                return NULL;
+       }
+       stp->st_schedid = -1;           /* Session-Timers ast_sched scheduler id */
 
        p->stimer = stp;
 
-       stp->st_schedid = -1;           /* Session-Timers ast_sched scheduler id */
-
        return p->stimer;
 }
 
@@ -25991,7 +25988,7 @@ static int handle_request_invite(struct sip_pvt *p, struct sip_request *req, str
        /* Check if OLI/ANI-II is present in From: */
        parse_oli(req, p->owner);
 
-       if (reinvite && p->stimer->st_active == TRUE) {
+       if (reinvite && p->stimer) {
                restart_session_timer(p);
        }
 
@@ -29216,41 +29213,110 @@ static void acl_change_stasis_cb(void *data, struct stasis_subscription *sub,
        restart_monitor();
 }
 
-/*! \brief Session-Timers: Restart session timer */
-static void restart_session_timer(struct sip_pvt *p)
+/*!
+ * \brief Session-Timers: Process session refresh timeout event
+ *
+ * \note Run by the sched thread.
+ */
+static int proc_session_timer(const void *vp)
 {
-       if (p->stimer->st_active == TRUE) {
-               ast_debug(2, "Session timer stopped: %d - %s\n", p->stimer->st_schedid, p->callid);
-               AST_SCHED_DEL_UNREF(sched, p->stimer->st_schedid,
-                               dialog_unref(p, "Removing session timer ref"));
-               start_session_timer(p);
+       struct sip_pvt *p = (struct sip_pvt *) vp;
+       struct sip_st_dlg *stimer = p->stimer;
+       int res = 0;
+
+       ast_assert(stimer != NULL);
+
+       ast_debug(2, "Session timer expired: %d - %s\n", stimer->st_schedid, p->callid);
+
+       if (!p->owner) {
+               goto return_unref;
        }
-}
 
+       if ((stimer->st_active != TRUE) || (ast_channel_state(p->owner) != AST_STATE_UP)) {
+               goto return_unref;
+       }
 
-/*! \brief Session-Timers: Stop session timer */
-static void stop_session_timer(struct sip_pvt *p)
+       if (stimer->st_ref == SESSION_TIMER_REFRESHER_US) {
+               res = 1;
+               if (T38_ENABLED == p->t38.state) {
+                       transmit_reinvite_with_sdp(p, TRUE, TRUE);
+               } else {
+                       transmit_reinvite_with_sdp(p, FALSE, TRUE);
+               }
+       } else {
+               struct ast_channel *owner;
+
+               ast_log(LOG_WARNING, "Session-Timer expired - %s\n", p->callid);
+
+               owner = sip_pvt_lock_full(p);
+               if (owner) {
+                       send_session_timeout(owner, "SIPSessionTimer");
+                       ast_softhangup_nolock(owner, AST_SOFTHANGUP_DEV);
+                       ast_channel_unlock(owner);
+                       ast_channel_unref(owner);
+               }
+               sip_pvt_unlock(p);
+       }
+
+return_unref:
+       if (!res) {
+               /* Session timer processing is no longer needed. */
+               ast_debug(2, "Session timer stopped: %d - %s\n",
+                       stimer->st_schedid, p->callid);
+               /* Don't pass go, don't collect $200.. we are the scheduled
+                * callback. We can rip ourself out here. */
+               stimer->st_schedid = -1;
+               stimer->st_active = FALSE;
+
+               /* If we are not asking to be rescheduled, then we need to release our
+                * reference to the dialog. */
+               dialog_unref(p, "Session timer st_schedid complete");
+       }
+
+       return res;
+}
+
+static void do_stop_session_timer(struct sip_pvt *pvt)
 {
-       if (p->stimer->st_active == TRUE) {
-               p->stimer->st_active = FALSE;
-               ast_debug(2, "Session timer stopped: %d - %s\n", p->stimer->st_schedid, p->callid);
-               AST_SCHED_DEL_UNREF(sched, p->stimer->st_schedid,
-                               dialog_unref(p, "removing session timer ref"));
+       struct sip_st_dlg *stimer = pvt->stimer;
+
+       if (-1 < stimer->st_schedid) {
+               ast_debug(2, "Session timer stopped: %d - %s\n",
+                       stimer->st_schedid, pvt->callid);
+               AST_SCHED_DEL_UNREF(sched, stimer->st_schedid,
+                       dialog_unref(pvt, "Stop scheduled session timer st_schedid"));
        }
 }
 
+/* Run by the sched thread. */
+static int __stop_session_timer(const void *data)
+{
+       struct sip_pvt *pvt = (void *) data;
 
-/*! \brief Session-Timers: Start session timer */
-static void start_session_timer(struct sip_pvt *p)
+       do_stop_session_timer(pvt);
+       dialog_unref(pvt, "Stop session timer action");
+       return 0;
+}
+
+/*! \brief Session-Timers: Stop session timer */
+static void stop_session_timer(struct sip_pvt *pvt)
 {
-       unsigned int timeout_ms;
+       struct sip_st_dlg *stimer = pvt->stimer;
 
-       if (p->stimer->st_schedid > -1) {
-               /* in the event a timer is already going, stop it */
-               ast_debug(2, "Session timer stopped: %d - %s\n", p->stimer->st_schedid, p->callid);
-               AST_SCHED_DEL_UNREF(sched, p->stimer->st_schedid,
-                       dialog_unref(p, "unref stimer->st_schedid from dialog"));
+       stimer->st_active = FALSE;
+       dialog_ref(pvt, "Stop session timer action");
+       if (ast_sched_add(sched, 0, __stop_session_timer, pvt) < 0) {
+               /* Uh Oh.  Expect bad behavior. */
+               dialog_unref(pvt, "Failed to schedule stop session timer action");
        }
+}
+
+/* Run by the sched thread. */
+static int __start_session_timer(const void *data)
+{
+       struct sip_pvt *pvt = (void *) data;
+       struct sip_st_dlg *stimer = pvt->stimer;
+       unsigned int timeout_ms;
 
        /*
         * RFC 4028 Section 10
@@ -29261,97 +29327,50 @@ static void start_session_timer(struct sip_pvt *p)
         * interval is RECOMMENDED.
         */
 
-       timeout_ms = (1000 * p->stimer->st_interval);
-       if (p->stimer->st_ref == SESSION_TIMER_REFRESHER_US) {
+       timeout_ms = (1000 * stimer->st_interval);
+       if (stimer->st_ref == SESSION_TIMER_REFRESHER_US) {
                timeout_ms /= 2;
        } else {
                timeout_ms -= MIN(timeout_ms / 3, 32000);
        }
 
-       p->stimer->st_schedid = ast_sched_add(sched, timeout_ms, proc_session_timer,
-                       dialog_ref(p, "adding session timer ref"));
+       /* in the event a timer is already going, stop it */
+       do_stop_session_timer(pvt);
 
-       if (p->stimer->st_schedid < 0) {
-               dialog_unref(p, "removing session timer ref");
-               ast_log(LOG_ERROR, "ast_sched_add failed - %s\n", p->callid);
+       dialog_ref(pvt, "Schedule session timer st_schedid");
+       stimer->st_schedid = ast_sched_add(sched, timeout_ms, proc_session_timer, pvt);
+       if (stimer->st_schedid < 0) {
+               dialog_unref(pvt, "Failed to schedule session timer st_schedid");
        } else {
-               p->stimer->st_active = TRUE;
-               ast_debug(2, "Session timer started: %d - %s %ums\n", p->stimer->st_schedid, p->callid, timeout_ms);
+               ast_debug(2, "Session timer started: %d - %s %ums\n",
+                       stimer->st_schedid, pvt->callid, timeout_ms);
        }
-}
 
+       dialog_unref(pvt, "Start session timer action");
+       return 0;
+}
 
-/*! \brief Session-Timers: Process session refresh timeout event */
-static int proc_session_timer(const void *vp)
+/*! \brief Session-Timers: Start session timer */
+static void start_session_timer(struct sip_pvt *pvt)
 {
-       struct sip_pvt *p = (struct sip_pvt *) vp;
-       int res = 0;
-
-       if (!p->stimer) {
-               ast_log(LOG_WARNING, "Null stimer in proc_session_timer - %s\n", p->callid);
-               goto return_unref;
-       }
-
-       ast_debug(2, "Session timer expired: %d - %s\n", p->stimer->st_schedid, p->callid);
-
-       if (!p->owner) {
-               goto return_unref;
-       }
-
-       if ((p->stimer->st_active != TRUE) || (ast_channel_state(p->owner) != AST_STATE_UP)) {
-               goto return_unref;
-       }
-
-       if (p->stimer->st_ref == SESSION_TIMER_REFRESHER_US) {
-               res = 1;
-               if (T38_ENABLED == p->t38.state) {
-                       transmit_reinvite_with_sdp(p, TRUE, TRUE);
-               } else {
-                       transmit_reinvite_with_sdp(p, FALSE, TRUE);
-               }
-       } else {
-               if (p->stimer->quit_flag) {
-                       goto return_unref;
-               }
-               ast_log(LOG_WARNING, "Session-Timer expired - %s\n", p->callid);
-               sip_pvt_lock(p);
-               while (p->owner && ast_channel_trylock(p->owner)) {
-                       sip_pvt_unlock(p);
-                       usleep(1);
-                       if (p->stimer && p->stimer->quit_flag) {
-                               goto return_unref;
-                       }
-                       sip_pvt_lock(p);
-               }
+       struct sip_st_dlg *stimer = pvt->stimer;
 
-               send_session_timeout(p->owner, "SIPSessionTimer");
-               ast_softhangup_nolock(p->owner, AST_SOFTHANGUP_DEV);
-               ast_channel_unlock(p->owner);
-               sip_pvt_unlock(p);
+       stimer->st_active = TRUE;
+       dialog_ref(pvt, "Start session timer action");
+       if (ast_sched_add(sched, 0, __start_session_timer, pvt) < 0) {
+               /* Uh Oh.  Expect bad behavior. */
+               dialog_unref(pvt, "Failed to schedule start session timer action");
        }
+}
 
-return_unref:
-       if (!res) {
-               /* An error occurred.  Stop session timer processing */
-               if (p->stimer) {
-                       ast_debug(2, "Session timer stopped: %d - %s\n", p->stimer->st_schedid, p->callid);
-                       /* Don't pass go, don't collect $200.. we are the scheduled
-                        * callback. We can rip ourself out here. */
-                       p->stimer->st_schedid = -1;
-                       /* Calling stop_session_timer is nice for consistent debug
-                        * logs. */
-                       stop_session_timer(p);
-               }
-
-               /* If we are not asking to be rescheduled, then we need to release our
-                * reference to the dialog. */
-               dialog_unref(p, "removing session timer ref");
+/*! \brief Session-Timers: Restart session timer */
+static void restart_session_timer(struct sip_pvt *p)
+{
+       if (p->stimer->st_active == TRUE) {
+               start_session_timer(p);
        }
-
-       return res;
 }
 
-
 /*! \brief Session-Timers: Function for parsing Min-SE header */
 int parse_minse (const char *p_hdrval, int *const p_interval)
 {

diff --git a/channels/sip/include/sip.h b/channels/sip/include/sip.h
index d60f49ecbe9fb1c7901bdb1a7a5a04e743969281..c8854b58e62d34cc588e1629641d5153913579eb 100644 (file)
--- a/channels/sip/include/sip.h
+++ b/channels/sip/include/sip.h
@@ -959,7 +959,6 @@ struct sip_st_dlg {
        int st_cached_max_se;              /*!< Session-Timers cached Session-Expires */
        enum st_mode st_cached_mode;       /*!< Session-Timers cached M.O. */
        enum st_refresher st_cached_ref;   /*!< Session-Timers session refresher */
-       unsigned char quit_flag:1;         /*!< Stop trying to lock; just quit */
 };