[svn-commits] rmudgett: branch 1.8 r343577 - /branches/1.8/channels/chan_sip.c
SVN commits to the Digium repositories
svn-commits at lists.digium.com
Mon Nov 7 13:37:03 CST 2011
Author: rmudgett
Date: Mon Nov 7 13:36:56 2011
New Revision: 343577
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=343577
Log:
Fix deadlock if peer is destroyed while sending MWI notice.
A dialog cannot be destroyed by the ao2_callback dialog_needdestroy
because of a deadlock between the dialogs container lock and the RWLOCK of
the events subscription list.
* Create dialogs_to_destroy container to hold dialogs that will be
destroyed.
* Ensure that the event subscription callback will never happen with an
invalid peer pointer by making the event callback removal the first thing
in the peer destructor callback.
(closes issue ASTERISK-18747)
Reported by: Gregory Hinton Nietsky
Review: https://reviewboard.asterisk.org/r/1564/
Modified:
branches/1.8/channels/chan_sip.c
Modified: branches/1.8/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/channels/chan_sip.c?view=diff&rev=343577&r1=343576&r2=343577
==============================================================================
--- branches/1.8/channels/chan_sip.c (original)
+++ branches/1.8/channels/chan_sip.c Mon Nov 7 13:36:56 2011
@@ -1073,6 +1073,12 @@
}
}
+/*!
+ * \details
+ * This container holds the dialogs that will be destroyed immediately.
+ */
+struct ao2_container *dialogs_to_destroy;
+
/*! \brief
* Here we implement the container for dialogs (sip_pvt), defining
* generic wrapper functions to ease the transition from the current
@@ -2924,6 +2930,13 @@
}
}
+/*!
+ * \brief Unlink a dialog from the dialogs container, as well as any other places
+ * that it may be currently stored.
+ *
+ * \note A reference to the dialog must be held before calling this function, and this
+ * function does not release that reference.
+ */
void dialog_unlink_all(struct sip_pvt *dialog)
{
struct sip_pkt *cp;
@@ -4550,16 +4563,24 @@
static void sip_destroy_peer(struct sip_peer *peer)
{
ast_debug(3, "Destroying SIP peer %s\n", peer->name);
- if (peer->outboundproxy)
+
+ /*
+ * Remove any mailbox event subscriptions for this peer before
+ * we destroy anything. An event subscription callback may be
+ * happening right now.
+ */
+ clear_peer_mailboxes(peer);
+
+ if (peer->outboundproxy) {
ao2_ref(peer->outboundproxy, -1);
- peer->outboundproxy = NULL;
+ peer->outboundproxy = NULL;
+ }
/* Delete it, it needs to disappear */
if (peer->call) {
dialog_unlink_all(peer->call);
peer->call = dialog_unref(peer->call, "peer->call is being unset");
}
-
if (peer->mwipvt) { /* We have an active subscription, delete it */
dialog_unlink_all(peer->mwipvt);
@@ -4587,7 +4608,6 @@
}
if (peer->dnsmgr)
ast_dnsmgr_release(peer->dnsmgr);
- clear_peer_mailboxes(peer);
if (peer->socket.tcptls_session) {
ao2_ref(peer->socket.tcptls_session, -1);
@@ -16525,14 +16545,12 @@
* \brief Match dialogs that need to be destroyed
*
* \details This is used with ao2_callback to unlink/delete all dialogs that
- * are marked needdestroy. It will return CMP_MATCH for candidates, and they
- * will be unlinked.
+ * are marked needdestroy.
*
* \todo Re-work this to improve efficiency. Currently, this function is called
* on _every_ dialog after processing _every_ incoming SIP/UDP packet, or
* potentially even more often when the scheduler has entries to run.
*/
-
static int dialog_needdestroy(void *dialogobj, void *arg, int flags)
{
struct sip_pvt *dialog = dialogobj;
@@ -16578,15 +16596,34 @@
}
sip_pvt_unlock(dialog);
- /* no, the unlink should handle this: dialog_unref(dialog, "needdestroy: one more refcount decrement to allow dialog to be destroyed"); */
- /* the CMP_MATCH will unlink this dialog from the dialog hash table */
- dialog_unlink_all(dialog);
- return 0; /* the unlink_all should unlink this from the table, so.... no need to return a match */
+
+ /* This dialog needs to be destroyed. */
+ ao2_t_link(dialogs_to_destroy, dialog, "Link dialog for destruction");
+ return 0;
}
sip_pvt_unlock(dialog);
return 0;
+}
+
+/*!
+ * \internal
+ * \brief ao2_callback to unlink the specified dialog object.
+ *
+ * \param obj Ptr to dialog to unlink.
+ * \param arg Don't care.
+ * \param flags Don't care.
+ *
+ * \retval CMP_MATCH
+ */
+static int dialog_unlink_callback(void *obj, void *arg, int flags)
+{
+ struct sip_pvt *dialog = obj;
+
+ dialog_unlink_all(dialog);
+
+ return CMP_MATCH;
}
/*! \brief Remove temporary realtime objects from memory (CLI) */
@@ -25279,12 +25316,19 @@
of time since the last time we did it (when MWI is being sent, we can
get back to this point every millisecond or less)
*/
- ao2_t_callback(dialogs, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, dialog_needdestroy, &t,
- "callback to remove dialogs w/needdestroy");
-
- /* the old methodology would be to restart the search for dialogs to delete with every
- dialog that was found and destroyed, probably because the list contents would change,
- so we'd need to restart. This isn't the best thing to do with callbacks. */
+ /*
+ * We cannot hold the dialogs container lock when we destroy a
+ * dialog because of potential deadlocks. Instead we link the
+ * doomed dialog into dialogs_to_destroy and then iterate over
+ * that container destroying the dialogs.
+ */
+ ao2_t_callback(dialogs, OBJ_NODATA | OBJ_MULTIPLE, dialog_needdestroy, &t,
+ "callback to monitor dialog status");
+ if (ao2_container_count(dialogs_to_destroy)) {
+ /* Now destroy the found dialogs that need to be destroyed. */
+ ao2_t_callback(dialogs_to_destroy, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE,
+ dialog_unlink_callback, NULL, "callback to dialog_unlink_all");
+ }
/* XXX TODO The scheduler usage in this module does not have sufficient
* synchronization being done between running the scheduler and places
@@ -29670,8 +29714,9 @@
peers = ao2_t_container_alloc(HASH_PEER_SIZE, peer_hash_cb, peer_cmp_cb, "allocate peers");
peers_by_ip = ao2_t_container_alloc(HASH_PEER_SIZE, peer_iphash_cb, peer_ipcmp_cb, "allocate peers_by_ip");
dialogs = ao2_t_container_alloc(HASH_DIALOG_SIZE, dialog_hash_cb, dialog_cmp_cb, "allocate dialogs");
+ dialogs_to_destroy = ao2_t_container_alloc(1, NULL, NULL, "allocate dialogs_to_destroy");
threadt = ao2_t_container_alloc(HASH_DIALOG_SIZE, threadt_hash_cb, threadt_cmp_cb, "allocate threadt table");
- if (!peers || !peers_by_ip || !dialogs || !threadt) {
+ if (!peers || !peers_by_ip || !dialogs || !dialogs_to_destroy || !threadt) {
ast_log(LOG_ERROR, "Unable to create primary SIP container(s)\n");
return AST_MODULE_LOAD_FAILURE;
}
@@ -29929,6 +29974,7 @@
ao2_t_ref(peers, -1, "unref the peers table");
ao2_t_ref(peers_by_ip, -1, "unref the peers_by_ip table");
ao2_t_ref(dialogs, -1, "unref the dialogs table");
+ ao2_t_ref(dialogs_to_destroy, -1, "unref dialogs_to_destroy");
ao2_t_ref(threadt, -1, "unref the thread table");
ao2_t_ref(sip_monitor_instances, -1, "unref the sip_monitor_instances table");
More information about the svn-commits
mailing list