Fix ao2_iterator API to hold references to containers being iterated.

See Mantis issue for details of what prompted this change.

Additional notes:

This patch changes the ao2_iterator API in two ways: F_AO2I_DONTLOCK
has become an enum instead of a macro, with a name that fits our
naming policy; also, it is now necessary to call
ao2_iterator_destroy() on any iterator that has been
created. Currently this only releases the reference to the container
being iterated, but in the future this could also release other
resources used by the iterator, if the iterator implementation changes
to use additional resources.

(closes issue #15987)
Reported by: kpfleming

Review: https://reviewboard.asterisk.org/r/383/


git-svn-id: http://svn.digium.com/svn/asterisk/branches/1.4@222152 f38db490-d61c-443f-a65b-d21fe96a405b

5 files changed, 107 insertions, 7 deletions

diff --git a/apps/app_queue.c b/apps/app_queue.c
index cfcc9fb05..a94538228 100644
--- a/apps/app_queue.c
+++ b/apps/app_queue.c
@@ -578,7 +578,7 @@ static enum queue_member_status get_member_status(struct call_queue *q, int max_
 			return QUEUE_NORMAL;
 		}
 	}
-
+	ao2_iterator_destroy(&mem_iter);
 	ast_mutex_unlock(&q->lock);
 	return result;
 }
@@ -634,6 +634,7 @@ static int update_status(const char *interface, const int status)
 			}
 			ao2_ref(cur, -1);
 		}
+		ao2_iterator_destroy(&mem_iter);
 		ast_mutex_unlock(&q->lock);
 	}
 	AST_LIST_UNLOCK(&queues);
@@ -924,6 +925,7 @@ static int interface_exists_global(const char *interface)
 			}
 			ao2_ref(mem, -1);
 		}
+		ao2_iterator_destroy(&mem_iter);
 		ast_mutex_unlock(&q->lock);
 		if (ret)
 			break;
@@ -1182,6 +1184,7 @@ static void free_members(struct call_queue *q, int all)
 		}
 		ao2_ref(cur, -1);
 	}
+	ao2_iterator_destroy(&mem_iter);
 }
 
 static void destroy_queue(struct call_queue *q)
@@ -1291,6 +1294,7 @@ static struct call_queue *find_queue_by_name_rt(const char *queuename, struct as
 			m->dead = 1;
 		ao2_ref(m, -1);
 	}
+	ao2_iterator_destroy(&mem_iter);
 
 	while ((interface = ast_category_browse(member_config, interface))) {
 		rt_handle_member_record(q, interface,
@@ -1312,6 +1316,7 @@ static struct call_queue *find_queue_by_name_rt(const char *queuename, struct as
 		}
 		ao2_ref(m, -1);
 	}
+	ao2_iterator_destroy(&mem_iter);
 
 	ast_mutex_unlock(&q->lock);
 
@@ -1362,6 +1367,7 @@ static void update_realtime_members(struct call_queue *q)
 			m->dead = 1;
 		ao2_ref(m, -1);
 	}
+	ao2_iterator_destroy(&mem_iter);
 
 	while ((interface = ast_category_browse(member_config, interface))) {
 		rt_handle_member_record(q, interface,
@@ -1383,6 +1389,7 @@ static void update_realtime_members(struct call_queue *q)
 		}
 		ao2_ref(m, -1);
 	}
+	ao2_iterator_destroy(&mem_iter);
 	ast_mutex_unlock(&q->lock);
 	ast_config_destroy(member_config);
 }
@@ -1782,6 +1789,7 @@ static int num_available_members(struct call_queue *q)
 			break;
 		}
 	}
+	ao2_iterator_destroy(&mem_iter);
 
 	return avl;
 }
@@ -2873,6 +2881,7 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
 		struct ast_dialed_interface *di;
 		AST_LIST_HEAD(, ast_dialed_interface) *dialed_interfaces;
 		if (!tmp) {
+			ao2_iterator_destroy(&memi);
 			ao2_ref(cur, -1);
 			ast_mutex_unlock(&qe->parent->lock);
 			if (use_weight)
@@ -2881,6 +2890,7 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
 		}
 		if (!datastore) {
 			if (!(datastore = ast_channel_datastore_alloc(&dialed_interface_info, NULL))) {
+				ao2_iterator_destroy(&memi);
 				ao2_ref(cur, -1);
 				ast_mutex_unlock(&qe->parent->lock);
 				if (use_weight)
@@ -2890,6 +2900,7 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
 			}
 			datastore->inheritance = DATASTORE_INHERIT_FOREVER;
 			if (!(dialed_interfaces = ast_calloc(1, sizeof(*dialed_interfaces)))) {
+				ao2_iterator_destroy(&memi);
 				ao2_ref(cur, -1);
 				ast_mutex_unlock(&qe->parent->lock);
 				if (use_weight)
@@ -2927,6 +2938,7 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
 		 * it won't call one that has already been called. */
 		if (strncasecmp(cur->interface, "Local/", 6)) {
 			if (!(di = ast_calloc(1, sizeof(*di) + strlen(cur->interface)))) {
+				ao2_iterator_destroy(&memi);
 				ao2_ref(cur, -1);
 				ast_mutex_unlock(&qe->parent->lock);
 				if (use_weight)
@@ -2962,6 +2974,7 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
 			free(tmp);
 		}
 	}
+	ao2_iterator_destroy(&memi);
 	if (qe->expire && (!qe->parent->timeout || (qe->expire - now) <= qe->parent->timeout))
 		to = (qe->expire - now) * 1000;
 	else
@@ -3330,10 +3343,13 @@ static struct member *interface_exists(struct call_queue *q, const char *interfa
 
 	mem_iter = ao2_iterator_init(q->members, 0);
 	while ((mem = ao2_iterator_next(&mem_iter))) {
-		if (!strcasecmp(interface, mem->interface))
+		if (!strcasecmp(interface, mem->interface)) {
+			ao2_iterator_destroy(&mem_iter);
 			return mem;
+		}
 		ao2_ref(mem, -1);
 	}
+	ao2_iterator_destroy(&mem_iter);
 
 	return NULL;
 }
@@ -3375,6 +3391,7 @@ static void dump_queue_members(struct call_queue *pm_queue)
 		}
 		value_len += res;
 	}
+	ao2_iterator_destroy(&mem_iter);
 	
 	if (value_len && !cur_member) {
 		if (ast_db_put(pm_family, pm_queue->name, value))
@@ -4228,6 +4245,7 @@ static int queue_function_qac(struct ast_channel *chan, char *cmd, char *data, c
 			}
 			ao2_ref(m, -1);
 		}
+		ao2_iterator_destroy(&mem_iter);
 		ast_mutex_unlock(&q->lock);
 	} else
 		ast_log(LOG_WARNING, "queue %s was not found\n", data);
@@ -4326,6 +4344,7 @@ static int queue_function_queuememberlist(struct ast_channel *chan, char *cmd, c
 			}
 			ao2_ref(m, -1);
 		}
+		ao2_iterator_destroy(&mem_iter);
 		ast_mutex_unlock(&q->lock);
 	} else
 		ast_log(LOG_WARNING, "queue %s was not found\n", data);
@@ -4458,6 +4477,7 @@ static int reload_queues(void)
 					}
 					ao2_ref(cur, -1);
 				}
+				ao2_iterator_destroy(&mem_iter);
 				for (var = ast_variable_browse(cfg, cat); var; var = var->next) {
 					if (!strcasecmp(var->name, "member")) {
 						struct member tmpmem;
@@ -4536,6 +4556,7 @@ static int reload_queues(void)
 					remove_from_interfaces(cur->state_interface);
 					ao2_ref(cur, -1);
 				}
+				ao2_iterator_destroy(&mem_iter);
 
 				if (q->strategy == QUEUE_STRATEGY_ROUNDROBIN)
 					rr_dep_warning();
@@ -4564,6 +4585,7 @@ static int reload_queues(void)
 				cur->status = ast_device_state(cur->state_interface);
 				ao2_ref(cur, -1);
 			}
+			ao2_iterator_destroy(&mem_iter);
 			ast_mutex_unlock(&q->lock);
 		}
 	}
@@ -4686,6 +4708,7 @@ static int __queues_show(struct mansession *s, int manager, int fd, int argc, ch
 					ast_cli(fd, "      %s%s%s", mem->membername, max_buf, term);
 				ao2_ref(mem, -1);
 			}
+			ao2_iterator_destroy(&mem_iter);
 		} else if (s)
 			astman_append(s, "   No Members%s", term);
 		else	
@@ -4828,6 +4851,7 @@ static int manager_queues_status(struct mansession *s, const struct message *m)
 				}
 				ao2_ref(mem, -1);
 			}
+			ao2_iterator_destroy(&mem_iter);
 			/* List Queue Entries */
 			pos = 1;
 			for (qe = q->head; qe; qe = qe->next) {
@@ -4845,6 +4869,7 @@ static int manager_queues_status(struct mansession *s, const struct message *m)
 					S_OR(qe->chan->cid.cid_name, "unknown"),
 					(long) (now - qe->start), idText);
 			}
+			ao2_iterator_destroy(&mem_iter);
 		}
 		ast_mutex_unlock(&q->lock);
 	}
@@ -5120,6 +5145,7 @@ static char *complete_queue_remove_member(const char *line, const char *word, in
 			while ((m = ao2_iterator_next(&mem_iter))) {
 				if (++which > state) {
 					char *tmp;
+					ao2_iterator_destroy(&mem_iter);
 					ast_mutex_unlock(&q->lock);
 					tmp = ast_strdup(m->interface);
 					ao2_ref(m, -1);
@@ -5127,6 +5153,7 @@ static char *complete_queue_remove_member(const char *line, const char *word, in
 				}
 				ao2_ref(m, -1);
 			}
+			ao2_iterator_destroy(&mem_iter);
 			ast_mutex_unlock(&q->lock);
 		}
 	}
diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index a9da5b732..ccf316273 100644
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -1325,6 +1325,7 @@ static int iax2_getpeername(struct sockaddr_in sin, char *host, int len)
 		}
 		peer_unref(peer);
 	}
+	ao2_iterator_destroy(&i);
 
 	if (!peer) {
 		peer = realtime_peer(NULL, &sin);
@@ -2132,6 +2133,7 @@ static int iax2_show_callnumber_usage(int fd, int argc, char *argv[])
 		}
 		ao2_ref(peercnt, -1);
 	}
+	ao2_iterator_destroy(&i);
 	if (argc == 4) {
 		ast_cli(fd, "\nNon-CallToken Validation Limit: %d\nNon-CallToken Validated: %d\n", global_maxcallno_nonval, total_nonval_callno_used);
 	} else if (argc == 5 && !found) {
@@ -3276,6 +3278,7 @@ static char *complete_iax2_show_peer(const char *line, const char *word, int pos
 		}
 		peer_unref(peer);
 	}
+	ao2_iterator_destroy(&i);
 
 	return res;
 }
@@ -4870,6 +4873,7 @@ static int iax2_getpeertrunk(struct sockaddr_in sin)
 		}
 		peer_unref(peer);
 	}
+	ao2_iterator_destroy(&i);
 
 	return res;
 }
@@ -5700,6 +5704,7 @@ static int iax2_show_users(int fd, int argc, char *argv[])
 			user->contexts ? user->contexts->context : context,
 			user->ha ? "Yes" : "No", pstr);
 	}
+	ao2_iterator_destroy(&i);
 
 	if (havepattern)
 		regfree(&regexbuf);
@@ -5817,6 +5822,7 @@ static int __iax2_show_peers(int manager, int fd, struct mansession *s, int argc
 				peer->encmethods ? "(E)" : "   ", status, term);
 		total_peers++;
 	}
+	ao2_iterator_destroy(&i);
 
 	if (s)
 		astman_append(s,"%d iax2 peers [%d online, %d offline, %d unmonitored]%s", total_peers, online_peers, offline_peers, unmonitored_peers, term);
@@ -6348,6 +6354,7 @@ static int check_access(int callno, struct sockaddr_in *sin, struct iax_ies *ies
 		ast_codec_pref_convert(&iaxs[callno]->rprefs, ies->codec_prefs, 32, 0);
 		ast_codec_pref_convert(&iaxs[callno]->prefs, ies->codec_prefs, 32, 0);
 	}
+	ao2_iterator_destroy(&i);
 
 	if (!gotcapability) 
 		iaxs[callno]->peercapability = iaxs[callno]->peerformat;
@@ -6415,6 +6422,7 @@ static int check_access(int callno, struct sockaddr_in *sin, struct iax_ies *ies
 		}
 		user_unref(user);
 	}
+	ao2_iterator_destroy(&i);
 	user = best;
 	if (!user && !ast_strlen_zero(iaxs[callno]->username)) {
 		user = realtime_user(iaxs[callno]->username, sin);
@@ -6944,6 +6952,7 @@ static int authenticate_reply(struct chan_iax2_pvt *p, struct sockaddr_in *sin,
 			}
 			peer_unref(peer);
 		}
+		ao2_iterator_destroy(&i);
 		if (!peer) {
 			/* We checked our list and didn't find one.  It's unlikely, but possible, 
 			   that we're trying to authenticate *to* a realtime peer */
@@ -11202,6 +11211,7 @@ static void prune_users(void)
 		}
 		user_unref(user);
 	}
+	ao2_iterator_destroy(&i);
 }
 
 /* Prune peers who still are supposed to be deleted */
@@ -11217,6 +11227,7 @@ static void prune_peers(void)
 		}
 		peer_unref(peer);
 	}
+	ao2_iterator_destroy(&i);
 }
 
 static void set_timing(void)
@@ -11637,6 +11648,7 @@ static void poke_all_peers(void)
 		iax2_poke_peer(peer, 0);
 		peer_unref(peer);
 	}
+	ao2_iterator_destroy(&i);
 }
 static int reload_config(void)
 {
diff --git a/include/asterisk/astobj2.h b/include/asterisk/astobj2.h
index 91836b9e7..b9d814e9f 100644
--- a/include/asterisk/astobj2.h
+++ b/include/asterisk/astobj2.h
@@ -263,6 +263,8 @@ Operations on container include:
 		ao2_ref(o, -1);
 	    }
 
+ 	    ao2_iterator_destroy(&i);
+
 	The difference with the callback is that the control
 	on how to iterate is left to us.
 
@@ -505,6 +507,10 @@ int ao2_match_by_addr(void *user_data, void *arg, int flags);
  * ao2_iterator_next() has its refcount incremented,
  * and the reference must be explicitly released when done with it.
  *
+ * In addition, ao2_iterator_init() will hold a reference to the container
+ * being iterated, which will be freed when ao2_iterator_destroy() is called
+ * to free up the resources used by the iterator (if any).
+ *
  * Example:
  *
  *  \code
@@ -520,6 +526,8 @@ int ao2_match_by_addr(void *user_data, void *arg, int flags);
  *     ao2_ref(o, -1);
  *  }
  *
+ *  ao2_iterator_destroy(&i);
+ *
  *  \endcode
  *
  */
@@ -538,13 +546,13 @@ int ao2_match_by_addr(void *user_data, void *arg, int flags);
  * - a bucket number;
  * - the object_id, which is also the container version number
  *   when the object was inserted. This identifies the object
- *   univoquely, however reaching the desired object requires
+ *   uniquely, however reaching the desired object requires
  *   scanning a list.
  * - a pointer, and a container version when we saved the pointer.
  *   If the container has not changed its version number, then we
  *   can safely follow the pointer to reach the object in constant time.
  * Details are in the implementation of ao2_iterator_next()
- * A freshly-initialized iterator has bucket=0, version = 0.
+ * A freshly-initialized iterator has bucket=0, version=0.
  */
 
 struct ao2_iterator {
@@ -552,7 +560,6 @@ struct ao2_iterator {
 	struct ao2_container *c;
 	/*! operation flags */
 	int flags;
-#define	F_AO2I_DONTLOCK	1	/*!< don't lock when iterating */
 	/*! current bucket */
 	int bucket;
 	/*! container version */
@@ -563,8 +570,47 @@ struct ao2_iterator {
 	unsigned int version;
 };
 
+/*! Flags that can be passed to ao2_iterator_init() to modify the behavior
+ * of the iterator.
+ */
+enum ao2_iterator_flags {
+	/*! Prevents ao2_iterator_next() from locking the container
+	 * while retrieving the next object from it.
+	 */
+	AO2_ITERATOR_DONTLOCK = (1 << 0),
+};
+
+/*!
+ * \brief Create an iterator for a container
+ *
+ * \param c the container
+ * \param flags one or more flags from ao2_iterator_flags
+ *
+ * \retval the constructed iterator
+ *
+ * \note This function does \b not take a pointer to an iterator;
+ *       rather, it returns an iterator structure that should be
+ *       assigned to (overwriting) an existing iterator structure
+ *       allocated on the stack or on the heap.
+ *
+ * This function will take a reference on the container being iterated.
+ *
+ */
 struct ao2_iterator ao2_iterator_init(struct ao2_container *c, int flags);
 
+/*!
+ * \brief Destroy a container iterator
+ *
+ * \param i the iterator to destroy
+ *
+ * \retval none
+ *
+ * This function will release the container reference held by the iterator
+ * and any other resources it may be holding.
+ *
+ */
+void ao2_iterator_destroy(struct ao2_iterator *i);
+
 void *ao2_iterator_next(struct ao2_iterator *a);
 
 #endif /* _ASTERISK_ASTOBJ2_H */
diff --git a/main/astobj2.c b/main/astobj2.c
index e7ec2de91..392aa09de 100644
--- a/main/astobj2.c
+++ b/main/astobj2.c
@@ -580,10 +580,21 @@ struct ao2_iterator ao2_iterator_init(struct ao2_container *c, int flags)
 		.c = c,
 		.flags = flags
 	};
+
+	ao2_ref(c, +1);
 	
 	return a;
 }
 
+/*!
+ * destroy an iterator
+ */
+void ao2_iterator_destroy(struct ao2_iterator *i)
+{
+	ao2_ref(i->c, -1);
+	i->c = NULL;
+}
+
 /*
  * move to the next element in the container.
  */
@@ -596,7 +607,7 @@ void * ao2_iterator_next(struct ao2_iterator *a)
 	if (INTERNAL_OBJ(a->c) == NULL)
 		return NULL;
 
-	if (!(a->flags & F_AO2I_DONTLOCK))
+	if (!(a->flags & AO2_ITERATOR_DONTLOCK))
 		ao2_lock(a->c);
 
 	/* optimization. If the container is unchanged and
@@ -637,7 +648,7 @@ found:
 		ao2_ref(ret, 1);
 	}
 
-	if (!(a->flags & F_AO2I_DONTLOCK))
+	if (!(a->flags & AO2_ITERATOR_DONTLOCK))
 		ao2_unlock(a->c);
 
 	return ret;
diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c
index 141f89c01..630a6db58 100644
--- a/res/res_musiconhold.c
+++ b/res/res_musiconhold.c
@@ -1349,6 +1349,8 @@ static int cli_files_show(int fd, int argc, char *argv[])
 		}
 	}
 
+ 	ao2_iterator_destroy(&i);
+
 	return 0;
 }
 
@@ -1371,6 +1373,8 @@ static int moh_classes_show(int fd, int argc, char *argv[])
 		}
 	}
 
+ 	ao2_iterator_destroy(&i);
+
 	return 0;
 }