[Asterisk-code-review] Stasis: Fix unsafe use of stasis unsubscribe in modules. (asterisk[13])
Matt Jordan
asteriskteam at digium.com
Sun May 24 13:56:12 CDT 2015
Matt Jordan has submitted this change and it was merged.
Change subject: Stasis: Fix unsafe use of stasis_unsubscribe in modules.
......................................................................
Stasis: Fix unsafe use of stasis_unsubscribe in modules.
Many uses of stasis_unsubscribe in modules can be reached through unload.
These have been switched to stasis_unsubscribe_and_join.
Some subscription callbacks do nothing, for these I've created a noop
callback function in stasis.c. This is used by some modules that monitor
MWI topics in order to enable cache, since the callback does not become
invalid after dlclose it is safe to use stasis_unsubscribe on these, even
during module unload.
ASTERISK-25121 #close
Change-Id: Ifc2549fbd8eef7d703c222978e8f452e2972189c
---
M channels/chan_dahdi.c
M channels/chan_iax2.c
M channels/chan_mgcp.c
M channels/chan_sip.c
M channels/chan_skinny.c
M channels/sig_pri.c
M include/asterisk/stasis.h
M main/stasis.c
M res/res_hep_rtcp.c
M res/res_pjsip_mwi.c
M res/res_security_log.c
M res/res_stasis_device_state.c
M res/res_xmpp.c
13 files changed, 38 insertions(+), 35 deletions(-)
Approvals:
Matt Jordan: Looks good to me, approved; Verified
Joshua Colp: Looks good to me, but someone else must approve
diff --git a/channels/chan_dahdi.c b/channels/chan_dahdi.c
index 6958e5b..953561c 100644
--- a/channels/chan_dahdi.c
+++ b/channels/chan_dahdi.c
@@ -600,14 +600,6 @@
static int dahdi_sendtext(struct ast_channel *c, const char *text);
-static void mwi_event_cb(void *userdata, struct stasis_subscription *sub, struct stasis_message *msg)
-{
- /* This module does not handle MWI in an event-based manner. However, it
- * subscribes to MWI for each mailbox that is configured so that the core
- * knows that we care about it. Then, chan_dahdi will get the MWI from the
- * event cache instead of checking the mailbox directly. */
-}
-
/*! \brief Avoid the silly dahdi_getevent which ignores a bunch of events */
static inline int dahdi_get_event(int fd)
{
@@ -12594,7 +12586,11 @@
mailbox_specific_topic = ast_mwi_topic(tmp->mailbox);
if (mailbox_specific_topic) {
- tmp->mwi_event_sub = stasis_subscribe_pool(mailbox_specific_topic, mwi_event_cb, NULL);
+ /* This module does not handle MWI in an event-based manner. However, it
+ * subscribes to MWI for each mailbox that is configured so that the core
+ * knows that we care about it. Then, chan_dahdi will get the MWI from the
+ * event cache instead of checking the mailbox directly. */
+ tmp->mwi_event_sub = stasis_subscribe_pool(mailbox_specific_topic, stasis_subscription_cb_noop, NULL);
}
}
#ifdef HAVE_DAHDI_LINEREVERSE_VMWI
diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index a5f86e1..dc30d9f 100644
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -1438,13 +1438,6 @@
return is_allowed;
}
-static void mwi_event_cb(void *userdata, struct stasis_subscription *sub, struct stasis_message *msg)
-{
- /* The MWI subscriptions exist just so the core knows we care about those
- * mailboxes. However, we just grab the events out of the cache when it
- * is time to send MWI, since it is only sent with a REGACK. */
-}
-
static void network_change_stasis_subscribe(void)
{
if (!network_change_sub) {
@@ -13029,7 +13022,10 @@
mailbox_specific_topic = ast_mwi_topic(peer->mailbox);
if (mailbox_specific_topic) {
- peer->mwi_event_sub = stasis_subscribe_pool(mailbox_specific_topic, mwi_event_cb, NULL);
+ /* The MWI subscriptions exist just so the core knows we care about those
+ * mailboxes. However, we just grab the events out of the cache when it
+ * is time to send MWI, since it is only sent with a REGACK. */
+ peer->mwi_event_sub = stasis_subscribe_pool(mailbox_specific_topic, stasis_subscription_cb_noop, NULL);
}
}
diff --git a/channels/chan_mgcp.c b/channels/chan_mgcp.c
index 08c4dc2..b75cd43 100644
--- a/channels/chan_mgcp.c
+++ b/channels/chan_mgcp.c
@@ -489,14 +489,6 @@
.func_channel_read = acf_channel_read,
};
-static void mwi_event_cb(void *userdata, struct stasis_subscription *sub, struct stasis_message *msg)
-{
- /* This module does not handle MWI in an event-based manner. However, it
- * subscribes to MWI for each mailbox that is configured so that the core
- * knows that we care about it. Then, chan_mgcp will get the MWI from the
- * event cache instead of checking the mailbox directly. */
-}
-
static int has_voicemail(struct mgcp_endpoint *p)
{
int new_msgs;
@@ -4237,7 +4229,11 @@
mailbox_specific_topic = ast_mwi_topic(e->mailbox);
if (mailbox_specific_topic) {
- e->mwi_event_sub = stasis_subscribe_pool(mailbox_specific_topic, mwi_event_cb, NULL);
+ /* This module does not handle MWI in an event-based manner. However, it
+ * subscribes to MWI for each mailbox that is configured so that the core
+ * knows that we care about it. Then, chan_mgcp will get the MWI from the
+ * event cache instead of checking the mailbox directly. */
+ e->mwi_event_sub = stasis_subscribe_pool(mailbox_specific_topic, stasis_subscription_cb_noop, NULL);
}
}
snprintf(e->rqnt_ident, sizeof(e->rqnt_ident), "%08lx", (unsigned long)ast_random());
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 1307a93..b1fbd6e 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -5004,7 +5004,7 @@
static void destroy_mailbox(struct sip_mailbox *mailbox)
{
if (mailbox->event_sub) {
- mailbox->event_sub = stasis_unsubscribe(mailbox->event_sub);
+ mailbox->event_sub = stasis_unsubscribe_and_join(mailbox->event_sub);
}
ast_free(mailbox);
}
diff --git a/channels/chan_skinny.c b/channels/chan_skinny.c
index ed82c7d..be9f9b6 100644
--- a/channels/chan_skinny.c
+++ b/channels/chan_skinny.c
@@ -8749,7 +8749,7 @@
skinny_unlocksub(sub);
}
if (l->mwi_event_sub) {
- l->mwi_event_sub = stasis_unsubscribe(l->mwi_event_sub);
+ l->mwi_event_sub = stasis_unsubscribe_and_join(l->mwi_event_sub);
}
ast_mutex_unlock(&l->lock);
unregister_exten(l);
diff --git a/channels/sig_pri.c b/channels/sig_pri.c
index 2815dac..4b91391 100644
--- a/channels/sig_pri.c
+++ b/channels/sig_pri.c
@@ -8991,7 +8991,7 @@
#if defined(HAVE_PRI_MWI)
for (idx = 0; idx < ARRAY_LEN(pri->mbox); ++idx) {
if (pri->mbox[idx].sub) {
- pri->mbox[idx].sub = stasis_unsubscribe(pri->mbox[idx].sub);
+ pri->mbox[idx].sub = stasis_unsubscribe_and_join(pri->mbox[idx].sub);
}
}
#endif /* defined(HAVE_PRI_MWI) */
diff --git a/include/asterisk/stasis.h b/include/asterisk/stasis.h
index 0b1b1e8..16b30cc 100644
--- a/include/asterisk/stasis.h
+++ b/include/asterisk/stasis.h
@@ -521,6 +521,17 @@
typedef void (*stasis_subscription_cb)(void *data, struct stasis_subscription *sub, struct stasis_message *message);
/*!
+ * \brief Stasis subscription callback function that does nothing.
+ *
+ * \note This callback should be used for events are not directly processed, but need
+ * to be generated so data can be retrieved from cache later. Subscriptions with this
+ * callback can be released with \ref stasis_unsubscribe, even during module unload.
+ *
+ * \since 13.5
+ */
+void stasis_subscription_cb_noop(void *data, struct stasis_subscription *sub, struct stasis_message *message);
+
+/*!
* \brief Create a subscription.
*
* In addition to being AO2 managed memory (requiring an ao2_cleanup() to free
diff --git a/main/stasis.c b/main/stasis.c
index c2d552e..6adbfc3 100644
--- a/main/stasis.c
+++ b/main/stasis.c
@@ -444,6 +444,10 @@
static void send_subscription_subscribe(struct stasis_topic *topic, struct stasis_subscription *sub);
static void send_subscription_unsubscribe(struct stasis_topic *topic, struct stasis_subscription *sub);
+void stasis_subscription_cb_noop(void *data, struct stasis_subscription *sub, struct stasis_message *message)
+{
+}
+
struct stasis_subscription *internal_stasis_subscribe(
struct stasis_topic *topic,
stasis_subscription_cb callback,
diff --git a/res/res_hep_rtcp.c b/res/res_hep_rtcp.c
index fe39f19..787512b 100644
--- a/res/res_hep_rtcp.c
+++ b/res/res_hep_rtcp.c
@@ -131,7 +131,7 @@
static int unload_module(void)
{
if (stasis_rtp_subscription) {
- stasis_rtp_subscription = stasis_unsubscribe(stasis_rtp_subscription);
+ stasis_rtp_subscription = stasis_unsubscribe_and_join(stasis_rtp_subscription);
}
return 0;
diff --git a/res/res_pjsip_mwi.c b/res/res_pjsip_mwi.c
index 2ab7dfe..76e0e4c 100644
--- a/res/res_pjsip_mwi.c
+++ b/res/res_pjsip_mwi.c
@@ -471,7 +471,7 @@
struct mwi_stasis_subscription *mwi_stasis = obj;
if (mwi_stasis->stasis_sub) {
ast_debug(3, "Removing stasis subscription to mailbox %s\n", mwi_stasis->mailbox);
- mwi_stasis->stasis_sub = stasis_unsubscribe(mwi_stasis->stasis_sub);
+ mwi_stasis->stasis_sub = stasis_unsubscribe_and_join(mwi_stasis->stasis_sub);
}
return CMP_MATCH;
}
diff --git a/res/res_security_log.c b/res/res_security_log.c
index 78ca121..c3fb3cf 100644
--- a/res/res_security_log.c
+++ b/res/res_security_log.c
@@ -152,7 +152,7 @@
static int unload_module(void)
{
if (security_stasis_sub) {
- security_stasis_sub = stasis_unsubscribe(security_stasis_sub);
+ security_stasis_sub = stasis_unsubscribe_and_join(security_stasis_sub);
}
ast_logger_unregister_level(LOG_SECURITY_NAME);
diff --git a/res/res_stasis_device_state.c b/res/res_stasis_device_state.c
index 8a1c230..aec6a6e 100644
--- a/res/res_stasis_device_state.c
+++ b/res/res_stasis_device_state.c
@@ -105,7 +105,7 @@
static void device_state_subscription_destroy(void *obj)
{
struct device_state_subscription *sub = obj;
- sub->sub = stasis_unsubscribe(sub->sub);
+ sub->sub = stasis_unsubscribe_and_join(sub->sub);
ast_string_field_free_memory(sub);
}
diff --git a/res/res_xmpp.c b/res/res_xmpp.c
index 9d21297..d979143 100644
--- a/res/res_xmpp.c
+++ b/res/res_xmpp.c
@@ -3568,12 +3568,12 @@
}
if (client->mwi_sub) {
- client->mwi_sub = stasis_unsubscribe(client->mwi_sub);
+ client->mwi_sub = stasis_unsubscribe_and_join(client->mwi_sub);
xmpp_pubsub_unsubscribe(client, "message_waiting");
}
if (client->device_state_sub) {
- client->device_state_sub = stasis_unsubscribe(client->device_state_sub);
+ client->device_state_sub = stasis_unsubscribe_and_join(client->device_state_sub);
xmpp_pubsub_unsubscribe(client, "device_state");
}
--
To view, visit https://gerrit.asterisk.org/526
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifc2549fbd8eef7d703c222978e8f452e2972189c
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
More information about the asterisk-code-review
mailing list