[asterisk-commits] rizzo: branch rizzo/astobj2 r47334 - /team/rizzo/astobj2/main/astobj2.c

asterisk-commits at lists.digium.com asterisk-commits at lists.digium.com
Wed Nov 8 11:21:19 MST 2006


Author: rizzo
Date: Wed Nov  8 12:21:19 2006
New Revision: 47334

URL: http://svn.digium.com/view/asterisk?rev=47334&view=rev
Log:
put ref/unref in the right places,
include safety checks to make sure we pass proper objects
to the various astobj2 functions.


Modified:
    team/rizzo/astobj2/main/astobj2.c

Modified: team/rizzo/astobj2/main/astobj2.c
URL: http://svn.digium.com/view/asterisk/team/rizzo/astobj2/main/astobj2.c?rev=47334&r1=47333&r2=47334&view=diff
==============================================================================
--- team/rizzo/astobj2/main/astobj2.c (original)
+++ team/rizzo/astobj2/main/astobj2.c Wed Nov  8 12:21:19 2006
@@ -35,12 +35,16 @@
  * the flags and a pointer to a destructor.
  * The refcount is used to decide when it is time to
  * invoke the destructor.
+ * The magic number is used for consistency check.
  */
 struct __priv_data {
         ast_mutex_t     lock;
         int             ref_counter;
         ao2_destructor_fn destructor_fn;
+	uint32_t magic;	/* magic number */
 };
+
+#define	AO2_MAGIC	0xa570b123
 
 /*!
  * What an astobj2 object looks like: fixed-size private data
@@ -57,7 +61,18 @@
  * From a pointer _p to a user-defined object,
  * return the pointer to the astobj2 structure
  */
-#define INTERNAL_OBJ(_p)	((struct astobj2 *)( (_p) == NULL ? NULL : (char *)(_p) - sizeof(struct astobj2) ))
+static struct astobj2 *INTERNAL_OBJ(void *user_data)
+{
+	struct astobj2 *p;
+
+	assert (user_data != NULL);
+	p = (struct astobj2 *)((char *)(user_data) - sizeof(struct astobj2));
+	if (AO2_MAGIC != (p->priv_data.magic ^ (uint32_t)p) ) {
+		ast_verbose("----!!!!!--------- bad magic number 0x%x for %p\n", p->priv_data.magic, p);
+		p = NULL;
+	}
+	return p;
+}
 
 /*
  * From a pointer _p to an astobj2 object,
@@ -67,12 +82,18 @@
 
 int ao2_lock(void *user_data)
 {
-	return ast_mutex_lock( &INTERNAL_OBJ(user_data)->priv_data.lock );
+	struct astobj2 *p = INTERNAL_OBJ(user_data);
+	if (p == NULL)
+		return -1;
+	return ast_mutex_lock( &p->priv_data.lock );
 }
 
 int ao2_unlock(void *user_data)
 {
-	return ast_mutex_unlock( &INTERNAL_OBJ(user_data)->priv_data.lock );
+	struct astobj2 *p = INTERNAL_OBJ(user_data);
+	if (p == NULL)
+		return -1;
+	return ast_mutex_unlock( &p->priv_data.lock );
 }
 
 /*
@@ -83,12 +104,13 @@
 #if 0
  	ast_log(LOG_NOTICE, "Reference counter changed by %i\n", delta);
 #endif
-	struct astobj2 *obj = INTERNAL_OBJ(user_data);
-
 	int current_value;
 	int ret;
-
-	assert(user_data != NULL);
+	struct astobj2 *obj = INTERNAL_OBJ(user_data);
+
+	if (obj == NULL)
+		return -1;
+
 	/* if delta is 0, just return the refcount */
 	if (delta == 0)
 		return (obj->priv_data.ref_counter);
@@ -96,6 +118,7 @@
 	/* we modify with an atomic operation the reference counter */
 	ret = ast_atomic_fetchadd_int( &obj->priv_data.ref_counter, delta );
 	current_value = ret + delta;
+	// ast_log(LOG_NOTICE, "refcount %d on object %p\n", current_value, user_data);
 	/* this case must never happen */
 	if (current_value < 0) {
 		ast_log(LOG_ERROR, "refcount %d on object %p\n", current_value, user_data);
@@ -140,6 +163,7 @@
 
 	/* init */
 	ast_mutex_init(&obj->priv_data.lock);
+	obj->priv_data.magic = AO2_MAGIC ^ (uint32_t)(obj);
 	obj->priv_data.ref_counter = 1;
 	obj->priv_data.destructor_fn = destructor_fn;	/* can be NULL */
 	ast_atomic_fetchadd_int(&ao2_total_objects, 1);
@@ -242,8 +266,12 @@
 {
 	int i;
 	/* create a new list entry */
-	struct bucket_list *p = ast_calloc(1, sizeof(struct bucket_list));
-
+	struct bucket_list *p;
+	struct astobj2 *obj = INTERNAL_OBJ(user_data);
+
+	if (obj == NULL)
+		return NULL;
+	p = ast_calloc(1, sizeof(struct bucket_list));
 	if (p == NULL)		/* alloc failure, die */
 		return NULL;
 	/* apply the hash function */
@@ -253,7 +281,7 @@
 	i %= c->n_buckets;
 
 	p->next = NULL;
-	p->astobj = INTERNAL_OBJ(user_data);
+	p->astobj = obj;
 	p->version = ast_atomic_fetchadd_int(&c->version, 1);
 	if (c->buckets[i].head == NULL)
 		c->buckets[i].head = p;
@@ -281,11 +309,10 @@
  */
 void *ao2_unlink(ao2_container *c, void *user_data)
 {
-	// ast_verbose("unlinking %p from container\n", user_data);
+	ast_verbose("unlinking %p from container\n", user_data);
+	if (INTERNAL_OBJ(user_data) == NULL)	/* safety check on the argument */
+		return NULL;
 	ao2_callback(c, OBJ_SINGLE | OBJ_UNLINK | OBJ_POINTER, match_by_addr, user_data);
-	/* the container has changed its content, so we update the version */
-	ast_atomic_fetchadd_int(&c->version, 1);
-	ao2_ref(user_data, -1);
 	return NULL;
 }
 
@@ -345,6 +372,7 @@
 
 		while (cur) {
 			int match = cb_fn(EXTERNAL_OBJ(cur->astobj), arg, flags) & (CMP_MATCH | CMP_STOP);
+			// ast_verbose("match on %p returns %d\n", arg, match);
 
 			/* we found the object, performing operations according flags */
 			if (match == 0) {	/* no match, no stop, continue */
@@ -356,8 +384,11 @@
 				break;
 			}
 			/* we have a match (CMP_MATCH) here */
-			if (flags & OBJ_DATA)	/* if must return the object, record the value */
+			if (flags & OBJ_DATA) {	/* if must return the object, record the value */
+				/* it is important to handle this case before the unlink */
 				ret = EXTERNAL_OBJ(cur->astobj);
+				ao2_ref(ret, 1);
+			}
 
 			if (flags & OBJ_UNLINK) {	/* must unlink */
 				struct bucket_list *x = cur;
@@ -369,9 +400,12 @@
 					prev->next = cur->next;
 				if (c->buckets[i].tail == cur) /* update tail if needed */
 					c->buckets[i].tail = prev;
+				ao2_ref(EXTERNAL_OBJ(cur->astobj), -1);
 				cur = cur->next ;	/* prepare for next cycle */
 				free(x);	/* free the link record */
+				/* update number of elements and version */
 				ast_atomic_fetchadd_int(&c->elements, -1);
+				ast_atomic_fetchadd_int(&c->version, 1);
 			} else {	/* prepare for next cycle */
 				prev = cur;
 				cur = cur->next;
@@ -384,18 +418,15 @@
 			}
 			if (flags & OBJ_DATA) {
 #if 0	/* XXX to be completed */
-				/* here build a list of object pointers and increase
-				 * the reference counter for each reference created
+				/*
+				 * This is the multiple-return case. We need to link
+				 * the object in a list. The refcount is already increased.
 				 */
 #endif
 			}
 		}
 	}
 	ao2_unlock(c);
-
-	/* increase the reference counter, unlock the container and return */
-	if ( ret != NULL )
-		ao2_ref(ret, 1);
 	return ret;
 }
 
@@ -568,6 +599,9 @@
 	ast_cli(fd, "testing callbacks again\n");
 	ao2_callback(c1, 0, print_cb, (void*)fd);
 
+	{ int a;
+		ao2_ref(&a, -1);
+	}
 	ast_cli(fd, "destroy container\n");
 	ao2_ref(c1, -1);	/* destroy container */
 	ast_verbose("at the end have %d objects\n",



More information about the asterisk-commits mailing list