[Asterisk-code-review] res pjsip mwi.c: Fix MWI subscription memory corruption crash. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/803

Change subject: res_pjsip_mwi.c: Fix MWI subscription memory corruption crash.
......................................................................

res_pjsip_mwi.c: Fix MWI subscription memory corruption crash.

MWI subscriptions can crash or corrupt memory when using the subscription
datastore to access the MWI subscription object because the datastore is
not holding a reference to the object.

* Give the subscription datastore a ref to the MWI subscription object.
It is unfortunate that the ref causes a circular ref chain that must be
explicitly broken to allow the memory to get released.  The loop is broken
when the subscription is shutdown and if the subscription setup fails.

ASTERISK-25168 #close
Reported by: Carl Fortin

Change-Id: Ice4fa823f138ff10a6c74d280699c41a82836d4f
---
M res/res_pjsip_mwi.c
1 file changed, 38 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/03/803/1

diff --git a/res/res_pjsip_mwi.c b/res/res_pjsip_mwi.c
index eae0293..2c1cb02 100644
--- a/res/res_pjsip_mwi.c
+++ b/res/res_pjsip_mwi.c
@@ -138,9 +138,17 @@
 
 	/* Safe strcpy */
 	strcpy(mwi_stasis_sub->mailbox, mailbox);
+
+	ast_debug(3, "Creating stasis MWI subscription to mailbox %s for endpoint %s\n",
+		mailbox, mwi_sub->id);
 	ao2_ref(mwi_sub, +1);
-	ast_debug(3, "Creating stasis MWI subscription to mailbox %s for endpoint %s\n", mailbox, mwi_sub->id);
 	mwi_stasis_sub->stasis_sub = stasis_subscribe_pool(topic, mwi_stasis_cb, mwi_sub);
+	if (!mwi_stasis_sub->stasis_sub) {
+		/* Failed to subscribe. */
+		ao2_ref(mwi_stasis_sub, -1);
+		ao2_ref(mwi_sub, -1);
+		mwi_stasis_sub = NULL;
+	}
 	return mwi_stasis_sub;
 }
 
@@ -491,25 +499,41 @@
 
 	mwi_sub = mwi_datastore->data;
 	ao2_callback(mwi_sub->stasis_subs, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, unsubscribe_stasis, NULL);
+	ast_sip_subscription_remove_datastore(sub, MWI_DATASTORE);
 
 	ao2_ref(mwi_datastore, -1);
 }
 
-static struct ast_datastore_info mwi_ds_info = { };
+static void mwi_ds_destroy(void *data)
+{
+	struct mwi_subscription *sub = data;
+
+	ao2_ref(sub, -1);
+}
+
+static struct ast_datastore_info mwi_ds_info = {
+	.destroy = mwi_ds_destroy,
+};
 
 static int add_mwi_datastore(struct mwi_subscription *sub)
 {
 	struct ast_datastore *mwi_datastore;
+	int res;
 
 	mwi_datastore = ast_sip_subscription_alloc_datastore(&mwi_ds_info, MWI_DATASTORE);
 	if (!mwi_datastore) {
 		return -1;
 	}
+	ao2_ref(sub, +1);
 	mwi_datastore->data = sub;
 
-	ast_sip_subscription_add_datastore(sub->sip_sub, mwi_datastore);
+	/*
+	 * NOTE:  Adding the datastore to the subscription creates a ref loop
+	 * that must be manually broken.
+	 */
+	res = ast_sip_subscription_add_datastore(sub->sip_sub, mwi_datastore);
 	ao2_ref(mwi_datastore, -1);
-	return 0;
+	return res;
 }
 
 /*!
@@ -621,8 +645,8 @@
 	}
 
 	if (add_mwi_datastore(sub)) {
-		ast_log(LOG_WARNING, "Unable to allocate datastore on MWI "
-			"subscription from %s\n", sub->id);
+		ast_log(LOG_WARNING, "Unable to add datastore for MWI subscription to %s\n",
+			sub->id);
 		ao2_ref(sub, -1);
 		return NULL;
 	}
@@ -715,12 +739,19 @@
 	} else {
 		sub = mwi_subscribe_single(endpoint, sip_sub, resource);
 	}
-
 	if (!sub) {
 		ao2_cleanup(endpoint);
 		return -1;
 	}
 
+	if (!ao2_container_count(sub->stasis_subs)) {
+		/*
+		 * We setup no MWI subscriptions so remove the MWI datastore
+		 * to break the ref loop.
+		 */
+		ast_sip_subscription_remove_datastore(sip_sub, MWI_DATASTORE);
+	}
+
 	ao2_cleanup(sub);
 	ao2_cleanup(endpoint);
 	return 0;

-- 
To view, visit https://gerrit.asterisk.org/803
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ice4fa823f138ff10a6c74d280699c41a82836d4f
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>