[Asterisk-code-review] res pjsip pubsub: Remove serializer when sending final NOTIFY. (asterisk[13])
Matt Jordan
asteriskteam at digium.com
Sun Oct 25 10:12:37 CDT 2015
Matt Jordan has submitted this change and it was merged.
Change subject: res_pjsip_pubsub: Remove serializer when sending final NOTIFY.
......................................................................
res_pjsip_pubsub: Remove serializer when sending final NOTIFY.
There have been crashes seen where a taskprocessor's listener is NULL
unexpectedly.
Looking at backtraces, the problem was specifically seen in PJSIP
serializers.
Subscriptions make the mistake of removing a serializer from a dialog
during subscription tree destruction. Since subscription trees are
reference-counted, guaranteeing the circumstances behind the destruction
are not possible. This makes it so that the dialog serializer can be
removed while not holding the dialog lock. This makes it possible for
the distributor to get a pointer to the dialog serializer and have that
serializer get freed out from under it.
The fix for this is to remove the serializer from a subscription dialog
when sending the final NOTIFY. This guarantees that the serializer is
removed with the dialog lock held. By doing this, we guarantee that if
the distributor gains access to the dialog's serializer, it will not be
possible for the serializer to get freed by another thread.
Change-Id: I21f5dac33529f65cec45679bdace60670800ff66
---
M res/res_pjsip_pubsub.c
1 file changed, 40 insertions(+), 31 deletions(-)
Approvals:
Anonymous Coward #1000019: Verified
Matt Jordan: Looks good to me, approved
Joshua Colp: Looks good to me, but someone else must approve
diff --git a/res/res_pjsip_pubsub.c b/res/res_pjsip_pubsub.c
index 75b286b..bb2f243 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -1022,26 +1022,6 @@
return strcmp(datastore1->uid, uid2) ? 0 : CMP_MATCH | CMP_STOP;
}
-static int subscription_remove_serializer(void *obj)
-{
- struct sip_subscription_tree *sub_tree = obj;
-
- /* This is why we keep the dialog on the subscription. When the subscription
- * is destroyed, there is no guarantee that the underlying dialog is ready
- * to be destroyed. Furthermore, there's no guarantee in the opposite direction
- * either. The dialog could be destroyed before our subscription is. We fix
- * this problem by keeping a reference to the dialog until it is time to
- * destroy the subscription. We need to have the dialog available when the
- * subscription is destroyed so that we can guarantee that our attempt to
- * remove the serializer will be successful.
- */
- ast_sip_dialog_set_serializer(sub_tree->dlg, NULL);
- ast_sip_dialog_set_endpoint(sub_tree->dlg, NULL);
- pjsip_dlg_dec_session(sub_tree->dlg, &pubsub_module);
-
- return 0;
-}
-
static void add_subscription(struct sip_subscription_tree *obj)
{
SCOPED_LOCK(lock, &subscriptions, AST_RWLIST_WRLOCK, AST_RWLIST_UNLOCK);
@@ -1208,9 +1188,6 @@
subscription_persistence_remove(sub_tree);
ao2_cleanup(sub_tree->endpoint);
- if (sub_tree->dlg) {
- ast_sip_push_task_synchronous(NULL, subscription_remove_serializer, sub_tree);
- }
destroy_subscriptions(sub_tree->root);
ast_taskprocessor_unreference(sub_tree->serializer);
@@ -1225,10 +1202,6 @@
static void subscription_setup_dialog(struct sip_subscription_tree *sub_tree, pjsip_dialog *dlg)
{
- /* We keep a reference to the dialog until our subscription is destroyed. See
- * the subscription_destructor for more details
- */
- pjsip_dlg_inc_session(dlg, &pubsub_module);
sub_tree->dlg = dlg;
ast_sip_dialog_set_serializer(dlg, sub_tree->serializer);
ast_sip_dialog_set_endpoint(dlg, sub_tree->endpoint);
@@ -1523,7 +1496,11 @@
ast_str_append(buf, 0, "Endpoint: %s\r\n",
ast_sorcery_object_get_id(sub_tree->endpoint));
- ast_copy_pj_str(str, &sub_tree->dlg->call_id->id, sizeof(str));
+ if (sub_tree->dlg) {
+ ast_copy_pj_str(str, &sub_tree->dlg->call_id->id, sizeof(str));
+ } else {
+ ast_copy_string(str, "<unknown>", sizeof(str));
+ }
ast_str_append(buf, 0, "Callid: %s\r\n", str);
ast_str_append(buf, 0, "State: %s\r\n", pjsip_evsub_get_state_name(sub_tree->evsub));
@@ -1544,10 +1521,16 @@
void *ast_sip_subscription_get_header(const struct ast_sip_subscription *sub, const char *header)
{
- pjsip_dialog *dlg = sub->tree->dlg;
- pjsip_msg *msg = ast_sip_mod_data_get(dlg->mod_data, pubsub_module.id, MOD_DATA_MSG);
+ pjsip_dialog *dlg;
+ pjsip_msg *msg;
pj_str_t name;
+ dlg = sub->tree->dlg;
+ if (!dlg) {
+ return NULL;
+ }
+
+ msg = ast_sip_mod_data_get(dlg->mod_data, pubsub_module.id, MOD_DATA_MSG);
pj_cstr(&name, header);
return pjsip_msg_find_hdr_by_name(msg, &name, NULL);
@@ -2152,6 +2135,10 @@
{
struct sip_subscription_tree *sub_tree = userdata;
+ if (!sub_tree->dlg) {
+ return 0;
+ }
+
pjsip_dlg_inc_lock(sub_tree->dlg);
/* It's possible that between when the notification was scheduled
* and now, that a new SUBSCRIBE arrived, requiring full state to be
@@ -2204,6 +2191,10 @@
{
int res;
+ if (!sub->tree->dlg) {
+ return 0;
+ }
+
pjsip_dlg_inc_lock(sub->tree->dlg);
if (!sub->tree->evsub) {
@@ -2245,7 +2236,13 @@
void ast_sip_subscription_get_remote_uri(struct ast_sip_subscription *sub, char *buf, size_t size)
{
- pjsip_dialog *dlg = sub->tree->dlg;
+ pjsip_dialog *dlg;
+
+ dlg = sub->tree->dlg;
+ if (!dlg) {
+ *buf = '\0';
+ return;
+ }
ast_copy_pj_str(buf, &dlg->remote.info_str, size);
}
@@ -3199,6 +3196,10 @@
{
struct sip_subscription_tree *sub_tree = userdata;
+ if (!sub_tree->dlg) {
+ return 0;
+ }
+
pjsip_dlg_inc_lock(sub_tree->dlg);
if (!sub_tree->evsub) {
pjsip_dlg_dec_lock(sub_tree->dlg);
@@ -3285,7 +3286,11 @@
pjsip_evsub_set_mod_data(evsub, pubsub_module.id, NULL);
sub_tree->evsub = NULL;
+ ast_sip_dialog_set_serializer(sub_tree->dlg, NULL);
+ ast_sip_dialog_set_endpoint(sub_tree->dlg, NULL);
+ sub_tree->dlg = NULL;
shutdown_subscriptions(sub_tree->root);
+
/* Remove evsub's reference to the sub_tree */
ao2_ref(sub_tree, -1);
}
@@ -3294,6 +3299,10 @@
{
struct sip_subscription_tree *sub_tree = userdata;
+ if (!sub_tree->dlg) {
+ return 0;
+ }
+
pjsip_dlg_inc_lock(sub_tree->dlg);
if (!sub_tree->evsub) {
pjsip_dlg_dec_lock(sub_tree->dlg);
--
To view, visit https://gerrit.asterisk.org/1496
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I21f5dac33529f65cec45679bdace60670800ff66
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
More information about the asterisk-code-review
mailing list