[Asterisk-code-review] res pjsip registrar: lock transport monitor when setting 're... (asterisk[16])

[Joshua C. Colp]

Joshua C. Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/10969 )

Change subject: res_pjsip_registrar: lock transport monitor when setting 'removing' flag
......................................................................

res_pjsip_registrar: lock transport monitor when setting 'removing' flag

A previous patch attempt to mitigate blocked threads on transport shutdown for
a given contact. It was thought that a second lock could be avoided by checking
the 'removing' flag on the transport monitor twice (once before and once after
the normal named aor locking). However as with usual threading issues if the
timing was right the original problem still occured.

This patch adds locking around the first 'removing' flag check and set, thus
nullifying the secondary check, so it was removed.

ASTERISK-28213

Change-Id: Iaa8e36e5311789549b76d8de42dfcea96013b2ed
---
M res/res_pjsip_registrar.c
1 file changed, 12 insertions(+), 18 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, but someone else must approve
  Joshua C. Colp: Looks good to me, approved; Approved for Submit



diff --git a/res/res_pjsip_registrar.c b/res/res_pjsip_registrar.c
index 83fbdf8..e6b7901 100644
--- a/res/res_pjsip_registrar.c
+++ b/res/res_pjsip_registrar.c
@@ -341,25 +341,15 @@
 
 	aor = ast_sip_location_retrieve_aor(monitor->aor_name);
 	if (!aor) {
+		ao2_lock(monitor);
+		monitor->removing = 0;
+		ao2_unlock(monitor);
 		ao2_ref(monitor, -1);
 		return 0;
 	}
 
 	ao2_lock(aor);
 
-	/*
-	 * We're now locked so check again to make sure some other thread is not
-	 * currently removing the contact, or already has.
-	 */
-	if (monitor->removing) {
-		ao2_unlock(aor);
-		ao2_ref(aor, -1);
-		ao2_ref(monitor, -1);
-		return 0;
-	}
-
-	monitor->removing = 1;
-
 	contact = ast_sip_location_retrieve_contact(monitor->contact_name);
 	if (contact) {
 		ast_sip_location_delete_contact(contact);
@@ -400,14 +390,15 @@
 	 * same monitor from different threads. Only one of the calls needs to do the
 	 * actual removing of the contact, so if one is currently removing then any
 	 * subsequent calls can skip.
-	 *
-	 * We'll call it non locked here, but check again once locked just in case the
-	 * flag was updated (see register_contact_transport_remove_cb).
 	 */
+	ao2_lock(monitor);
 	if (monitor->removing) {
+		ao2_unlock(monitor);
 		return;
 	}
 
+	monitor->removing = 1;
+
 	/*
 	 * Push off to a default serializer.  This is in case sorcery
 	 * does database accesses for contacts.  Database accesses may
@@ -416,8 +407,11 @@
 	 */
 	ao2_ref(monitor, +1);
 	if (ast_sip_push_task(NULL, register_contact_transport_remove_cb, monitor)) {
+		monitor->removing = 0;
 		ao2_ref(monitor, -1);
 	}
+
+	ao2_unlock(monitor);
 }
 
 AST_VECTOR(excess_contact_vector, struct ast_sip_contact *);
@@ -728,8 +722,8 @@
 				 * the contact won't be valid anymore if that happens.
 				 */
 				contact_name = ast_sorcery_object_get_id(contact);
-				monitor = ao2_alloc_options(sizeof(*monitor) + 2 + strlen(aor_name)
-					+ strlen(contact_name), NULL, AO2_ALLOC_OPT_LOCK_NOLOCK);
+				monitor = ao2_alloc(sizeof(*monitor) + 2 + strlen(aor_name)
+					+ strlen(contact_name), NULL);
 				if (monitor) {
 					strcpy(monitor->aor_name, aor_name);/* Safe */
 					monitor->contact_name = monitor->aor_name + strlen(aor_name) + 1;

-- 
To view, visit https://gerrit.asterisk.org/10969
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaa8e36e5311789549b76d8de42dfcea96013b2ed
Gerrit-Change-Number: 10969
Gerrit-PatchSet: 3
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190208/4b33c347/attachment.html>