[Asterisk-code-review] astobj2: Prevent potential deadlocks with ao2 global obj rel... (asterisk[13])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5361 )

Change subject: astobj2: Prevent potential deadlocks with ao2_global_obj_release
......................................................................


astobj2: Prevent potential deadlocks with ao2_global_obj_release

The ao2_global_obj_release() function holds an exclusive lock on the
global object while it is being dereferenced. Any destructors that
run during this time that call ao2_global_obj_ref() will deadlock
because a read lock is required.

Instead, we make the global object inaccessible inside of the write
lock and only dereference it once we have released the lock. This
allows the affected destructors to fail gracefully.

While this doesn't completely solve the referenced issue (the error
message about not being able to create an IQ continues to be shown)
it does solve the backtrace spew that accompanied it.

ASTERISK-21009 #close
Reported by: Marcello Ceschia

Change-Id: Idf40ae136b5070dba22cb576ea8414fbc9939385
---
M include/asterisk/astobj2.h
M main/astobj2.c
2 files changed, 5 insertions(+), 27 deletions(-)

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



diff --git a/include/asterisk/astobj2.h b/include/asterisk/astobj2.h
index 4bd44db..390e0ea 100644
--- a/include/asterisk/astobj2.h
+++ b/include/asterisk/astobj2.h
@@ -741,16 +741,16 @@
  */
 #ifdef REF_DEBUG
 #define ao2_t_global_obj_release(holder, tag)	\
-	__ao2_global_obj_release(&holder, (tag), __FILE__, __LINE__, __PRETTY_FUNCTION__, #holder)
+	__ao2_global_obj_replace_unref(&holder, NULL, (tag), __FILE__, __LINE__, __PRETTY_FUNCTION__, #holder)
 #define ao2_global_obj_release(holder)	\
-	__ao2_global_obj_release(&holder, "", __FILE__, __LINE__, __PRETTY_FUNCTION__, #holder)
+	__ao2_global_obj_replace_unref(&holder, NULL, "", __FILE__, __LINE__, __PRETTY_FUNCTION__, #holder)
 
 #else
 
 #define ao2_t_global_obj_release(holder, tag)	\
-	__ao2_global_obj_release(&holder, NULL, __FILE__, __LINE__, __PRETTY_FUNCTION__, #holder)
+	__ao2_global_obj_replace_unref(&holder, NULL, NULL, __FILE__, __LINE__, __PRETTY_FUNCTION__, #holder)
 #define ao2_global_obj_release(holder)	\
-	__ao2_global_obj_release(&holder, NULL, __FILE__, __LINE__, __PRETTY_FUNCTION__, #holder)
+	__ao2_global_obj_replace_unref(&holder, NULL, NULL, __FILE__, __LINE__, __PRETTY_FUNCTION__, #holder)
 #endif
 
 void __ao2_global_obj_release(struct ao2_global_obj *holder, const char *tag, const char *file, int line, const char *func, const char *name);
diff --git a/main/astobj2.c b/main/astobj2.c
index 569db0b..147df7a 100644
--- a/main/astobj2.c
+++ b/main/astobj2.c
@@ -635,29 +635,7 @@
 
 void __ao2_global_obj_release(struct ao2_global_obj *holder, const char *tag, const char *file, int line, const char *func, const char *name)
 {
-	if (!holder) {
-		/* For sanity */
-		ast_log(LOG_ERROR, "Must be called with a global object!\n");
-		ast_assert(0);
-		return;
-	}
-	if (__ast_rwlock_wrlock(file, line, func, &holder->lock, name)) {
-		/* Could not get the write lock. */
-		ast_assert(0);
-		return;
-	}
-
-	/* Release the held ao2 object. */
-	if (holder->obj) {
-		if (tag) {
-			__ao2_ref_debug(holder->obj, -1, tag, file, line, func);
-		} else {
-			__ao2_ref(holder->obj, -1);
-		}
-		holder->obj = NULL;
-	}
-
-	__ast_rwlock_unlock(file, line, func, &holder->lock, name);
+	__ao2_global_obj_replace_unref(holder, NULL, tag, file, line, func, name);
 }
 
 void *__ao2_global_obj_replace(struct ao2_global_obj *holder, void *obj, const char *tag, const char *file, int line, const char *func, const char *name)

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Idf40ae136b5070dba22cb576ea8414fbc9939385
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>