[Asterisk-code-review] chan sip.c: Fix t38id deadlock potential. (asterisk[13])
Joshua Colp
asteriskteam at digium.com
Thu Mar 17 10:29:00 CDT 2016
Joshua Colp has submitted this change and it was merged.
Change subject: chan_sip.c: Fix t38id deadlock potential.
......................................................................
chan_sip.c: Fix t38id 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: If595e4456cd059d7171880c7f354e844c21b5f5f
---
M channels/chan_sip.c
1 file changed, 81 insertions(+), 22 deletions(-)
Approvals:
Mark Michelson: Looks good to me, approved
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 f369df4..9f6ee21 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1512,6 +1512,7 @@
static void do_stop_session_timer(struct sip_pvt *pvt);
static void stop_reinvite_retry(struct sip_pvt *pvt);
static void stop_retrans_pkt(struct sip_pkt *pkt);
+static void stop_t38_abort_timer(struct sip_pvt *pvt);
/*! \brief Definition of this channel for PBX channel registration */
struct ast_channel_tech sip_tech = {
@@ -7644,13 +7645,13 @@
/* Negotiation can not take place without a valid max_ifp value. */
if (!parameters->max_ifp) {
if (p->t38.state == T38_PEER_REINVITE) {
- AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr"));
+ stop_t38_abort_timer(p);
transmit_response_reliable(p, "488 Not acceptable here", &p->initreq);
}
change_t38_state(p, T38_REJECTED);
break;
} else if (p->t38.state == T38_PEER_REINVITE) {
- AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr"));
+ stop_t38_abort_timer(p);
p->t38.our_parms = *parameters;
/* modify our parameters to conform to the peer's parameters,
* based on the rules in the ITU T.38 recommendation
@@ -7684,7 +7685,7 @@
case AST_T38_REFUSED:
case AST_T38_REQUEST_TERMINATE: /* Shutdown T38 */
if (p->t38.state == T38_PEER_REINVITE) {
- AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr"));
+ stop_t38_abort_timer(p);
change_t38_state(p, T38_REJECTED);
transmit_response_reliable(p, "488 Not acceptable here", &p->initreq);
} else if (p->t38.state == T38_ENABLED) {
@@ -7696,7 +7697,7 @@
struct ast_control_t38_parameters parameters = p->t38.their_parms;
if (p->t38.state == T38_PEER_REINVITE) {
- AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "when you delete the t38id sched, you should dec the refcount for the stored dialog ptr"));
+ stop_t38_abort_timer(p);
parameters.max_ifp = ast_udptl_get_far_max_ifp(p->udptl);
parameters.request_response = AST_T38_REQUEST_NEGOTIATE;
if (p->owner) {
@@ -25298,25 +25299,87 @@
return 0;
}
-/*! \brief Called to deny a T38 reinvite if the core does not respond to our request */
+/*!
+ * \brief Called to deny a T38 reinvite if the core does not respond to our request
+ *
+ * \note Run by the sched thread.
+ */
static int sip_t38_abort(const void *data)
{
- struct sip_pvt *p = (struct sip_pvt *) data;
+ struct sip_pvt *pvt = (struct sip_pvt *) data;
+ struct ast_channel *owner;
- sip_pvt_lock(p);
- /* an application may have taken ownership of the T.38 negotiation on this
- * channel while we were waiting to grab the lock... if it did, the scheduler
- * id will have been reset to -1, which is our indication that we do *not*
- * want to abort the negotiation process
+ owner = sip_pvt_lock_full(pvt);
+ pvt->t38id = -1;
+
+ /*
+ * An application may have taken ownership of the T.38 negotiation
+ * on the channel while we were waiting to grab the lock. If it
+ * did, the T.38 state will have been changed. This is our
+ * indication that we do *not* want to abort the negotiation
+ * process.
*/
- if (p->t38id != -1) {
- change_t38_state(p, T38_REJECTED);
- transmit_response_reliable(p, "488 Not acceptable here", &p->initreq);
- p->t38id = -1;
- dialog_unref(p, "unref the dialog ptr from sip_t38_abort, because it held a dialog ptr");
+ if (pvt->t38.state == T38_PEER_REINVITE) {
+ /* Still waiting for a response on timeout so reject the offer. */
+ change_t38_state(pvt, T38_REJECTED);
+ transmit_response_reliable(pvt, "488 Not acceptable here", &pvt->initreq);
}
- sip_pvt_unlock(p);
+
+ if (owner) {
+ ast_channel_unlock(owner);
+ ast_channel_unref(owner);
+ }
+ sip_pvt_unlock(pvt);
+ dialog_unref(pvt, "t38id complete");
return 0;
+}
+
+/* Run by the sched thread. */
+static int __stop_t38_abort_timer(const void *data)
+{
+ struct sip_pvt *pvt = (void *) data;
+
+ AST_SCHED_DEL_UNREF(sched, pvt->t38id,
+ dialog_unref(pvt, "Stop scheduled t38id"));
+ dialog_unref(pvt, "Stop t38id action");
+ return 0;
+}
+
+static void stop_t38_abort_timer(struct sip_pvt *pvt)
+{
+ dialog_ref(pvt, "Stop t38id action");
+ if (ast_sched_add(sched, 0, __stop_t38_abort_timer, pvt) < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ dialog_unref(pvt, "Failed to schedule stop t38id action");
+ }
+}
+
+/* Run by the sched thread. */
+static int __start_t38_abort_timer(const void *data)
+{
+ struct sip_pvt *pvt = (void *) data;
+
+ AST_SCHED_DEL_UNREF(sched, pvt->t38id,
+ dialog_unref(pvt, "Stop scheduled t38id"));
+
+ dialog_ref(pvt, "Schedule t38id");
+ pvt->t38id = ast_sched_add(sched, 5000, sip_t38_abort, pvt);
+ if (pvt->t38id < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ dialog_unref(pvt, "Failed to schedule t38id");
+ }
+
+ dialog_unref(pvt, "Start t38id action");
+ return 0;
+}
+
+static void start_t38_abort_timer(struct sip_pvt *pvt)
+{
+ dialog_ref(pvt, "Start t38id action");
+ if (ast_sched_add(sched, 0, __start_t38_abort_timer, pvt) < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ dialog_unref(pvt, "Failed to schedule start t38id action");
+ }
}
/*!
@@ -26201,11 +26264,7 @@
transmit_response(p, "100 Trying", req);
if (p->t38.state == T38_PEER_REINVITE) {
- if (p->t38id > -1) {
- /* reset t38 abort timer */
- AST_SCHED_DEL_UNREF(sched, p->t38id, dialog_unref(p, "remove ref for t38id"));
- }
- p->t38id = ast_sched_add(sched, 5000, sip_t38_abort, dialog_ref(p, "passing dialog ptr into sched structure based on t38id for sip_t38_abort."));
+ start_t38_abort_timer(p);
} else if (p->t38.state == T38_ENABLED) {
ast_set_flag(&p->flags[1], SIP_PAGE2_DIALOG_ESTABLISHED);
transmit_response_with_t38_sdp(p, "200 OK", req, (reinvite ? XMIT_RELIABLE : (req->ignore ? XMIT_UNRELIABLE : XMIT_CRITICAL)));
--
To view, visit https://gerrit.asterisk.org/2399
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If595e4456cd059d7171880c7f354e844c21b5f5f
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: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-code-review
mailing list