[Asterisk-code-review] res pjsip pubsub: Eliminate race during initial NOTIFY. (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_pjsip_pubsub: Eliminate race during initial NOTIFY.
......................................................................


res_pjsip_pubsub: Eliminate race during initial NOTIFY.

There is a slim chance of a race condition occurring where two threads
can both attempt to manipulate the same area.

Thread A can be handling an incoming initial SUBSCRIBE request. Thread A
lets the specific subscription handler know that the subscription has
been established.

At this point, Thread B may detect a state change on the subscribed
resource and queue up a notification task on Thread C, the subscription
serializer thread.

Now Thread A attempts to generate the initial NOTIFY request to send to
the subscriber at the same time that Thread C attempts to generate a
state change NOTIFY request to send to the subscriber.

The result is that Threads A and C can step on the same memory area,
resulting in a crash. The crash has been observed as happening when
attempting to allocate more space to hold the body for the NOTIFY.

The solution presented here is to queue the subscription establishment
and initial NOTIFY generation onto the subscription serializer thread
(Thread C in the above scenario). This way, there is no way that a state
change notification can occur before the initial NOTIFY is sent, and if
there is a quick succession of NOTIFYs, we can guarantee that the two
NOTIFY requests will be sent in succession.

Change-Id: I5a89a77b5f2717928c54d6efb9955e5f6f5cf815
---
M res/res_pjsip_pubsub.c
1 file changed, 23 insertions(+), 10 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/res/res_pjsip_pubsub.c b/res/res_pjsip_pubsub.c
index e3b1024..7d84b46 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -1306,7 +1306,7 @@
 	return sub_tree;
 }
 
-static int generate_initial_notify(struct ast_sip_subscription *sub);
+static int initial_notify_task(void *obj);
 static int send_notify(struct sip_subscription_tree *sub_tree, unsigned int force_full_state);
 
 /*! \brief Callback function to perform the actual recreation of a subscription */
@@ -1404,10 +1404,9 @@
 		}
 		sub_tree->persistence = ao2_bump(persistence);
 		subscription_persistence_update(sub_tree, &rdata);
-		if (generate_initial_notify(sub_tree->root)) {
+		if (ast_sip_push_task(sub_tree->serializer, initial_notify_task, ao2_bump(sub_tree))) {
 			pjsip_evsub_terminate(sub_tree->evsub, PJ_TRUE);
-		} else {
-			send_notify(sub_tree, 1);
+			ao2_ref(sub_tree, -1);
 		}
 	} else {
 		ast_sorcery_delete(ast_sip_get_sorcery(), persistence);
@@ -2545,6 +2544,24 @@
 	return res;
 }
 
+static int initial_notify_task(void * obj)
+{
+	struct sip_subscription_tree *sub_tree;
+
+	sub_tree = obj;
+	if (generate_initial_notify(sub_tree->root)) {
+		pjsip_evsub_terminate(sub_tree->evsub, PJ_TRUE);
+	} else {
+		send_notify(sub_tree, 1);
+		ast_test_suite_event_notify("SUBSCRIPTION_ESTABLISHED",
+			"Resource: %s",
+			sub_tree->root->resource);
+	}
+
+	ao2_ref(sub_tree, -1);
+	return 0;
+}
+
 static pj_bool_t pubsub_on_rx_subscribe_request(pjsip_rx_data *rdata)
 {
 	pjsip_expires_hdr *expires_header;
@@ -2632,13 +2649,9 @@
 		sub_tree->persistence = subscription_persistence_create(sub_tree);
 		subscription_persistence_update(sub_tree, rdata);
 		sip_subscription_accept(sub_tree, rdata, resp);
-		if (generate_initial_notify(sub_tree->root)) {
+		if (ast_sip_push_task(sub_tree->serializer, initial_notify_task, ao2_bump(sub_tree))) {
 			pjsip_evsub_terminate(sub_tree->evsub, PJ_TRUE);
-		} else {
-			send_notify(sub_tree, 1);
-			ast_test_suite_event_notify("SUBSCRIPTION_ESTABLISHED",
-					"Resource: %s",
-					sub_tree->root->resource);
+			ao2_ref(sub_tree, -1);
 		}
 	}
 

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5a89a77b5f2717928c54d6efb9955e5f6f5cf815
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>