[Asterisk-code-review] chan sip.c: Fix session timers deadlock potential. (asterisk[master])
Anonymous Coward
asteriskteam at digium.com
Thu Mar 17 08:19:34 CDT 2016
Anonymous Coward #1000019 has submitted this change and it was merged.
Change subject: chan_sip.c: Fix session timers deadlock potential.
......................................................................
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
---
M channels/chan_sip.c
M channels/sip/include/sip.h
2 files changed, 132 insertions(+), 114 deletions(-)
Approvals:
Anonymous Coward #1000019: Verified
Joshua Colp: Looks good to me, approved
George Joseph: Looks good to me, but someone else must approve
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index edcd66e..792090a 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1488,7 +1488,6 @@
/*------ 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 @@
/* 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 @@
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 @@
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 @@
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 @@
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,12 +8729,12 @@
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 @@
/* 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 @@
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;
+ }
+
+ 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)
+{
+ 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;
+
+ 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 *p)
+static void 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;
+
+ 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");
}
}
-
-/*! \brief Session-Timers: Start session timer */
-static void start_session_timer(struct sip_pvt *p)
+/* 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;
-
- 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"));
- }
/*
* RFC 4028 Section 10
@@ -29261,96 +29327,49 @@
* 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;
+ struct sip_st_dlg *stimer = pvt->stimer;
- if (!p->stimer) {
- ast_log(LOG_WARNING, "Null stimer in proc_session_timer - %s\n", p->callid);
- goto return_unref;
+ 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");
}
-
- 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);
- }
-
- send_session_timeout(p->owner, "SIPSessionTimer");
- ast_softhangup_nolock(p->owner, AST_SOFTHANGUP_DEV);
- ast_channel_unlock(p->owner);
- sip_pvt_unlock(p);
- }
-
-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");
- }
-
- return res;
}
+/*! \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);
+ }
+}
/*! \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 d60f49e..c8854b5 100644
--- a/channels/sip/include/sip.h
+++ b/channels/sip/include/sip.h
@@ -959,7 +959,6 @@
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 */
};
--
To view, visit https://gerrit.asterisk.org/2410
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6d65269151ba95e0d8fe4e9e611881cde2ab4900
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
More information about the asterisk-code-review
mailing list