From 0a8f9d2cf0872be7ffc623066c4220de3bdfb97c Mon Sep 17 00:00:00 2001
From: Richard Mudgett <rmudgett@digium.com>
Date: Thu, 9 Jun 2011 16:47:07 +0000
Subject: [PATCH] Merged revisions 322749 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8
........
r322749 | rmudgett | 2011-06-09 11:31:53 -0500 (Thu, 09 Jun 2011) | 15 lines
Remove potential deadlock in call pickup race.
Deadlock is possible in ast_do_pickup() when holding the target channel
lock and trying to get the chan channel lock. Also, holding the target
lock when calling ast_channel_masquerade() is not a good idea because that
routine does deadlock avoidance.
* Removed the need to hold the target lock after marking the target with a
datastore and getting the connected line data off of the target channel.
* Moved can_pickup() to ast_can_pickup() in features.c. Now all the call
pickup methods use the same basic call pickup availability check.
Review: https://reviewboard.asterisk.org/r/1234/
........
git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@322750 65c4cc65-6c06-0410-ace0-fbb531ad65f3
---
apps/app_directed_pickup.c | 24 +++-------
include/asterisk/features.h | 20 +++++++-
main/features.c | 93 +++++++++++++++++++++++++++----------
3 files changed, 92 insertions(+), 45 deletions(-)
diff --git a/apps/app_directed_pickup.c b/apps/app_directed_pickup.c
index 38f24effc3..63a7621ff8 100644
--- a/apps/app_directed_pickup.c
+++ b/apps/app_directed_pickup.c
@@ -97,19 +97,6 @@ static const char app[] = "Pickup";
static const char app2[] = "PickupChan";
/*! \todo This application should return a result code, like PICKUPRESULT */
-/* Helper function that determines whether a channel is capable of being picked up */
-static int can_pickup(struct ast_channel *chan)
-{
- if (!chan->pbx && !chan->masq &&
- !ast_test_flag(chan, AST_FLAG_ZOMBIE) &&
- (chan->_state == AST_STATE_RINGING ||
- chan->_state == AST_STATE_RING ||
- chan->_state == AST_STATE_DOWN)) {
- return 1;
- }
- return 0;
-}
-
struct pickup_by_name_args {
const char *name;
size_t len;
@@ -121,7 +108,7 @@ static int pickup_by_name_cb(void *obj, void *arg, void *data, int flags)
struct pickup_by_name_args *args = data;
ast_channel_lock(target);
- if (!strncasecmp(target->name, args->name, args->len) && can_pickup(target)) {
+ if (!strncasecmp(target->name, args->name, args->len) && ast_can_pickup(target)) {
/* Return with the channel still locked on purpose */
return CMP_MATCH | CMP_STOP;
}
@@ -190,7 +177,7 @@ static int pickup_by_exten(struct ast_channel *chan, const char *exten, const ch
while ((target = ast_channel_iterator_next(iter))) {
ast_channel_lock(target);
- if ((chan != target) && can_pickup(target)) {
+ if ((chan != target) && ast_can_pickup(target)) {
ast_log(LOG_NOTICE, "%s pickup by %s\n", target->name, chan->name);
break;
}
@@ -217,7 +204,7 @@ static int find_by_mark(void *obj, void *arg, void *data, int flags)
ast_channel_lock(target);
tmp = pbx_builtin_getvar_helper(target, PICKUPMARK);
- if (tmp && !strcasecmp(tmp, mark) && can_pickup(target)) {
+ if (tmp && !strcasecmp(tmp, mark) && ast_can_pickup(target)) {
/* Return with the channel still locked on purpose */
return CMP_MATCH | CMP_STOP;
}
@@ -249,7 +236,8 @@ static int find_channel_by_group(void *obj, void *arg, void *data, int flags)
struct ast_channel *chan = data;/*!< Channel wanting to pickup call */
ast_channel_lock(target);
- if (chan != target && (chan->pickupgroup & target->callgroup) && can_pickup(target)) {
+ if (chan != target && (chan->pickupgroup & target->callgroup)
+ && ast_can_pickup(target)) {
/* Return with the channel still locked on purpose */
return CMP_MATCH | CMP_STOP;
}
@@ -316,7 +304,7 @@ static int find_by_part(void *obj, void *arg, void *data, int flags)
ast_channel_lock(target);
if (len <= strlen(target->name) && !strncmp(target->name, part, len)
- && can_pickup(target)) {
+ && ast_can_pickup(target)) {
/* Return with the channel still locked on purpose */
return CMP_MATCH | CMP_STOP;
}
diff --git a/include/asterisk/features.h b/include/asterisk/features.h
index 4ea9413836..7e749f2e0f 100644
--- a/include/asterisk/features.h
+++ b/include/asterisk/features.h
@@ -119,12 +119,28 @@ const char *ast_pickup_ext(void);
/*! \brief Bridge a call, optionally allowing redirection */
int ast_bridge_call(struct ast_channel *chan, struct ast_channel *peer,struct ast_bridge_config *config);
+/*!
+ * \brief Test if a channel can be picked up.
+ *
+ * \param chan Channel to test if can be picked up.
+ *
+ * \note This function assumes that chan is locked.
+ *
+ * \return TRUE if channel can be picked up.
+ */
+int ast_can_pickup(struct ast_channel *chan);
+
/*! \brief Pickup a call */
int ast_pickup_call(struct ast_channel *chan);
/*!
- * \brief Pickup a call target
- * \note This function assumes that target is locked
+ * \brief Pickup a call target.
+ *
+ * \param chan channel that initiated pickup.
+ * \param target channel to be picked up.
+ *
+ * \note This function assumes that target is locked.
+ *
* \retval 0 on success.
* \retval -1 on failure.
*/
diff --git a/main/features.c b/main/features.c
index 81421f17b8..34c88473b0 100644
--- a/main/features.c
+++ b/main/features.c
@@ -577,7 +577,7 @@ static const struct ast_datastore_info dial_features_info = {
.type = "dial-features",
.destroy = dial_features_destroy,
.duplicate = dial_features_duplicate,
- };
+};
/* Forward declarations */
static struct ast_parkinglot *parkinglot_addref(struct ast_parkinglot *parkinglot);
@@ -5747,17 +5747,42 @@ static int manager_park(struct mansession *s, const struct message *m)
return 0;
}
+/*!
+ * The presence of this datastore on the channel indicates that
+ * someone is attemting to pickup or has picked up the channel.
+ * The purpose is to prevent a race between two channels
+ * attempting to pickup the same channel.
+ */
+static const struct ast_datastore_info pickup_active = {
+ .type = "pickup-active",
+};
+
+int ast_can_pickup(struct ast_channel *chan)
+{
+ if (!chan->pbx && !chan->masq && !ast_test_flag(chan, AST_FLAG_ZOMBIE)
+ && (chan->_state == AST_STATE_RINGING
+ || chan->_state == AST_STATE_RING
+ /*
+ * Check the down state as well because some SIP devices do not
+ * give 180 ringing when they can just give 183 session progress
+ * instead. Issue 14005. (Some ISDN switches as well for that
+ * matter.)
+ */
+ || chan->_state == AST_STATE_DOWN)
+ && !ast_channel_datastore_find(chan, &pickup_active, NULL)) {
+ return 1;
+ }
+ return 0;
+}
+
static int find_channel_by_group(void *obj, void *arg, void *data, int flags)
{
struct ast_channel *target = obj;/*!< Potential pickup target */
struct ast_channel *chan = data;/*!< Channel wanting to pickup call */
ast_channel_lock(target);
- if (chan != target && (chan->pickupgroup & target->callgroup) &&
- !target->pbx &&
- ((target->_state == AST_STATE_RINGING) || (target->_state == AST_STATE_RING)) &&
- !target->masq &&
- !ast_test_flag(target, AST_FLAG_ZOMBIE)) {
+ if (chan != target && (chan->pickupgroup & target->callgroup)
+ && ast_can_pickup(target)) {
/* Return with the channel still locked on purpose */
return CMP_MATCH | CMP_STOP;
}
@@ -5808,23 +5833,30 @@ int ast_pickup_call(struct ast_channel *chan)
return res;
}
-/*!
- * \brief Pickup a call target, Common Code.
- * \param chan channel that initiated pickup.
- * \param target channel.
- *
- * Answer calling channel, flag channel as answered on queue, masq channels together.
- */
int ast_do_pickup(struct ast_channel *chan, struct ast_channel *target)
{
struct ast_party_connected_line connected_caller;
struct ast_channel *chans[2] = { chan, target };
+ struct ast_datastore *ds_pickup;
+ const char *chan_name;/*!< A masquerade changes channel names. */
+ const char *target_name;/*!< A masquerade changes channel names. */
+ int res = -1;
- ast_debug(1, "Call pickup on '%s' by '%s'\n", target->name, chan->name);
- ast_cel_report_event(target, AST_CEL_PICKUP, NULL, NULL, chan);
+ target_name = ast_strdupa(target->name);
+ ast_debug(1, "Call pickup on '%s' by '%s'\n", target_name, chan->name);
+
+ /* Mark the target to block any call pickup race. */
+ ds_pickup = ast_datastore_alloc(&pickup_active, NULL);
+ if (!ds_pickup) {
+ ast_log(LOG_WARNING,
+ "Unable to create channel datastore on '%s' for call pickup\n", target_name);
+ return -1;
+ }
+ ast_channel_datastore_add(target, ds_pickup);
ast_party_connected_line_init(&connected_caller);
ast_party_connected_line_copy(&connected_caller, &target->connected);
+ ast_channel_unlock(target);/* The pickup race is avoided so we do not need the lock anymore. */
connected_caller.source = AST_CONNECTED_LINE_UPDATE_SOURCE_ANSWER;
if (ast_channel_connected_line_macro(NULL, chan, &connected_caller, 0, 0)) {
ast_channel_update_connected_line(chan, &connected_caller, NULL);
@@ -5832,37 +5864,48 @@ int ast_do_pickup(struct ast_channel *chan, struct ast_channel *target)
ast_party_connected_line_free(&connected_caller);
ast_channel_lock(chan);
+ chan_name = ast_strdupa(chan->name);
ast_connected_line_copy_from_caller(&connected_caller, &chan->caller);
ast_channel_unlock(chan);
connected_caller.source = AST_CONNECTED_LINE_UPDATE_SOURCE_ANSWER;
ast_channel_queue_connected_line_update(chan, &connected_caller, NULL);
ast_party_connected_line_free(&connected_caller);
+ ast_cel_report_event(target, AST_CEL_PICKUP, NULL, NULL, chan);
+
if (ast_answer(chan)) {
- ast_log(LOG_WARNING, "Unable to answer '%s'\n", chan->name);
- return -1;
+ ast_log(LOG_WARNING, "Unable to answer '%s'\n", chan_name);
+ goto pickup_failed;
}
if (ast_queue_control(chan, AST_CONTROL_ANSWER)) {
- ast_log(LOG_WARNING, "Unable to queue answer on '%s'\n", chan->name);
- return -1;
+ ast_log(LOG_WARNING, "Unable to queue answer on '%s'\n", chan_name);
+ goto pickup_failed;
}
if (ast_channel_masquerade(target, chan)) {
- ast_log(LOG_WARNING, "Unable to masquerade '%s' into '%s'\n", chan->name, target->name);
- return -1;
+ ast_log(LOG_WARNING, "Unable to masquerade '%s' into '%s'\n", chan_name,
+ target_name);
+ goto pickup_failed;
}
/* If you want UniqueIDs, set channelvars in manager.conf to CHANNEL(uniqueid) */
ast_manager_event_multichan(EVENT_FLAG_CALL, "Pickup", 2, chans,
- "Channel: %s\r\nTargetChannel: %s\r\n", chan->name, target->name);
+ "Channel: %s\r\n"
+ "TargetChannel: %s\r\n",
+ chan_name, target_name);
- /* Do the masquerade manually to make sure that is is completed. */
- ast_channel_unlock(target);
+ /* Do the masquerade manually to make sure that it is completed. */
ast_do_masquerade(target);
+ res = 0;
+
+pickup_failed:
ast_channel_lock(target);
+ if (!ast_channel_datastore_remove(target, ds_pickup)) {
+ ast_datastore_free(ds_pickup);
+ }
- return 0;
+ return res;
}
static char *app_bridge = "Bridge";
--
GitLab