[asterisk-commits] chan sip.c: Fix t38id deadlock potential. (asterisk[master])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Thu Mar 17 11:10:07 CDT 2016
Anonymous Coward #1000019 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:
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 fe06085..40859a6 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1514,6 +1514,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 = {
@@ -7649,13 +7650,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
@@ -7689,7 +7690,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) {
@@ -7701,7 +7702,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) {
@@ -25337,25 +25338,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");
+ }
}
/*!
@@ -26240,11 +26303,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/2414
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If595e4456cd059d7171880c7f354e844c21b5f5f
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-commits
mailing list