[svn-commits] rmudgett: branch 1.8 r322749 - in /branches/1.8: apps/ include/asterisk/ main/

SVN commits to the Digium repositories svn-commits at lists.digium.com
Thu Jun 9 11:31:58 CDT 2011


Author: rmudgett
Date: Thu Jun  9 11:31:53 2011
New Revision: 322749

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=322749
Log:
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/

Modified:
    branches/1.8/apps/app_directed_pickup.c
    branches/1.8/include/asterisk/features.h
    branches/1.8/main/features.c

Modified: branches/1.8/apps/app_directed_pickup.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/apps/app_directed_pickup.c?view=diff&rev=322749&r1=322748&r2=322749
==============================================================================
--- branches/1.8/apps/app_directed_pickup.c (original)
+++ branches/1.8/apps/app_directed_pickup.c Thu Jun  9 11:31:53 2011
@@ -97,19 +97,6 @@
 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 @@
 	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 @@
 
 	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 @@
 
 	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 @@
 	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 @@
 
 	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;
 	}

Modified: branches/1.8/include/asterisk/features.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/include/asterisk/features.h?view=diff&rev=322749&r1=322748&r2=322749
==============================================================================
--- branches/1.8/include/asterisk/features.h (original)
+++ branches/1.8/include/asterisk/features.h Thu Jun  9 11:31:53 2011
@@ -119,12 +119,28 @@
 /*! \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.
  */

Modified: branches/1.8/main/features.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/features.c?view=diff&rev=322749&r1=322748&r2=322749
==============================================================================
--- branches/1.8/main/features.c (original)
+++ branches/1.8/main/features.c Thu Jun  9 11:31:53 2011
@@ -569,7 +569,7 @@
  	.type = "dial-features",
  	.destroy = dial_features_destroy,
  	.duplicate = dial_features_duplicate,
- };
+};
  
 /* Forward declarations */
 static struct ast_parkinglot *parkinglot_addref(struct ast_parkinglot *parkinglot);
@@ -5691,17 +5691,42 @@
 	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;
 	}
@@ -5752,23 +5777,30 @@
 	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 };
-
-	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);
+	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;
+
+	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);
@@ -5776,37 +5808,48 @@
 	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);
-
-	/* Do the masquerade manually to make sure that is is completed. */
-	ast_channel_unlock(target);
+		"Channel: %s\r\n"
+		"TargetChannel: %s\r\n",
+		chan_name, target_name);
+
+	/* Do the masquerade manually to make sure that it is completed. */
 	ast_do_masquerade(target);
+	res = 0;
+
+pickup_failed:
 	ast_channel_lock(target);
-
-	return 0;
+	if (!ast_channel_datastore_remove(target, ds_pickup)) {
+		ast_datastore_free(ds_pickup);
+	}
+
+	return res;
 }
 
 static char *app_bridge = "Bridge";




More information about the svn-commits mailing list