[Asterisk-code-review] chan sip.c: Fix provisional keepalive sched id deadlock. (asterisk[13])
Anonymous Coward
asteriskteam at digium.com
Thu Mar 17 07:35:28 CDT 2016
Anonymous Coward #1000019 has submitted this change and it was merged.
Change subject: chan_sip.c: Fix provisional_keepalive_sched_id deadlock.
......................................................................
chan_sip.c: Fix provisional_keepalive_sched_id deadlock.
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: I98a694fd42bc81436c83aa92de03226e6e4e3f48
---
M channels/chan_sip.c
1 file changed, 80 insertions(+), 27 deletions(-)
Approvals:
Kevin Harwell: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved; Verified
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 92ca92a..8516c25 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1508,6 +1508,9 @@
static int sip_subscribe_mwi_do(const void *data);
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);
+
/*! \brief Definition of this channel for PBX channel registration */
struct ast_channel_tech sip_tech = {
.type = "SIP",
@@ -4506,25 +4509,14 @@
}
}
+/* Run by the sched thread. */
static int send_provisional_keepalive_full(struct sip_pvt *pvt, int with_sdp)
{
const char *msg = NULL;
struct ast_channel *chan;
int res = 0;
- int old_sched_id = pvt->provisional_keepalive_sched_id;
chan = sip_pvt_lock_full(pvt);
- /* Check that nothing has changed while we were waiting for the lock */
- if (old_sched_id != pvt->provisional_keepalive_sched_id) {
- /* Keepalive has been cancelled or rescheduled, clean up and leave */
- if (chan) {
- ast_channel_unlock(chan);
- chan = ast_channel_unref(chan);
- }
- sip_pvt_unlock(pvt);
- dialog_unref(pvt, "dialog ref for provisional keepalive");
- return 0;
- }
if (!pvt->last_provisional || !strncasecmp(pvt->last_provisional, "100", 3)) {
msg = "183 Session Progress";
@@ -4537,25 +4529,23 @@
transmit_response(pvt, S_OR(msg, pvt->last_provisional), &pvt->initreq);
}
res = PROVIS_KEEPALIVE_TIMEOUT;
- }
-
- if (chan) {
- ast_channel_unlock(chan);
- chan = ast_channel_unref(chan);
- }
-
- if (!res) {
+ } else {
pvt->provisional_keepalive_sched_id = -1;
}
sip_pvt_unlock(pvt);
+ if (chan) {
+ ast_channel_unlock(chan);
+ ast_channel_unref(chan);
+ }
if (!res) {
- dialog_unref(pvt, "dialog ref for provisional keepalive");
+ dialog_unref(pvt, "Schedule provisional keepalive complete");
}
return res;
}
+/* Run by the sched thread. */
static int send_provisional_keepalive(const void *data)
{
struct sip_pvt *pvt = (struct sip_pvt *) data;
@@ -4563,6 +4553,7 @@
return send_provisional_keepalive_full(pvt, 0);
}
+/* Run by the sched thread. */
static int send_provisional_keepalive_with_sdp(const void *data)
{
struct sip_pvt *pvt = (void *) data;
@@ -4570,12 +4561,73 @@
return send_provisional_keepalive_full(pvt, 1);
}
+/* Run by the sched thread. */
+static int __update_provisional_keepalive_full(struct sip_pvt *pvt, int with_sdp)
+{
+ AST_SCHED_DEL_UNREF(sched, pvt->provisional_keepalive_sched_id,
+ dialog_unref(pvt, "Stop scheduled provisional keepalive for update"));
+
+ sip_pvt_lock(pvt);
+ if (pvt->invitestate < INV_COMPLETED) {
+ /* Provisional keepalive is still needed. */
+ dialog_ref(pvt, "Schedule provisional keepalive");
+ pvt->provisional_keepalive_sched_id = ast_sched_add(sched, PROVIS_KEEPALIVE_TIMEOUT,
+ with_sdp ? send_provisional_keepalive_with_sdp : send_provisional_keepalive,
+ pvt);
+ if (pvt->provisional_keepalive_sched_id < 0) {
+ dialog_unref(pvt, "Failed to schedule provisional keepalive");
+ }
+ }
+ sip_pvt_unlock(pvt);
+
+ dialog_unref(pvt, "Update provisional keepalive action");
+ return 0;
+}
+
+/* Run by the sched thread. */
+static int __update_provisional_keepalive(const void *data)
+{
+ struct sip_pvt *pvt = (void *) data;
+
+ return __update_provisional_keepalive_full(pvt, 0);
+}
+
+/* Run by the sched thread. */
+static int __update_provisional_keepalive_with_sdp(const void *data)
+{
+ struct sip_pvt *pvt = (void *) data;
+
+ return __update_provisional_keepalive_full(pvt, 1);
+}
+
static void update_provisional_keepalive(struct sip_pvt *pvt, int with_sdp)
{
- AST_SCHED_DEL_UNREF(sched, pvt->provisional_keepalive_sched_id, dialog_unref(pvt, "when you delete the provisional_keepalive_sched_id, you should dec the refcount for the stored dialog ptr"));
+ dialog_ref(pvt, "Update provisional keepalive action");
+ if (ast_sched_add(sched, 0,
+ with_sdp ? __update_provisional_keepalive_with_sdp : __update_provisional_keepalive,
+ pvt) < 0) {
+ dialog_unref(pvt, "Failed to schedule update provisional keepalive action");
+ }
+}
- pvt->provisional_keepalive_sched_id = ast_sched_add(sched, PROVIS_KEEPALIVE_TIMEOUT,
- with_sdp ? send_provisional_keepalive_with_sdp : send_provisional_keepalive, dialog_ref(pvt, "Increment refcount to pass dialog pointer to sched callback"));
+/* Run by the sched thread. */
+static int __stop_provisional_keepalive(const void *data)
+{
+ struct sip_pvt *pvt = (void *) data;
+
+ AST_SCHED_DEL_UNREF(sched, pvt->provisional_keepalive_sched_id,
+ dialog_unref(pvt, "Stop scheduled provisional keepalive"));
+ dialog_unref(pvt, "Stop provisional keepalive action");
+ return 0;
+}
+
+static void stop_provisional_keepalive(struct sip_pvt *pvt)
+{
+ dialog_ref(pvt, "Stop provisional keepalive action");
+ if (ast_sched_add(sched, 0, __stop_provisional_keepalive, pvt) < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ dialog_unref(pvt, "Failed to schedule stop provisional keepalive action");
+ }
}
static void add_required_respheader(struct sip_request *req)
@@ -4631,7 +4683,7 @@
/* If we are sending a final response to an INVITE, stop retransmitting provisional responses */
if (p->initreq.method == SIP_INVITE && reliable == XMIT_CRITICAL) {
- AST_SCHED_DEL_UNREF(sched, p->provisional_keepalive_sched_id, dialog_unref(p, "when you delete the provisional_keepalive_sched_id, you should dec the refcount for the stored dialog ptr"));
+ stop_provisional_keepalive(p);
}
res = (reliable) ?
@@ -7039,7 +7091,8 @@
}
} else { /* Incoming call, not up */
const char *res;
- AST_SCHED_DEL_UNREF(sched, p->provisional_keepalive_sched_id, dialog_unref(p, "when you delete the provisional_keepalive_sched_id, you should dec the refcount for the stored dialog ptr"));
+
+ stop_provisional_keepalive(p);
if (p->hangupcause && (res = hangup_cause2sip(p->hangupcause)))
transmit_response_reliable(p, res, &p->initreq);
else
@@ -9099,7 +9152,7 @@
* \post pvt is locked
* \post pvt->owner is locked and its reference count is increased (if pvt->owner is not NULL)
*
- * \returns a pointer to the locked and reffed pvt->owner channel if it exists.
+ * \return a pointer to the locked and reffed pvt->owner channel if it exists.
*/
static struct ast_channel *sip_pvt_lock_full(struct sip_pvt *pvt)
{
--
To view, visit https://gerrit.asterisk.org/2393
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I98a694fd42bc81436c83aa92de03226e6e4e3f48
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
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