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

[Matt Jordan]

Matt Jordan has submitted this change and it was merged.

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(-)

Approvals:
  Mark Michelson: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, approved



diff --git a/res/res_pjsip_mwi.c b/res/res_pjsip_mwi.c
index 36490d9..e9d73f8 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/806
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ice4fa823f138ff10a6c74d280699c41a82836d4f
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>