[asterisk-commits] chan sip: Fix early call pickup caused deadlock. (asterisk[11])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Sat Jul 4 19:09:19 CDT 2015
Joshua Colp has submitted this change and it was merged.
Change subject: chan_sip: Fix early call pickup caused deadlock.
......................................................................
chan_sip: Fix early call pickup caused deadlock.
If non-magic pickup (no "pickup-" in callid) is used, chan_sip locks the
channel it wants to pick up, and a bit further down, it locks the
channel list when allocating a new channel.
That causes a deadlock when another part of the code traverses over the
channel list, locking all the channels one by one.
This changeset fixes it by releasing the locks before calling sip_new
and reacquiring them afterwards. Unfortunately this involves doing the
checks we already did again (because the channel may have changed).
While trying to avoid duplicate code, I did some refactoring for
readability:
- if refer_locked == 1, we guarantee there is a locked channel
- magic_callid holds a cached version of !ast_strlen_zero(pickup.exten)
This is for branch 11 only. It appears that the changed code in 13 does
not lock the components like it does in 11 and below. Reproducing the
deadlock on 13 has thusfar failed.
ASTERISK-25213 #close
Change-Id: Ie1d15bec7d634035f48892e1ed6227411d7de2c1
---
M channels/chan_sip.c
1 file changed, 96 insertions(+), 34 deletions(-)
Approvals:
Anonymous Coward #1000019: Verified
Matt Jordan: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index de23063..f4867ae 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -25121,6 +25121,39 @@
return 0;
}
+/*! \brief Checks state of the p->refer->refer_call state: can it be picked up?
+ *
+ * It will terminate the call if it cannot be picked up; e.g. because it
+ * was gone, or because it wasn't in a ringing state.
+ *
+ * \return 1 if terminated, 0 if ok
+ */
+static int terminate_on_invalid_replaces_state(struct sip_pvt *p, struct sip_request *req, const char *replace_id)
+{
+ if (p->refer->refer_call == p) {
+ ast_log(LOG_NOTICE, "INVITE with replaces into its own call id (%s == %s)!\n", replace_id, p->callid);
+ transmit_response_reliable(p, "400 Bad request", req); /* The best way to not not accept the transfer */
+
+ } else if (!p->refer->refer_call->owner) {
+ /* Oops, someting wrong anyway, no owner, no call */
+ ast_log(LOG_NOTICE, "Supervised transfer attempted to replace non-existing call id (%s)!\n", replace_id);
+ /* Check for better return code */
+ transmit_response_reliable(p, "481 Call Leg Does Not Exist (Replace)", req);
+
+ } else if (ast_channel_state(p->refer->refer_call->owner) != AST_STATE_RINGING &&
+ ast_channel_state(p->refer->refer_call->owner) != AST_STATE_RING &&
+ ast_channel_state(p->refer->refer_call->owner) != AST_STATE_UP) {
+ ast_log(LOG_NOTICE, "Supervised transfer attempted to replace non-ringing or active call id (%s)!\n", replace_id);
+ transmit_response_reliable(p, "603 Declined (Replaces)", req);
+
+ } else {
+ /* Ok */
+ return 0;
+ }
+ /* Terminated */
+ return 1;
+}
+
/*!
* \brief bare-bones support for SIP UPDATE
*
@@ -25322,6 +25355,7 @@
int gotdest;
const char *p_replaces;
char *replace_id = NULL;
+ int magic_call_id = 0;
int refer_locked = 0;
const char *required;
unsigned int required_profile = 0;
@@ -25553,40 +25587,27 @@
ast_channel_unlock(subscription->owner);
}
subscription = dialog_unref(subscription, "unref dialog subscription");
+ magic_call_id = !ast_strlen_zero(pickup.exten);
}
}
- /* This locks both refer_call pvt and refer_call pvt's owner!!!*/
- if (!error && ast_strlen_zero(pickup.exten) && (p->refer->refer_call = get_sip_pvt_byid_locked(replace_id, totag, fromtag)) == NULL) {
- ast_log(LOG_NOTICE, "Supervised transfer attempted to replace non-existent call id (%s)!\n", replace_id);
- transmit_response_reliable(p, "481 Call Leg Does Not Exist (Replaces)", req);
- error = 1;
- } else {
- refer_locked = 1;
- }
-
- /* The matched call is the call from the transferer to Asterisk .
- We want to bridge the bridged part of the call to the
- incoming invite, thus taking over the refered call */
-
- if (p->refer->refer_call == p) {
- ast_log(LOG_NOTICE, "INVITE with replaces into it's own call id (%s == %s)!\n", replace_id, p->callid);
- transmit_response_reliable(p, "400 Bad request", req); /* The best way to not not accept the transfer */
- error = 1;
- }
-
- if (!error && ast_strlen_zero(pickup.exten) && !p->refer->refer_call->owner) {
- /* Oops, someting wrong anyway, no owner, no call */
- ast_log(LOG_NOTICE, "Supervised transfer attempted to replace non-existing call id (%s)!\n", replace_id);
- /* Check for better return code */
- transmit_response_reliable(p, "481 Call Leg Does Not Exist (Replace)", req);
- error = 1;
- }
-
- if (!error && ast_strlen_zero(pickup.exten) && ast_channel_state(p->refer->refer_call->owner) != AST_STATE_RINGING && ast_channel_state(p->refer->refer_call->owner) != AST_STATE_RING && ast_channel_state(p->refer->refer_call->owner) != AST_STATE_UP) {
- ast_log(LOG_NOTICE, "Supervised transfer attempted to replace non-ringing or active call id (%s)!\n", replace_id);
- transmit_response_reliable(p, "603 Declined (Replaces)", req);
- error = 1;
+ if (!error) {
+ if (magic_call_id) {
+ ;
+ /* This locks both refer_call pvt and refer_call pvt's owner!!!*/
+ } else if ((p->refer->refer_call = get_sip_pvt_byid_locked(replace_id, totag, fromtag))) {
+ /* The matched call is the call from the transferer to Asterisk .
+ We want to bridge the bridged part of the call to the
+ incoming invite, thus taking over the refered call */
+ refer_locked = 1;
+ if (terminate_on_invalid_replaces_state(p, req, replace_id)) {
+ error = 1;
+ }
+ } else {
+ ast_log(LOG_NOTICE, "Supervised transfer attempted to replace non-existent call id (%s)!\n", replace_id);
+ transmit_response_reliable(p, "481 Call Leg Does Not Exist (Replaces)", req);
+ error = 1;
+ }
}
if (error) { /* Give up this dialog */
@@ -25828,10 +25849,51 @@
goto request_invite_cleanup;
}
+ /* We cannot call sip_new with any channel locks
+ * held. Unlock the referred channel if locked. */
+ if (refer_locked) {
+ sip_pvt_unlock(p->refer->refer_call);
+ if (p->refer->refer_call->owner) {
+ ast_channel_unlock(p->refer->refer_call->owner);
+ }
+ }
+
/* First invitation - create the channel. Allocation
* failures are handled below. */
-
c = sip_new(p, AST_STATE_DOWN, S_OR(p->peername, NULL), NULL, p->logger_callid);
+
+ /* Reacquire the lock on the referred call. */
+ if (refer_locked) {
+ /* Reuse deadlock avoid pattern found in
+ * get_sip_pvt_byid_locked. */
+ sip_pvt_lock(p->refer->refer_call);
+ while (p->refer->refer_call->owner && ast_channel_trylock(p->refer->refer_call->owner)) {
+ sip_pvt_unlock(p->refer->refer_call);
+ usleep(1);
+ sip_pvt_lock(p->refer->refer_call);
+ }
+
+ /* And now, check once more that the call is still
+ * still available. Yuck. Bail out if it isn't. */
+ if (terminate_on_invalid_replaces_state(p, req, replace_id)) {
+ /* Free the referred call: it's not ours anymore. */
+ sip_pvt_unlock(p->refer->refer_call);
+ if (p->refer->refer_call->owner) {
+ ast_channel_unlock(p->refer->refer_call->owner);
+ }
+ p->refer->refer_call = dialog_unref(p->refer->refer_call, "unref dialog p->refer->refer_call");
+ refer_locked = 0;
+ /* Kill the channel we just created. */
+ sip_pvt_unlock(p);
+ ast_hangup(c);
+ sip_pvt_lock(p); /* pvt is expected to remain locked on return, so re-lock it */
+ /* Mark the call as failed. */
+ sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
+ p->invitestate = INV_COMPLETED;
+ res = INV_REQ_ERROR;
+ goto request_invite_cleanup;
+ }
+ }
if (cc_recall_core_id != -1) {
ast_setup_cc_recall_datastore(c, cc_recall_core_id);
@@ -25891,7 +25953,7 @@
p->lastinvite = seqno;
if (c && replace_id) { /* Attended transfer or call pickup - we're the target */
- if (!ast_strlen_zero(pickup.exten)) {
+ if (magic_call_id) {
append_history(p, "Xfer", "INVITE/Replace received");
/* Let the caller know we're giving it a shot */
@@ -26062,7 +26124,7 @@
request_invite_cleanup:
- if (refer_locked && p->refer && p->refer->refer_call) {
+ if (refer_locked) {
sip_pvt_unlock(p->refer->refer_call);
if (p->refer->refer_call->owner) {
ast_channel_unlock(p->refer->refer_call->owner);
--
To view, visit https://gerrit.asterisk.org/771
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie1d15bec7d634035f48892e1ed6227411d7de2c1
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
More information about the asterisk-commits
mailing list