[Asterisk-code-review] astobj2: Assert on attempt to lock object with no locking. (asterisk[master])

[Corey Farrell]

Corey Farrell has uploaded this change for review. ( https://gerrit.asterisk.org/10376


Change subject: astobj2: Assert on attempt to lock object with no locking.
......................................................................

astobj2: Assert on attempt to lock object with no locking.

Add assertion when __ao2_lock or __ao2_trylock are called against an
object that does not contain a lock.  No assertion is performed by
__adjust_lock as this is used by functions which use the OBJ_NOLOCK
flag.  Inject OBJ_NOLOCK for standard ao2_container operations if the
container was allocated without locking.

Remove invalid locking of PJSIP CLI formatter registry.

Change-Id: I5518e55739769792a76f766ce25880d7d944f63b
---
M main/astobj2.c
M main/astobj2_container.c
M main/astobj2_private.h
M res/res_pjsip/pjsip_cli.c
4 files changed, 32 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/76/10376/1

diff --git a/main/astobj2.c b/main/astobj2.c
index 23109a6..13f4648 100644
--- a/main/astobj2.c
+++ b/main/astobj2.c
@@ -178,6 +178,16 @@
 	return !p || IS_AO2_MAGIC_BAD(p) ? 0 : 1;
 }
 
+int internal_ao2_nolock(void *user_data)
+{
+	struct astobj2 *p = INTERNAL_OBJ(user_data);
+
+	ast_assert(p != NULL);
+
+	return (p->priv_data.options & AO2_ALLOC_OPT_LOCK_MASK) == AO2_ALLOC_OPT_LOCK_NOLOCK
+		? OBJ_NOLOCK : 0;
+}
+
 void log_bad_ao2(void *user_data, const char *file, int line, const char *func)
 {
 	struct astobj2 *p;
@@ -242,7 +252,8 @@
 		break;
 	case AO2_ALLOC_OPT_LOCK_NOLOCK:
 		/* The ao2 object has no lock. */
-		break;
+		__ast_assert_failed(0, "ao2_lock called on object with no locking.", file, line, func);
+		return -1;
 	case AO2_ALLOC_OPT_LOCK_OBJ:
 		obj_lockobj = INTERNAL_OBJ_LOCKOBJ(user_data);
 		res = __ao2_lock(obj_lockobj->lockobj.lock, lock_how, file, func, line, var);
@@ -358,7 +369,8 @@
 		break;
 	case AO2_ALLOC_OPT_LOCK_NOLOCK:
 		/* The ao2 object has no lock. */
-		return 0;
+		__ast_assert_failed(0, "ao2_trylock called on object with no locking.", file, line, func);
+		return -1;
 	case AO2_ALLOC_OPT_LOCK_OBJ:
 		obj_lockobj = INTERNAL_OBJ_LOCKOBJ(user_data);
 		res = __ao2_trylock(obj_lockobj->lockobj.lock, lock_how, file, func, line, var);
@@ -433,6 +445,8 @@
 		ast_log(LOG_ERROR, "Invalid lock option on ao2 object %p\n", user_data);
 		/* Fall through */
 	case AO2_ALLOC_OPT_LOCK_NOLOCK:
+		/* No assertion for __adjust_lock, this is used internally by AO2 code
+		 * that uses OBJ_NOLOCK. */
 	case AO2_ALLOC_OPT_LOCK_MUTEX:
 		orig_lock = AO2_LOCK_REQ_MUTEX;
 		break;
diff --git a/main/astobj2_container.c b/main/astobj2_container.c
index 9bea58f..88810dc 100644
--- a/main/astobj2_container.c
+++ b/main/astobj2_container.c
@@ -109,6 +109,7 @@
 		return 0;
 	}
 
+	flags |= internal_ao2_nolock(self);
 	if (flags & OBJ_NOLOCK) {
 		orig_lock = __adjust_lock(self, AO2_LOCK_REQ_WRLOCK, 1);
 	} else {
@@ -296,6 +297,7 @@
 	}
 
 	/* avoid modifications to the content */
+	flags |= internal_ao2_nolock(self);
 	if (flags & OBJ_NOLOCK) {
 		if (flags & OBJ_UNLINK) {
 			orig_lock = __adjust_lock(self, AO2_LOCK_REQ_WRLOCK, 1);
@@ -489,6 +491,10 @@
 		.flags = flags
 	};
 
+	if (internal_ao2_nolock(c)) {
+		a.flags |= AO2_ITERATOR_DONTLOCK;
+	}
+
 	ao2_t_ref(c, +1, "Init iterator with container.");
 
 	return a;
diff --git a/main/astobj2_private.h b/main/astobj2_private.h
index ef47ed7..9975112 100644
--- a/main/astobj2_private.h
+++ b/main/astobj2_private.h
@@ -61,6 +61,16 @@
 #define is_ao2_object(user_data) \
 	__is_ao2_object(user_data, __FILE__, __LINE__, __PRETTY_FUNCTION__)
 
+/*!
+ * \brief Determine if locking should be skipped.
+ *
+ * \retval OBJ_NOLOCK if the object was allocated with AO2_ALLOC_OPT_LOCK_NOLOCK.
+ * \retval 0 if the object has a lock.
+ *
+ * \note This function is not NULL safe and assumes \a user_data points to a valid object.
+ * */
+int internal_ao2_nolock(void *user_data);
+
 enum ao2_lock_req __adjust_lock(void *user_data, enum ao2_lock_req lock_how, int keep_stronger);
 
 #endif /* ASTOBJ2_PRIVATE_H_ */
diff --git a/res/res_pjsip/pjsip_cli.c b/res/res_pjsip/pjsip_cli.c
index 4544a17..52b1f76 100644
--- a/res/res_pjsip/pjsip_cli.c
+++ b/res/res_pjsip/pjsip_cli.c
@@ -326,11 +326,9 @@
 int ast_sip_unregister_cli_formatter(struct ast_sip_cli_formatter_entry *formatter)
 {
 	if (formatter) {
-		ao2_wrlock(formatter_registry);
 		if (ao2_ref(formatter, -1) == 2) {
 			ao2_unlink_flags(formatter_registry, formatter, OBJ_NOLOCK);
 		}
-		ao2_unlock(formatter_registry);
 	}
 	return 0;
 }

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5518e55739769792a76f766ce25880d7d944f63b
Gerrit-Change-Number: 10376
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181002/6015a935/attachment-0001.html>