[asterisk-commits] rmudgett: branch 1.8 r340279 - in /branches/1.8: include/asterisk/ main/

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Tue Oct 11 13:23:18 CDT 2011


Author: rmudgett
Date: Tue Oct 11 13:23:14 2011
New Revision: 340279

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=340279
Log:
Convert registered AMI actions to ao2 objects.

* Fixed race between calling an AMI action callback and unregistering that
action.  Refixes ASTERISK-13784 broken by ASTERISK-17785 change.

* Fixed potential memory leak if an AMI action failed to get registered
because is already was registered.  Part of the ao2 conversion.

* Fixed AMI ListCommands action not walking the actions list with a lock
held.

* Fix usage of ast_strdupa() and alloca() in loops.  Excess stack usage.

* Fix AMI Originate action Variable header requiring a space after the
header colon.  Reported by Yaroslav Panych on the asterisk-dev list.

* Increased the number of listed variables allowed per AMI Originate
action Variable header to 64.

* Fixed AMI GetConfigJSON action output format.

* Fixed usage of res contents outside of scope in append_channel_vars().

* Fixed inconsistency of config file channelvars option.  The values no
longer accumulate with every channelvars option in the config file.  Only
the last value is kept to be consistent with the CLI "manager show
settings" command.

(closes issue ASTERISK-18479)
Reported by: Jaco Kroon

Modified:
    branches/1.8/include/asterisk/manager.h
    branches/1.8/main/manager.c

Modified: branches/1.8/include/asterisk/manager.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/include/asterisk/manager.h?view=diff&rev=340279&r1=340278&r2=340279
==============================================================================
--- branches/1.8/include/asterisk/manager.h (original)
+++ branches/1.8/include/asterisk/manager.h Tue Oct 11 13:23:14 2011
@@ -152,6 +152,13 @@
 	enum ast_doc_src docsrc;
 	/*! For easy linking */
 	AST_RWLIST_ENTRY(manager_action) list;
+	/*!
+	 * \brief TRUE if the AMI action is registered and the callback can be called.
+	 *
+	 * \note Needed to prevent a race between calling the callback
+	 * function and unregestring the AMI action object.
+	 */
+	unsigned int registered:1;
 };
 
 /*! \brief External routines may register/unregister manager callbacks this way 

Modified: branches/1.8/main/manager.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/manager.c?view=diff&rev=340279&r1=340278&r2=340279
==============================================================================
--- branches/1.8/main/manager.c (original)
+++ branches/1.8/main/manager.c Tue Oct 11 13:23:14 2011
@@ -1042,6 +1042,30 @@
 
 static void free_channelvars(void);
 
+/*!
+ * \internal
+ * \brief Find a registered action object.
+ *
+ * \param name Name of AMI action to find.
+ *
+ * \return Reffed action found or NULL
+ */
+static struct manager_action *action_find(const char *name)
+{
+	struct manager_action *act;
+
+	AST_RWLIST_RDLOCK(&actions);
+	AST_RWLIST_TRAVERSE(&actions, act, list) {
+		if (!strcasecmp(name, act->action)) {
+			ao2_t_ref(act, +1, "found action object");
+			break;
+		}
+	}
+	AST_RWLIST_UNLOCK(&actions);
+
+	return act;
+}
+
 /*! \brief Add a custom hook to be called when an event is fired */
 void ast_manager_register_hook(struct manager_custom_hook *hook)
 {
@@ -1058,12 +1082,12 @@
 	AST_RWLIST_UNLOCK(&manager_hooks);
 }
 
-int check_manager_enabled()
+int check_manager_enabled(void)
 {
 	return manager_enabled;
 }
 
-int check_webmanager_enabled()
+int check_webmanager_enabled(void)
 {
 	return (webmanager_enabled && manager_enabled);
 }
@@ -1443,15 +1467,14 @@
 						ast_xmldoc_printable(S_OR(cur->arguments, "Not available"), 1),
 						seealso_title,
 						ast_xmldoc_printable(S_OR(cur->seealso, "Not available"), 1));
-				} else {
+				} else
 #endif
+				{
 					ast_cli(a->fd, "Action: %s\nSynopsis: %s\nPrivilege: %s\n%s\n",
-							cur->action, cur->synopsis,
-							authority_to_str(cur->authority, &authority),
-							S_OR(cur->description, ""));
-#ifdef AST_XML_DOCS
+						cur->action, cur->synopsis,
+						authority_to_str(cur->authority, &authority),
+						S_OR(cur->description, ""));
 				}
-#endif
 			}
 		}
 	}
@@ -1612,8 +1635,9 @@
 	ast_cli(a->fd, HSMC_FORMAT, "------", "---------", "--------");
 
 	AST_RWLIST_RDLOCK(&actions);
-	AST_RWLIST_TRAVERSE(&actions, cur, list)
+	AST_RWLIST_TRAVERSE(&actions, cur, list) {
 		ast_cli(a->fd, HSMC_FORMAT, cur->action, authority_to_str(cur->authority, &authority), cur->synopsis);
+	}
 	AST_RWLIST_UNLOCK(&actions);
 
 	return CLI_SUCCESS;
@@ -1717,15 +1741,23 @@
 	return next;
 }
 
-/*
- * Generic function to return either the first or the last matching header
- * from a list of variables, possibly skipping empty strings.
- * At the moment there is only one use of this function in this file,
- * so we make it static.
- */
 #define	GET_HEADER_FIRST_MATCH	0
 #define	GET_HEADER_LAST_MATCH	1
 #define	GET_HEADER_SKIP_EMPTY	2
+
+/*!
+ * \brief Return a matching header value.
+ *
+ * \details
+ * Generic function to return either the first or the last
+ * matching header from a list of variables, possibly skipping
+ * empty strings.
+ *
+ * \note At the moment there is only one use of this function in
+ * this file, so we make it static.
+ *
+ * \note Never returns NULL.
+ */
 static const char *__astman_get_header(const struct message *m, char *var, int mode)
 {
 	int x, l = strlen(var);
@@ -1737,65 +1769,101 @@
 			const char *value = h + l + 1;
 			value = ast_skip_blanks(value); /* ignore leading spaces in the value */
 			/* found a potential candidate */
-			if (mode & GET_HEADER_SKIP_EMPTY && ast_strlen_zero(value))
+			if ((mode & GET_HEADER_SKIP_EMPTY) && ast_strlen_zero(value)) {
 				continue;	/* not interesting */
-			if (mode & GET_HEADER_LAST_MATCH)
+			}
+			if (mode & GET_HEADER_LAST_MATCH) {
 				result = value;	/* record the last match so far */
-			else
+			} else {
 				return value;
+			}
 		}
 	}
 
 	return result;
 }
 
-/*
- * Return the first matching variable from an array.
- * This is the legacy function and is implemented in therms of
- * __astman_get_header().
+/*!
+ * \brief Return the first matching variable from an array.
+ *
+ * \note This is the legacy function and is implemented in
+ * therms of __astman_get_header().
+ *
+ * \note Never returns NULL.
  */
 const char *astman_get_header(const struct message *m, char *var)
 {
 	return __astman_get_header(m, var, GET_HEADER_FIRST_MATCH);
 }
 
-
-struct ast_variable *astman_get_variables(const struct message *m)
-{
-	int varlen, x, y;
-	struct ast_variable *head = NULL, *cur;
-
+/*!
+ * \internal
+ * \brief Process one "Variable:" header value string.
+ *
+ * \param head Current list of AMI variables to get new values added.
+ * \param hdr_val Header value string to process.
+ *
+ * \return New variable list head.
+ */
+static struct ast_variable *man_do_variable_value(struct ast_variable *head, const char *hdr_val)
+{
+	char *parse;
 	AST_DECLARE_APP_ARGS(args,
-		AST_APP_ARG(vars)[32];
+		AST_APP_ARG(vars)[64];
 	);
 
-	varlen = strlen("Variable: ");
-
-	for (x = 0; x < m->hdrcount; x++) {
-		char *parse, *var, *val;
-
-		if (strncasecmp("Variable: ", m->headers[x], varlen)) {
-			continue;
-		}
-		parse = ast_strdupa(m->headers[x] + varlen);
-
-		AST_STANDARD_APP_ARGS(args, parse);
-		if (!args.argc) {
-			continue;
-		}
+	hdr_val = ast_skip_blanks(hdr_val); /* ignore leading spaces in the value */
+	parse = ast_strdupa(hdr_val);
+
+	/* Break the header value string into name=val pair items. */
+	AST_STANDARD_APP_ARGS(args, parse);
+	if (args.argc) {
+		int y;
+
+		/* Process each name=val pair item. */
 		for (y = 0; y < args.argc; y++) {
+			struct ast_variable *cur;
+			char *var;
+			char *val;
+
 			if (!args.vars[y]) {
 				continue;
 			}
-			var = val = ast_strdupa(args.vars[y]);
+			var = val = args.vars[y];
 			strsep(&val, "=");
+
+			/* XXX We may wish to trim whitespace from the strings. */
 			if (!val || ast_strlen_zero(var)) {
 				continue;
 			}
+
+			/* Create new variable list node and prepend it to the list. */
 			cur = ast_variable_new(var, val, "");
-			cur->next = head;
-			head = cur;
-		}
+			if (cur) {
+				cur->next = head;
+				head = cur;
+			}
+		}
+	}
+
+	return head;
+}
+
+struct ast_variable *astman_get_variables(const struct message *m)
+{
+	int varlen;
+	int x;
+	struct ast_variable *head = NULL;
+
+	static const char var_hdr[] = "Variable:";
+
+	/* Process all "Variable:" headers. */
+	varlen = strlen(var_hdr);
+	for (x = 0; x < m->hdrcount; x++) {
+		if (strncasecmp(var_hdr, m->headers[x], varlen)) {
+			continue;
+		}
+		head = man_do_variable_value(head, m->headers[x] + varlen);
 	}
 
 	return head;
@@ -1807,11 +1875,11 @@
 {
 	const char *action;
 	int ret = 0;
-	struct manager_action *tmp;
+	struct manager_action *act_found;
 	struct mansession s = {.session = NULL, };
 	struct message m = { 0 };
-	char header_buf[1025] = { '\0' };
-	const char *src = msg;
+	char *dup_str;
+	char *src;
 	int x = 0;
 	int curlen;
 
@@ -1819,8 +1887,14 @@
 		return -1;
 	}
 
+	/* Create our own copy of the AMI action msg string. */
+	src = dup_str = ast_strdup(msg);
+	if (!dup_str) {
+		return -1;
+	}
+
 	/* convert msg string to message struct */
-	curlen = strlen(msg);
+	curlen = strlen(src);
 	for (x = 0; x < curlen; x++) {
 		int cr;	/* set if we have \r */
 		if (src[x] == '\r' && x+1 < curlen && src[x+1] == '\n')
@@ -1829,11 +1903,11 @@
 			cr = 1;	/* also accept \n only */
 		else
 			continue;
-		/* don't copy empty lines */
-		if (x) {
-			memmove(header_buf, src, x);	/*... but trim \r\n */
-			header_buf[x] = '\0';		/* terminate the string */
-			m.headers[m.hdrcount++] = ast_strdupa(header_buf);
+		/* don't keep empty lines */
+		if (x && m.hdrcount < ARRAY_LEN(m.headers)) {
+			/* ... but trim \r\n and terminate the header string */
+			src[x] = '\0';
+			m.headers[m.hdrcount++] = src;
 		}
 		x += cr;
 		curlen -= x;		/* remaining size */
@@ -1841,25 +1915,29 @@
 		x = -1;			/* reset loop */
 	}
 
-	action = astman_get_header(&m,"Action");
-	if (action && strcasecmp(action,"login")) {
-
-		AST_RWLIST_RDLOCK(&actions);
-		AST_RWLIST_TRAVERSE(&actions, tmp, list) {
-			if (strcasecmp(action, tmp->action))
-				continue;
+	action = astman_get_header(&m, "Action");
+	if (strcasecmp(action, "login")) {
+		act_found = action_find(action);
+		if (act_found) {
 			/*
-			* we have to simulate a session for this action request
-			* to be able to pass it down for processing
-			* This is necessary to meet the previous design of manager.c
-			*/
+			 * we have to simulate a session for this action request
+			 * to be able to pass it down for processing
+			 * This is necessary to meet the previous design of manager.c
+			 */
 			s.hook = hook;
 			s.f = (void*)1; /* set this to something so our request will make it through all functions that test it*/
-			ret = tmp->func(&s, &m);
-			break;
-		}
-		AST_RWLIST_UNLOCK(&actions);
-	}
+
+			ao2_lock(act_found);
+			if (act_found->registered && act_found->func) {
+				ret = act_found->func(&s, &m);
+			} else {
+				ret = -1;
+			}
+			ao2_unlock(act_found);
+			ao2_t_ref(act_found, -1, "done with found action object");
+		}
+	}
+	ast_free(dup_str);
 	return ret;
 }
 
@@ -2468,6 +2546,24 @@
 	*out = '\0';
 }
 
+/*!
+ * \internal
+ * \brief Append a JSON escaped string to the manager stream.
+ *
+ * \param s AMI stream to append a string.
+ * \param str String to append to the stream after JSON escaping it.
+ *
+ * \return Nothing
+ */
+static void astman_append_json(struct mansession *s, const char *str)
+{
+	char *buf;
+
+	buf = alloca(2 * strlen(str) + 1);
+	json_escape(buf, str);
+	astman_append(s, "%s", buf);
+}
+
 static int action_getconfigjson(struct mansession *s, const struct message *m)
 {
 	struct ast_config *cfg;
@@ -2475,8 +2571,6 @@
 	char *category = NULL;
 	struct ast_variable *v;
 	int comma1 = 0;
-	char *buf = NULL;
-	unsigned int buf_len = 0;
 	struct ast_flags config_flags = { CONFIG_FLAG_WITHCOMMENTS | CONFIG_FLAG_NOCACHE };
 
 	if (ast_strlen_zero(fn)) {
@@ -2492,41 +2586,22 @@
 		return 0;
 	}
 
-	buf_len = 512;
-	buf = alloca(buf_len);
-
 	astman_start_ack(s, m);
 	astman_append(s, "JSON: {");
 	while ((category = ast_category_browse(cfg, category))) {
 		int comma2 = 0;
-		if (buf_len < 2 * strlen(category) + 1) {
-			buf_len *= 2;
-			buf = alloca(buf_len);
-		}
-		json_escape(buf, category);
-		astman_append(s, "%s\"%s\":[", comma1 ? "," : "", buf);
-		if (!comma1) {
-			comma1 = 1;
-		}
+
+		astman_append(s, "%s\"", comma1 ? "," : "");
+		astman_append_json(s, category);
+		astman_append(s, "\":[");
+		comma1 = 1;
 		for (v = ast_variable_browse(cfg, category); v; v = v->next) {
-			if (comma2) {
-				astman_append(s, ",");
-			}
-			if (buf_len < 2 * strlen(v->name) + 1) {
-				buf_len *= 2;
-				buf = alloca(buf_len);
-			}
-			json_escape(buf, v->name);
-			astman_append(s, "\"%s", buf);
-			if (buf_len < 2 * strlen(v->value) + 1) {
-				buf_len *= 2;
-				buf = alloca(buf_len);
-			}
-			json_escape(buf, v->value);
-			astman_append(s, "%s\"", buf);
-			if (!comma2) {
-				comma2 = 1;
-			}
+			astman_append(s, "%s\"", comma2 ? "," : "");
+			astman_append_json(s, v->name);
+			astman_append(s, "\":\"");
+			astman_append_json(s, v->value);
+			astman_append(s, "\"");
+			comma2 = 1;
 		}
 		astman_append(s, "]");
 	}
@@ -2885,19 +2960,20 @@
 	return 0;
 }
 
-/*! \note The actionlock is read-locked by the caller of this function */
 static int action_listcommands(struct mansession *s, const struct message *m)
 {
 	struct manager_action *cur;
-	struct ast_str *temp = ast_str_alloca(BUFSIZ); /* XXX very large ? */
+	struct ast_str *temp = ast_str_alloca(256);
 
 	astman_start_ack(s, m);
+	AST_RWLIST_RDLOCK(&actions);
 	AST_RWLIST_TRAVERSE(&actions, cur, list) {
-		if (s->session->writeperm & cur->authority || cur->authority == 0) {
+		if ((s->session->writeperm & cur->authority) || cur->authority == 0) {
 			astman_append(s, "%s: %s (Priv: %s)\r\n",
 				cur->action, cur->synopsis, authority_to_str(cur->authority, &temp));
 		}
 	}
+	AST_RWLIST_UNLOCK(&actions);
 	astman_append(s, "\r\n");
 
 	return 0;
@@ -4485,14 +4561,12 @@
  */
 static int process_message(struct mansession *s, const struct message *m)
 {
-	char action[80] = "";
 	int ret = 0;
-	struct manager_action *tmp;
-	const char *user = astman_get_header(m, "Username");
-	int (*call_func)(struct mansession *s, const struct message *m) = NULL;
-
-	ast_copy_string(action, __astman_get_header(m, "Action", GET_HEADER_SKIP_EMPTY), sizeof(action));
-
+	struct manager_action *act_found;
+	const char *user;
+	const char *action;
+
+	action = __astman_get_header(m, "Action", GET_HEADER_SKIP_EMPTY);
 	if (ast_strlen_zero(action)) {
 		report_req_bad_format(s, "NONE");
 		mansession_lock(s);
@@ -4501,7 +4575,10 @@
 		return 0;
 	}
 
-	if (!s->session->authenticated && strcasecmp(action, "Login") && strcasecmp(action, "Logoff") && strcasecmp(action, "Challenge")) {
+	if (!s->session->authenticated
+		&& strcasecmp(action, "Login")
+		&& strcasecmp(action, "Logoff")
+		&& strcasecmp(action, "Challenge")) {
 		if (!s->session->authenticated) {
 			report_req_not_allowed(s, action);
 		}
@@ -4511,8 +4588,12 @@
 		return 0;
 	}
 
-	if (!allowmultiplelogin && !s->session->authenticated && user &&
-		(!strcasecmp(action, "Login") || !strcasecmp(action, "Challenge"))) {
+	if (!allowmultiplelogin
+		&& !s->session->authenticated
+		&& (!strcasecmp(action, "Login")
+			|| !strcasecmp(action, "Challenge"))) {
+		user = astman_get_header(m, "Username");
+
 		if (check_manager_session_inuse(user)) {
 			report_session_limit(s);
 			sleep(1);
@@ -4523,37 +4604,38 @@
 		}
 	}
 
-	AST_RWLIST_RDLOCK(&actions);
-	AST_RWLIST_TRAVERSE(&actions, tmp, list) {
-		if (strcasecmp(action, tmp->action)) {
-			continue;
-		}
-		if (s->session->writeperm & tmp->authority || tmp->authority == 0) {
-			call_func = tmp->func;
-		}
-		break;
-	}
-	AST_RWLIST_UNLOCK(&actions);
-
-	if (tmp) {
-		if (call_func) {
-			/* Call our AMI function after we unlock our actions lists */
-			ast_debug(1, "Running action '%s'\n", tmp->action);
-			ret = call_func(s, m);
-		} else {
-			/* If we found our action but don't have a function pointer, access
-			 * was denied, so bail out.
+	act_found = action_find(action);
+	if (act_found) {
+		/* Found the requested AMI action. */
+		int acted = 0;
+
+		if ((s->session->writeperm & act_found->authority)
+			|| act_found->authority == 0) {
+			/* We have the authority to execute the action. */
+			ao2_lock(act_found);
+			if (act_found->registered && act_found->func) {
+				ast_debug(1, "Running action '%s'\n", act_found->action);
+				ret = act_found->func(s, m);
+				acted = 1;
+			}
+			ao2_unlock(act_found);
+		}
+		if (!acted) {
+			/*
+			 * We did not execute the action because access was denied, it
+			 * was no longer registered, or no action was really registered.
+			 * Complain about it and leave.
 			 */
 			report_req_not_allowed(s, action);
 			mansession_lock(s);
 			astman_send_error(s, m, "Permission denied");
 			mansession_unlock(s);
 		}
+		ao2_t_ref(act_found, -1, "done with found action object");
 	} else {
 		char buf[512];
-		if (!tmp) {
-			report_req_bad_format(s, action);
-		}
+
+		report_req_bad_format(s, action);
 		snprintf(buf, sizeof(buf), "Invalid/unknown command: %s. Use Action: ListCommands to show available commands.", action);
 		mansession_lock(s);
 		astman_send_error(s, m, buf);
@@ -4669,44 +4751,85 @@
 	return res;
 }
 
+/*!
+ * \internal
+ * \brief Read and process an AMI action request.
+ *
+ * \param s AMI session to process action request.
+ *
+ * \retval 0 Retain AMI connection for next command.
+ * \retval -1 Drop AMI connection due to logoff or connection error.
+ */
 static int do_message(struct mansession *s)
 {
 	struct message m = { 0 };
 	char header_buf[sizeof(s->session->inbuf)] = { '\0' };
 	int res;
+	int idx;
+	int hdr_loss;
 	time_t now;
 
+	hdr_loss = 0;
 	for (;;) {
 		/* Check if any events are pending and do them if needed */
 		if (process_events(s)) {
-			return -1;
+			res = -1;
+			break;
 		}
 		res = get_input(s, header_buf);
 		if (res == 0) {
+			/* No input line received. */
 			if (!s->session->authenticated) {
-				if(time(&now) == -1) {
+				if (time(&now) == -1) {
 					ast_log(LOG_ERROR, "error executing time(): %s\n", strerror(errno));
-					return -1;
+					res = -1;
+					break;
 				}
 
 				if (now - s->session->authstart > authtimeout) {
 					if (displayconnects) {
 						ast_verb(2, "Client from %s, failed to authenticate in %d seconds\n", ast_inet_ntoa(s->session->sin.sin_addr), authtimeout);
 					}
-					return -1;
+					res = -1;
+					break;
 				}
 			}
 			continue;
 		} else if (res > 0) {
+			/* Input line received. */
 			if (ast_strlen_zero(header_buf)) {
-				return process_message(s, &m) ? -1 : 0;
-			} else if (m.hdrcount < (AST_MAX_MANHEADERS - 1)) {
-				m.headers[m.hdrcount++] = ast_strdupa(header_buf);
+				if (hdr_loss) {
+					mansession_lock(s);
+					astman_send_error(s, &m, "Too many lines in message or allocation failure");
+					mansession_unlock(s);
+					res = 0;
+				} else {
+					res = process_message(s, &m) ? -1 : 0;
+				}
+				break;
+			} else if (m.hdrcount < ARRAY_LEN(m.headers)) {
+				m.headers[m.hdrcount] = ast_strdup(header_buf);
+				if (!m.headers[m.hdrcount]) {
+					/* Allocation failure. */
+					hdr_loss = 1;
+				} else {
+					++m.hdrcount;
+				}
+			} else {
+				/* Too many lines in message. */
+				hdr_loss = 1;
 			}
 		} else {
-			return res;
-		}
-	}
+			/* Input error. */
+			break;
+		}
+	}
+
+	/* Free AMI request headers. */
+	for (idx = 0; idx < m.hdrcount; ++idx) {
+		ast_free((void *) m.headers[idx]);
+	}
+	return res;
 }
 
 /*! \brief The body of the individual manager session.
@@ -4874,14 +4997,18 @@
 static void append_channel_vars(struct ast_str **pbuf, struct ast_channel *chan)
 {
 	struct manager_channel_variable *var;
+
 	AST_RWLIST_RDLOCK(&channelvars);
 	AST_LIST_TRAVERSE(&channelvars, var, entry) {
-		const char *val = "";
+		const char *val;
+		struct ast_str *res;
+
 		if (var->isfunc) {
-			struct ast_str *res = ast_str_thread_get(&manager_event_funcbuf, 16);
-			int ret;
-			if (res && (ret = ast_func_read2(chan, var->name, &res, 0)) == 0) {
+			res = ast_str_thread_get(&manager_event_funcbuf, 16);
+			if (res && ast_func_read2(chan, var->name, &res, 0) == 0) {
 				val = ast_str_buffer(res);
+			} else {
+				val = NULL;
 			}
 		} else {
 			val = pbx_builtin_getvar_helper(chan, var->name);
@@ -4985,23 +5112,29 @@
 int ast_manager_unregister(char *action)
 {
 	struct manager_action *cur;
-	struct timespec tv = { 5, };
-
-	if (AST_RWLIST_TIMEDWRLOCK(&actions, &tv)) {
-		ast_log(LOG_ERROR, "Could not obtain lock on manager list\n");
-		return -1;
-	}
+
+	AST_RWLIST_WRLOCK(&actions);
 	AST_RWLIST_TRAVERSE_SAFE_BEGIN(&actions, cur, list) {
 		if (!strcasecmp(action, cur->action)) {
 			AST_RWLIST_REMOVE_CURRENT(list);
-			ast_string_field_free_memory(cur);
-			ast_free(cur);
-			ast_verb(2, "Manager unregistered action %s\n", action);
 			break;
 		}
 	}
 	AST_RWLIST_TRAVERSE_SAFE_END;
 	AST_RWLIST_UNLOCK(&actions);
+
+	if (cur) {
+		/*
+		 * We have removed the action object from the container so we
+		 * are no longer in a hurry.
+		 */
+		ao2_lock(cur);
+		cur->registered = 0;
+		ao2_unlock(cur);
+
+		ao2_t_ref(cur, -1, "action object removed from list");
+		ast_verb(2, "Manager unregistered action %s\n", action);
+	}
 
 	return 0;
 }
@@ -5019,14 +5152,12 @@
 static int ast_manager_register_struct(struct manager_action *act)
 {
 	struct manager_action *cur, *prev = NULL;
-	struct timespec tv = { 5, };
-
-	if (AST_RWLIST_TIMEDWRLOCK(&actions, &tv)) {
-		ast_log(LOG_ERROR, "Could not obtain lock on manager list\n");
-		return -1;
-	}
+
+	AST_RWLIST_WRLOCK(&actions);
 	AST_RWLIST_TRAVERSE(&actions, cur, list) {
-		int ret = strcasecmp(cur->action, act->action);
+		int ret;
+
+		ret = strcasecmp(cur->action, act->action);
 		if (ret == 0) {
 			ast_log(LOG_WARNING, "Manager: Action '%s' already registered\n", act->action);
 			AST_RWLIST_UNLOCK(&actions);
@@ -5038,6 +5169,8 @@
 		}
 	}
 
+	ao2_t_ref(act, +1, "action object added to list");
+	act->registered = 1;
 	if (prev) {
 		AST_RWLIST_INSERT_AFTER(&actions, prev, act, list);
 	} else {
@@ -5051,16 +5184,36 @@
 	return 0;
 }
 
+/*!
+ * \internal
+ * \brief Destroy the registered AMI action object.
+ *
+ * \param obj Object to destroy.
+ *
+ * \return Nothing
+ */
+static void action_destroy(void *obj)
+{
+	struct manager_action *doomed = obj;
+
+	if (doomed->synopsis) {
+		/* The string fields were initialized. */
+		ast_string_field_free_memory(doomed);
+	}
+}
+
 /*! \brief register a new command with manager, including online help. This is
 	the preferred way to register a manager command */
 int ast_manager_register2(const char *action, int auth, int (*func)(struct mansession *s, const struct message *m), const char *synopsis, const char *description)
 {
-	struct manager_action *cur = NULL;
-#ifdef AST_XML_DOCS
-	char *tmpxml;
-#endif
-
-	if (!(cur = ast_calloc_with_stringfields(1, struct manager_action, 128))) {
+	struct manager_action *cur;
+
+	cur = ao2_alloc(sizeof(*cur), action_destroy);
+	if (!cur) {
+		return -1;
+	}
+	if (ast_string_field_init(cur, 128)) {
+		ao2_t_ref(cur, -1, "action object creation failed");
 		return -1;
 	}
 
@@ -5069,6 +5222,8 @@
 	cur->func = func;
 #ifdef AST_XML_DOCS
 	if (ast_strlen_zero(synopsis) && ast_strlen_zero(description)) {
+		char *tmpxml;
+
 		tmpxml = ast_xmldoc_build_synopsis("manager", action, NULL);
 		ast_string_field_set(cur, synopsis, tmpxml);
 		ast_free(tmpxml);
@@ -5090,19 +5245,21 @@
 		ast_free(tmpxml);
 
 		cur->docsrc = AST_XML_DOC;
-	} else {
+	} else
 #endif
+	{
 		ast_string_field_set(cur, synopsis, synopsis);
 		ast_string_field_set(cur, description, description);
 #ifdef AST_XML_DOCS
 		cur->docsrc = AST_STATIC_DOC;
-	}
 #endif
+	}
 	if (ast_manager_register_struct(cur)) {
-		ast_free(cur);
+		ao2_t_ref(cur, -1, "action object registration failed");
 		return -1;
 	}
 
+	ao2_t_ref(cur, -1, "action object registration successful");
 	return 0;
 }
 /*! @}
@@ -5539,7 +5696,7 @@
 	char template[] = "/tmp/ast-http-XXXXXX";	/* template for temporary file */
 	struct ast_str *http_header = NULL, *out = NULL;
 	struct message m = { 0 };
-	unsigned int x;
+	unsigned int idx;
 	size_t hdrlen;
 
 	if (method != AST_HTTP_GET && method != AST_HTTP_HEAD && method != AST_HTTP_POST) {
@@ -5614,12 +5771,16 @@
 		params = ast_http_get_post_vars(ser, headers);
 	}
 
-	for (x = 0, v = params; v && (x < AST_MAX_MANHEADERS); x++, v = v->next) {
+	for (v = params; v && m.hdrcount < ARRAY_LEN(m.headers); v = v->next) {
 		hdrlen = strlen(v->name) + strlen(v->value) + 3;
-		m.headers[m.hdrcount] = alloca(hdrlen);
+		m.headers[m.hdrcount] = ast_malloc(hdrlen);
+		if (!m.headers[m.hdrcount]) {
+			/* Allocation failure */
+			continue;
+		}
 		snprintf((char *) m.headers[m.hdrcount], hdrlen, "%s: %s", v->name, v->value);
 		ast_debug(1, "HTTP Manager add header %s\n", m.headers[m.hdrcount]);
-		m.hdrcount = x + 1;
+		++m.hdrcount;
 	}
 
 	if (process_message(&s, &m)) {
@@ -5633,6 +5794,12 @@
 			}
 		}
 		session->needdestroy = 1;
+	}
+
+	/* Free request headers. */
+	for (idx = 0; idx < m.hdrcount; ++idx) {
+		ast_free((void *) m.headers[idx]);
+		m.headers[idx] = NULL;
 	}
 
 	ast_str_append(&http_header, 0,
@@ -5743,7 +5910,7 @@
 	struct ast_str *http_header = NULL, *out = NULL;
 	size_t result_size = 512;
 	struct message m = { 0 };
-	unsigned int x;
+	unsigned int idx;
 	size_t hdrlen;
 
 	time_t time_now = time(NULL);
@@ -5940,12 +6107,16 @@
 		params = ast_http_get_post_vars(ser, headers);
 	}
 
-	for (x = 0, v = params; v && (x < AST_MAX_MANHEADERS); x++, v = v->next) {
+	for (v = params; v && m.hdrcount < ARRAY_LEN(m.headers); v = v->next) {
 		hdrlen = strlen(v->name) + strlen(v->value) + 3;
-		m.headers[m.hdrcount] = alloca(hdrlen);
+		m.headers[m.hdrcount] = ast_malloc(hdrlen);
+		if (!m.headers[m.hdrcount]) {
+			/* Allocation failure */
+			continue;
+		}
 		snprintf((char *) m.headers[m.hdrcount], hdrlen, "%s: %s", v->name, v->value);
 		ast_verb(4, "HTTP Manager add header %s\n", m.headers[m.hdrcount]);
-		m.hdrcount = x + 1;
+		++m.hdrcount;
 	}
 
 	if (process_message(&s, &m)) {
@@ -5954,6 +6125,12 @@
 		}
 
 		session->needdestroy = 1;
+	}
+
+	/* Free request headers. */
+	for (idx = 0; idx < m.hdrcount; ++idx) {
+		ast_free((void *) m.headers[idx]);
+		m.headers[idx] = NULL;
 	}
 
 	if (s.f) {
@@ -6237,6 +6414,43 @@
 	AST_CLI_DEFINE(handle_manager_reload, "Reload manager configurations"),
 	AST_CLI_DEFINE(handle_manager_show_settings, "Show manager global settings"),
 };
+
+/*!
+ * \internal
+ * \brief Load the config channelvars variable.
+ *
+ * \param var Config variable to load.
+ *
+ * \return Nothing
+ */
+static void load_channelvars(struct ast_variable *var)
+{
+	struct manager_channel_variable *mcv;
+	char *remaining = ast_strdupa(var->value);
+	char *next;
+
+	ast_free(manager_channelvars);
+	manager_channelvars = ast_strdup(var->value);
+
+	/*
+	 * XXX TODO: To allow dialplan functions to have more than one
+	 * parameter requires eliminating the '|' as a separator so we
+	 * could use AST_STANDARD_APP_ARGS() to separate items.
+	 */
+	free_channelvars();
+	AST_RWLIST_WRLOCK(&channelvars);
+	while ((next = strsep(&remaining, ",|"))) {
+		if (!(mcv = ast_calloc(1, sizeof(*mcv) + strlen(next) + 1))) {
+			break;
+		}
+		strcpy(mcv->name, next); /* SAFE */
+		if (strchr(next, '(')) {
+			mcv->isfunc = 1;
+		}
+		AST_RWLIST_INSERT_TAIL(&channelvars, mcv, entry);
+	}
+	AST_RWLIST_UNLOCK(&channelvars);
+}
 
 static int __init_manager(int reload)
 {
@@ -6387,22 +6601,7 @@
 				authlimit = limit;
 			}
 		} else if (!strcasecmp(var->name, "channelvars")) {
-			struct manager_channel_variable *mcv;
-			char *remaining = ast_strdupa(val), *next;
-			ast_free(manager_channelvars);
-			manager_channelvars = ast_strdup(val);
-			AST_RWLIST_WRLOCK(&channelvars);
-			while ((next = strsep(&remaining, ",|"))) {
-				if (!(mcv = ast_calloc(1, sizeof(*mcv) + strlen(next) + 1))) {
-					break;
-				}
-				strcpy(mcv->name, next); /* SAFE */
-				if (strchr(next, '(')) {
-					mcv->isfunc = 1;
-				}
-				AST_RWLIST_INSERT_TAIL(&channelvars, mcv, entry);
-			}
-			AST_RWLIST_UNLOCK(&channelvars);
+			load_channelvars(var);
 		} else {
 			ast_log(LOG_NOTICE, "Invalid keyword <%s> = <%s> in manager.conf [general]\n",
 				var->name, val);




More information about the asterisk-commits mailing list