[Asterisk-code-review] chan sip: Fix early call pickup caused deadlock. (asterisk[11])
Walter Doekes
asteriskteam at digium.com
Thu Jul 2 06:36:55 CDT 2015
Walter Doekes has uploaded a new change for review.
https://gerrit.asterisk.org/771
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)
ASTERISK-25213 #close
Change-Id: Ie1d15bec7d634035f48892e1ed6227411d7de2c1
---
M channels/chan_sip.c
1 file changed, 96 insertions(+), 34 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/71/771/1
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: newchange
Gerrit-Change-Id: Ie1d15bec7d634035f48892e1ed6227411d7de2c1
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Walter Doekes <walter+asterisk at wjd.nu>
More information about the asterisk-code-review
mailing list