[Asterisk-code-review] chan sip.c: Fix mwi resub deadlock potential. (asterisk[13])
Joshua Colp
asteriskteam at digium.com
Thu Mar 17 10:29:10 CDT 2016
Joshua Colp has submitted this change and it was merged.
Change subject: chan_sip.c: Fix mwi resub deadlock potential.
......................................................................
chan_sip.c: Fix mwi resub deadlock potential.
This patch is part of a series to resolve deadlocks in chan_sip.c.
Stopping a scheduled event can result in a deadlock if the scheduled event
is running when you try to stop the event. If you hold a lock needed by
the scheduled event while trying to stop the scheduled event then a
deadlock can happen. The general strategy for resolving the deadlock
potential is to push the actual starting and stopping of the scheduled
events off onto the scheduler/do_monitor() thread by scheduling an
immediate one shot scheduled event. Some restructuring may be needed
because the code may assume that the start/stop of the scheduled events is
immediate.
ASTERISK-25023 #close
Change-Id: I96d429c57a48861fd8bde63dd93db4e92dc3adb6
---
M channels/chan_sip.c
1 file changed, 92 insertions(+), 33 deletions(-)
Approvals:
Mark Michelson: Looks good to me, approved
Joshua Colp: Looks good to me, approved; Verified
George Joseph: Looks good to me, but someone else must approve
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 2cc365d..9e7872b 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1501,9 +1501,7 @@
/*!--- SIP MWI Subscription support */
static int sip_subscribe_mwi(const char *value, int lineno);
-static void sip_subscribe_mwi_destroy(void *data);
static void sip_send_all_mwi_subscriptions(void);
-static int sip_subscribe_mwi_do(const void *data);
static int __sip_subscribe_mwi_do(struct sip_subscription_mwi *mwi);
/* Scheduler id start/stop/reschedule functions. */
@@ -6511,12 +6509,12 @@
static void sip_subscribe_mwi_destroy(void *data)
{
struct sip_subscription_mwi *mwi = data;
+
if (mwi->call) {
mwi->call->mwi = NULL;
mwi->call = dialog_unref(mwi->call, "sip_subscription_mwi destruction");
}
- AST_SCHED_DEL(sched, mwi->resub);
ast_string_field_free_memory(mwi);
}
@@ -14645,20 +14643,94 @@
return send_request(p, &req, init ? XMIT_CRITICAL : XMIT_RELIABLE, p->ocseq);
}
-/*! \brief Send a subscription or resubscription for MWI */
+/*!
+ * \brief Send a subscription or resubscription for MWI
+ *
+ * \note Run by the sched thread.
+ */
static int sip_subscribe_mwi_do(const void *data)
{
- struct sip_subscription_mwi *mwi = (struct sip_subscription_mwi*)data;
-
- if (!mwi) {
- return -1;
- }
+ struct sip_subscription_mwi *mwi = (struct sip_subscription_mwi *) data;
mwi->resub = -1;
__sip_subscribe_mwi_do(mwi);
- ao2_t_ref(mwi, -1, "unref mwi to balance ast_sched_add");
+ ao2_t_ref(mwi, -1, "Scheduled mwi resub complete");
return 0;
+}
+
+/* Run by the sched thread. */
+static int __shutdown_mwi_subscription(const void *data)
+{
+ struct sip_subscription_mwi *mwi = (void *) data;
+
+ AST_SCHED_DEL_UNREF(sched, mwi->resub,
+ ao2_t_ref(mwi, -1, "Stop scheduled mwi resub"));
+
+ if (mwi->dnsmgr) {
+ ast_dnsmgr_release(mwi->dnsmgr);
+ mwi->dnsmgr = NULL;
+ ao2_t_ref(mwi, -1, "dnsmgr release");
+ }
+
+ ao2_t_ref(mwi, -1, "Shutdown MWI subscription action");
+ return 0;
+}
+
+static void shutdown_mwi_subscription(struct sip_subscription_mwi *mwi)
+{
+ ao2_t_ref(mwi, +1, "Shutdown MWI subscription action");
+ if (ast_sched_add(sched, 0, __shutdown_mwi_subscription, mwi) < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ ao2_t_ref(mwi, -1, "Failed to schedule shutdown MWI subscription action");
+ }
+}
+
+struct mwi_subscription_data {
+ struct sip_subscription_mwi *mwi;
+ int ms;
+};
+
+/* Run by the sched thread. */
+static int __start_mwi_subscription(const void *data)
+{
+ struct mwi_subscription_data *sched_data = (void *) data;
+ struct sip_subscription_mwi *mwi = sched_data->mwi;
+ int ms = sched_data->ms;
+
+ ast_free(sched_data);
+
+ AST_SCHED_DEL_UNREF(sched, mwi->resub,
+ ao2_t_ref(mwi, -1, "Stop scheduled mwi resub"));
+
+ ao2_t_ref(mwi, +1, "Schedule mwi resub");
+ mwi->resub = ast_sched_add(sched, ms, sip_subscribe_mwi_do, mwi);
+ if (mwi->resub < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ ao2_t_ref(mwi, -1, "Failed to schedule mwi resub");
+ }
+
+ ao2_t_ref(mwi, -1, "Start MWI subscription action");
+ return 0;
+}
+
+static void start_mwi_subscription(struct sip_subscription_mwi *mwi, int ms)
+{
+ struct mwi_subscription_data *sched_data;
+
+ sched_data = ast_malloc(sizeof(*sched_data));
+ if (!sched_data) {
+ /* Uh Oh. Expect bad behavior. */
+ return;
+ }
+ sched_data->mwi = mwi;
+ sched_data->ms = ms;
+ ao2_t_ref(mwi, +1, "Start MWI subscription action");
+ if (ast_sched_add(sched, 0, __start_mwi_subscription, sched_data) < 0) {
+ /* Uh Oh. Expect bad behavior. */
+ ao2_t_ref(mwi, -1, "Failed to schedule start MWI subscription action");
+ ast_free(sched_data);
+ }
}
static void on_dns_update_registry(struct ast_sockaddr *old, struct ast_sockaddr *new, void *data)
@@ -17159,6 +17231,7 @@
acl_change_sub = stasis_unsubscribe_and_join(acl_change_sub);
}
+/* Run by the sched thread. */
static int network_change_sched_cb(const void *data)
{
network_change_sched_id = -1;
@@ -23986,9 +24059,7 @@
p->options = NULL;
}
p->mwi->subscribed = 1;
- if ((p->mwi->resub = ast_sched_add(sched, mwi_expiry * 1000, sip_subscribe_mwi_do, ao2_t_bump(p->mwi, "mwi ast_sched_add"))) < 0) {
- ao2_t_ref(p->mwi, -1, "mwi ast_sched_add < 0");
- }
+ start_mwi_subscription(p->mwi, mwi_expiry * 1000);
break;
case 401:
case 407:
@@ -33560,18 +33631,12 @@
static void sip_send_all_mwi_subscriptions(void)
{
struct ao2_iterator iter;
- struct sip_subscription_mwi *iterator;
+ struct sip_subscription_mwi *mwi;
iter = ao2_iterator_init(subscription_mwi_list, 0);
- while ((iterator = ao2_t_iterator_next(&iter, "sip_send_all_mwi_subscriptions iter"))) {
- ao2_lock(iterator);
- AST_SCHED_DEL(sched, iterator->resub);
- ao2_t_ref(iterator, +1, "mwi added to schedule");
- if ((iterator->resub = ast_sched_add(sched, 1, sip_subscribe_mwi_do, iterator)) < 0) {
- ao2_t_ref(iterator, -1, "mwi failed to schedule");
- }
- ao2_unlock(iterator);
- ao2_t_ref(iterator, -1, "sip_send_all_mwi_subscriptions iter");
+ while ((mwi = ao2_t_iterator_next(&iter, "sip_send_all_mwi_subscriptions iter"))) {
+ start_mwi_subscription(mwi, 1);
+ ao2_t_ref(mwi, -1, "sip_send_all_mwi_subscriptions iter");
}
ao2_iterator_destroy(&iter);
}
@@ -35233,18 +35298,12 @@
{
struct ao2_iterator iter;
- struct sip_subscription_mwi *iterator;
+ struct sip_subscription_mwi *mwi;
iter = ao2_iterator_init(subscription_mwi_list, 0);
- while ((iterator = ao2_t_iterator_next(&iter, "unload_module iter"))) {
- ao2_lock(iterator);
- if (iterator->dnsmgr) {
- ast_dnsmgr_release(iterator->dnsmgr);
- iterator->dnsmgr = NULL;
- ao2_t_ref(iterator, -1, "dnsmgr release");
- }
- ao2_unlock(iterator);
- ao2_t_ref(iterator, -1, "unload_module iter");
+ while ((mwi = ao2_t_iterator_next(&iter, "unload_module iter"))) {
+ shutdown_mwi_subscription(mwi);
+ ao2_t_ref(mwi, -1, "unload_module iter");
}
ao2_iterator_destroy(&iter);
}
--
To view, visit https://gerrit.asterisk.org/2401
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I96d429c57a48861fd8bde63dd93db4e92dc3adb6
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-code-review
mailing list