[Asterisk-code-review] stasis: Fix dial masquerade datastore lifetime (asterisk[13])

[Matt Jordan]

Matt Jordan has submitted this change and it was merged.

Change subject: stasis: Fix dial masquerade datastore lifetime
......................................................................


stasis: Fix dial masquerade datastore lifetime

A recent change went into Asterisk which added reference counts to the
channels stored in a dial masquerade datastore. Unfortunately this
included a reference to the caller in a dialing operation. While all
of the dialed targets have the datastore removed from them upon dialing
completion this did not occur for the caller, causing it to have a
reference to itself that could go never go away (as it depended on
the destruction of the datastore which only happened when the channel
was destroyed). This resulted in the caller channel remaining on the
system despite it having hung up.

This change does the following to fix this issue:

1. The dial masquerade datastore is now removed from the caller upon
dialing completion, just like the dialed targets.
2. Upon destruction of the caller all the dialed targets are also
removed from the dial masquerade datastore (just in case).
3. The reference to the caller has been removed as it should not be
possible for the datastore to now be valid/useful after the lifetime
of the caller has ended.

ASTERISK-25025 #close

Change-Id: I1ef4ca5ca04980028604cc2af5d2992ac3431b3f
---
M main/stasis_channels.c
1 file changed, 55 insertions(+), 4 deletions(-)

Approvals:
  Scott Griepentrog: Looks good to me, but someone else must approve
  Matt Jordan: Looks good to me, approved; Verified



diff --git a/main/stasis_channels.c b/main/stasis_channels.c
index 9ed38c3..ae76c1e 100644
--- a/main/stasis_channels.c
+++ b/main/stasis_channels.c
@@ -364,6 +364,7 @@
 }
 
 static void remove_dial_masquerade(struct ast_channel *peer);
+static void remove_dial_masquerade_caller(struct ast_channel *caller);
 static int set_dial_masquerade(struct ast_channel *caller,
 	struct ast_channel *peer, const char *dialstring);
 
@@ -372,6 +373,11 @@
 	const char *forward)
 {
 	ast_assert(peer != NULL);
+
+	/* XXX With an early bridge the below dial masquerade datastore code could, theoretically,
+	 * go away as the act of changing the channel during dialing would be done using the bridge
+	 * API itself and not a masquerade.
+	 */
 
 	if (caller) {
 		/*
@@ -407,6 +413,7 @@
 			ast_channel_unlock(forwarded);
 		}
 		ast_channel_unlock(peer);
+		remove_dial_masquerade_caller(caller);
 		ast_channel_unlock(caller);
 	}
 }
@@ -1349,7 +1356,6 @@
 	while ((cur = AST_LIST_REMOVE_HEAD(&masq_data->dialed_peers, list))) {
 		dial_target_free(cur);
 	}
-	masq_data->caller = ast_channel_cleanup(masq_data->caller);
 }
 
 static void dial_masquerade_datastore_remove_chan(struct dial_masquerade_datastore *masq_data, struct ast_channel *chan)
@@ -1396,6 +1402,16 @@
  */
 static void dial_masquerade_datastore_destroy(void *data)
 {
+	ao2_ref(data, -1);
+}
+
+/*!
+ * \internal
+ * \brief Datastore destructor for dial_masquerade_datastore
+ */
+static void dial_masquerade_caller_datastore_destroy(void *data)
+{
+	dial_masquerade_datastore_cleanup(data);
 	ao2_ref(data, -1);
 }
 
@@ -1517,6 +1533,13 @@
 	.chan_breakdown = dial_masquerade_breakdown,
 };
 
+static const struct ast_datastore_info dial_masquerade_caller_info = {
+	.type = "stasis-chan-dial-masq",
+	.destroy = dial_masquerade_caller_datastore_destroy,
+	.chan_fixup = dial_masquerade_fixup,
+	.chan_breakdown = dial_masquerade_breakdown,
+};
+
 /*!
  * \internal
  * \brief Find the dial masquerade datastore on the given channel.
@@ -1527,7 +1550,14 @@
  */
 static struct ast_datastore *dial_masquerade_datastore_find(struct ast_channel *chan)
 {
-	return ast_channel_datastore_find(chan, &dial_masquerade_info, NULL);
+	struct ast_datastore *datastore;
+
+	datastore = ast_channel_datastore_find(chan, &dial_masquerade_info, NULL);
+	if (!datastore) {
+		datastore = ast_channel_datastore_find(chan, &dial_masquerade_caller_info, NULL);
+	}
+
+	return datastore;
 }
 
 /*!
@@ -1546,7 +1576,7 @@
 {
 	struct ast_datastore *datastore;
 
-	datastore = ast_datastore_alloc(&dial_masquerade_info, NULL);
+	datastore = ast_datastore_alloc(!masq_data ? &dial_masquerade_caller_info : &dial_masquerade_info, NULL);
 	if (!datastore) {
 		return NULL;
 	}
@@ -1557,7 +1587,7 @@
 			ast_datastore_free(datastore);
 			return NULL;
 		}
-		masq_data->caller = ast_channel_ref(chan);
+		masq_data->caller = chan;
 	}
 
 	datastore->data = masq_data;
@@ -1663,3 +1693,24 @@
 	ast_channel_datastore_remove(peer, datastore);
 	ast_datastore_free(datastore);
 }
+
+static void remove_dial_masquerade_caller(struct ast_channel *caller)
+{
+	struct ast_datastore *datastore;
+	struct dial_masquerade_datastore *masq_data;
+
+	datastore = dial_masquerade_datastore_find(caller);
+	if (!datastore) {
+		return;
+	}
+
+	masq_data = datastore->data;
+	if (!masq_data || !AST_LIST_EMPTY(&masq_data->dialed_peers)) {
+		return;
+	}
+
+	dial_masquerade_datastore_remove_chan(masq_data, caller);
+
+	ast_channel_datastore_remove(caller, datastore);
+	ast_datastore_free(datastore);
+}
\ No newline at end of file

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1ef4ca5ca04980028604cc2af5d2992ac3431b3f
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Scott Griepentrog <sgriepentrog at digium.com>