[Asterisk-code-review] chan sip.c: Fix waitid deadlock potential. (asterisk[13])
Anonymous Coward
asteriskteam at digium.com
Thu Mar 17 09:02:42 CDT 2016
Anonymous Coward #1000019 has submitted this change and it was merged.
Change subject: chan_sip.c: Fix waitid deadlock potential.
......................................................................
chan_sip.c: Fix waitid 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.
* Made always run check_pendings() under the scheduler thread so scheduler
ids can be checked safely.
ASTERISK-25023
Change-Id: Ia834d6edd5bdb47c163e4ecf884428a4a8b17d52
---
M channels/chan_sip.c
1 file changed, 80 insertions(+), 17 deletions(-)
Approvals:
Kevin Harwell: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved; Verified
George Joseph: Looks good to me, but someone else must approve
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index ccdf8cf..ea137bb 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1510,6 +1510,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);
+static void stop_reinvite_retry(struct sip_pvt *pvt);
/*! \brief Definition of this channel for PBX channel registration */
struct ast_channel_tech sip_tech = {
@@ -7218,7 +7219,7 @@
but we can't send one while we have "INVITE" outstanding. */
ast_set_flag(&p->flags[0], SIP_PENDINGBYE);
ast_clear_flag(&p->flags[0], SIP_NEEDREINVITE);
- AST_SCHED_DEL_UNREF(sched, p->waitid, dialog_unref(p, "when you delete the waitid sched, you should dec the refcount for the stored dialog ptr"));
+ stop_reinvite_retry(p);
sip_cancel_destroy(p);
/* If we have an ongoing reinvite, there is a chance that we have gotten a provisional
@@ -22900,10 +22901,13 @@
}
}
-/*! \brief Check pending actions on SIP call
+/*!
+ * \brief Check pending actions on SIP call
*
* \note both sip_pvt and sip_pvt's owner channel (if present)
* must be locked for this function.
+ *
+ * \note Run by the sched thread.
*/
static void check_pendings(struct sip_pvt *p)
{
@@ -22938,7 +22942,11 @@
sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
} else if (ast_test_flag(&p->flags[0], SIP_NEEDREINVITE)) {
/* if we can't REINVITE, hold it for later */
- if (p->pendinginvite || p->invitestate == INV_CALLING || p->invitestate == INV_PROCEEDING || p->invitestate == INV_EARLY_MEDIA || p->waitid > -1) {
+ if (p->pendinginvite
+ || p->invitestate == INV_CALLING
+ || p->invitestate == INV_PROCEEDING
+ || p->invitestate == INV_EARLY_MEDIA
+ || p->waitid > -1) {
ast_debug(2, "NOT Sending pending reinvite (yet) on '%s'\n", p->callid);
} else {
ast_debug(2, "Sending pending reinvite on '%s'\n", p->callid);
@@ -22949,10 +22957,39 @@
}
}
-/*! \brief Reset the NEEDREINVITE flag after waiting when we get 491 on a Re-invite
- to avoid race conditions between asterisk servers.
- Called from the scheduler.
-*/
+/* Run by the sched thread. */
+static int __sched_check_pendings(const void *data)
+{
+ struct sip_pvt *pvt = (void *) data;
+ struct ast_channel *owner;
+
+ owner = sip_pvt_lock_full(pvt);
+ check_pendings(pvt);
+ if (owner) {
+ ast_channel_unlock(owner);
+ ast_channel_unref(owner);
+ }
+ sip_pvt_unlock(pvt);
+
+ dialog_unref(pvt, "Check pending actions action");
+ return 0;
+}
+
+static void sched_check_pendings(struct sip_pvt *pvt)
+{
+ dialog_ref(pvt, "Check pending actions action");
+ if (ast_sched_add(sched, 0, __sched_check_pendings, pvt) < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ dialog_unref(pvt, "Failed to schedule check pending actions action");
+ }
+}
+
+/*!
+ * \brief Reset the NEEDREINVITE flag after waiting when we get 491 on a Re-invite
+ * to avoid race conditions between asterisk servers.
+ *
+ * \note Run by the sched thread.
+ */
static int sip_reinvite_retry(const void *data)
{
struct sip_pvt *p = (struct sip_pvt *) data;
@@ -22967,8 +23004,28 @@
ast_channel_unlock(owner);
ast_channel_unref(owner);
}
- dialog_unref(p, "unref the dialog ptr from sip_reinvite_retry, because it held a dialog ptr");
+ dialog_unref(p, "Schedule waitid complete");
return 0;
+}
+
+/* Run by the sched thread. */
+static int __stop_reinvite_retry(const void *data)
+{
+ struct sip_pvt *pvt = (void *) data;
+
+ AST_SCHED_DEL_UNREF(sched, pvt->waitid,
+ dialog_unref(pvt, "Stop scheduled waitid"));
+ dialog_unref(pvt, "Stop reinvite retry action");
+ return 0;
+}
+
+static void stop_reinvite_retry(struct sip_pvt *pvt)
+{
+ dialog_ref(pvt, "Stop reinvite retry action");
+ if (ast_sched_add(sched, 0, __stop_reinvite_retry, pvt) < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ dialog_unref(pvt, "Failed to schedule stop reinvite retry action");
+ }
}
/*!
@@ -23195,7 +23252,7 @@
if (!req->ignore && p->invitestate != INV_CANCELLED) {
sip_cancel_destroy(p);
}
- check_pendings(p);
+ sched_check_pendings(p);
break;
case 180: /* 180 Ringing */
@@ -23253,7 +23310,7 @@
}
ast_rtp_instance_activate(p->rtp);
}
- check_pendings(p);
+ sched_check_pendings(p);
break;
case 181: /* Call Is Being Forwarded */
@@ -23286,7 +23343,7 @@
ast_party_redirecting_free(&redirecting);
sip_handle_cc(p, req, AST_CC_CCNR);
}
- check_pendings(p);
+ sched_check_pendings(p);
break;
case 183: /* Session progress */
@@ -23347,7 +23404,7 @@
ast_queue_control(p->owner, AST_CONTROL_RINGING);
}
}
- check_pendings(p);
+ sched_check_pendings(p);
break;
case 200: /* 200 OK on invite - someone's answering our call */
@@ -23497,7 +23554,7 @@
p->invitestate = INV_TERMINATED;
ast_set_flag(&p->flags[1], SIP_PAGE2_DIALOG_ESTABLISHED);
xmitres = transmit_request(p, SIP_ACK, seqno, XMIT_UNRELIABLE, TRUE);
- check_pendings(p);
+ sched_check_pendings(p);
break;
case 407: /* Proxy authentication */
@@ -23609,7 +23666,7 @@
update_call_counter(p, DEC_CALL_LIMIT);
append_history(p, "Hangup", "Got 487 on CANCEL request from us on call without owner. Killing this dialog.");
}
- check_pendings(p);
+ sched_check_pendings(p);
sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
break;
case 415: /* Unsupported media type */
@@ -23641,6 +23698,7 @@
/* Reset the flag after a while
*/
int wait;
+
/* RFC 3261, if owner of call, wait between 2.1 to 4 seconds,
* if not owner of call, wait 0 to 2 seconds */
if (p->outgoing_call) {
@@ -23648,7 +23706,12 @@
} else {
wait = ast_random() % 2000;
}
- p->waitid = ast_sched_add(sched, wait, sip_reinvite_retry, dialog_ref(p, "passing dialog ptr into sched structure based on waitid for sip_reinvite_retry."));
+ dialog_ref(p, "Schedule waitid for sip_reinvite_retry.");
+ p->waitid = ast_sched_add(sched, wait, sip_reinvite_retry, p);
+ if (p->waitid < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ dialog_ref(p, "Failed to schedule waitid");
+ }
ast_debug(2, "Reinvite race. Scheduled sip_reinvite_retry in %d secs in handle_response_invite (waitid %d, dialog '%s')\n",
wait, p->waitid, p->callid);
}
@@ -26872,7 +26935,7 @@
/* Destroy any pending invites so we won't try to do another
* scheduled reINVITE. */
- AST_SCHED_DEL_UNREF(sched, p->waitid, dialog_unref(p, "decrement refcount from sip_destroy because waitid won't be scheduled"));
+ stop_reinvite_retry(p);
return 1;
}
@@ -28404,7 +28467,7 @@
ast_queue_control(p->owner, AST_CONTROL_SRCCHANGE);
}
}
- check_pendings(p);
+ sched_check_pendings(p);
} else if (p->glareinvite == seqno) {
/* handle ack for the 491 pending sent for glareinvite */
p->glareinvite = 0;
--
To view, visit https://gerrit.asterisk.org/2396
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia834d6edd5bdb47c163e4ecf884428a4a8b17d52
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
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>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-code-review
mailing list