Fix deadlock when using dummy channels.

Dummy channels created by ast_dummy_channel_alloc() should be destoyed by
ast_channel_unref().  Using ast_channel_release() needlessly grabs the
channel container lock and can cause a deadlock as a result.

* Analyzed use of ast_dummy_channel_alloc() and made use
ast_channel_unref() when done with the dummy channel.  (Primary reason for
the reported deadlock.)

* Made app_dial.c:dial_exec_full() not call ast_call() holding any channel
locks.  Chan_local could not perform deadlock avoidance correctly.
(Potential deadlock exposed by this issue.  Secondary reason for the
reported deadlock since the held lock was part of the deadlock chain.)

* Fixed some uses of ast_dummy_channel_alloc() not checking the returned
channel pointer for failure.

* Fixed some potential chan=NULL pointer usage in func_odbc.c.  Protected
by testing the bogus_chan value.

* Fixed needlessly clearing a 1024 char auto array when setting the first
char to zero is enough in manager.c:action_getvar().

(closes issue ASTERISK-18613)
Reported by: Thomas Arimont
Patches:
      jira_asterisk_18613_v1.8.patch (license #5621) patch uploaded by rmudgett
Tested by: Thomas Arimont


git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.8@337973 65c4cc65-6c06-0410-ace0-fbb531ad65f3

apps/app_dial.c

@@ -2325,7 +2325,9 @@ static int dial_exec_full(struct ast_channel *chan, const char *data, struct ast
 			ast_copy_string(tc->exten, chan->exten, sizeof(tc->exten));
 
 		ast_channel_unlock(tc);
+		ast_channel_unlock(chan);
 		res = ast_call(tc, numsubst, 0); /* Place the call, but don't wait on the answer */
+		ast_channel_lock(chan);
 
 		/* Save the info in cdr's that we called them */
 		if (chan->cdr)

apps/app_minivm.c

@@ -1313,6 +1313,9 @@ static int sendmail(struct minivm_template *template, struct minivm_account *vmu
 	}
 	/* Allocate channel used for chanvar substitution */
 	ast = ast_dummy_channel_alloc();
+	if (!ast) {
+		return -1;
+	}
 
 	snprintf(dur, sizeof(dur), "%d:%02d", duration / 60, duration % 60);
 
@@ -1473,8 +1476,7 @@ static int sendmail(struct minivm_template *template, struct minivm_account *vmu
 	ast_safe_system(tmp2);
 	ast_debug(1, "Sent message to %s with command '%s' - %s\n", vmu->email, global_mailcmd, template->attachment ? "(media attachment)" : "");
 	ast_debug(3, "Actual command used: %s\n", tmp2);
-	if (ast)
-		ast = ast_channel_release(ast);
+	ast = ast_channel_unref(ast);
 	ast_free(str1);
 	ast_free(str2);
 	return 0;

apps/app_voicemail.c

@@ -4581,7 +4581,7 @@ static void make_email_file(FILE *p, char *srcemail, struct ast_vm_user *vmu, in
 			} else {
 				fprintf(p, "From: %s <%s>" ENDL, ast_str_quote(&str2, 0, ast_str_buffer(str1)), who);
 			}
-			ast = ast_channel_release(ast);
+			ast = ast_channel_unref(ast);
 		} else {
 			ast_log(AST_LOG_WARNING, "Cannot allocate the channel for variables substitution\n");
 		}
@@ -4626,7 +4626,7 @@ static void make_email_file(FILE *p, char *srcemail, struct ast_vm_user *vmu, in
 			} else {
 				fprintf(p, "Subject: %s" ENDL, ast_str_buffer(str1));
 			}
-			ast = ast_channel_release(ast);
+			ast = ast_channel_unref(ast);
 		} else {
 			ast_log(AST_LOG_WARNING, "Cannot allocate the channel for variables substitution\n");
 		}
@@ -4712,7 +4712,7 @@ static void make_email_file(FILE *p, char *srcemail, struct ast_vm_user *vmu, in
 #else
 			fprintf(p, "%s" ENDL, ast_str_buffer(str1));
 #endif
-			ast = ast_channel_release(ast);
+			ast = ast_channel_unref(ast);
 		} else {
 			ast_log(AST_LOG_WARNING, "Cannot allocate the channel for variables substitution\n");
 		}
@@ -4939,7 +4939,7 @@ static int sendpage(char *srcemail, char *pager, int msgnum, char *context, char
 			} else {
 				fprintf(p, "From: %s <%s>" ENDL, ast_str_quote(&str2, 0, ast_str_buffer(str1)), who);
 			}
-			ast = ast_channel_release(ast);
+			ast = ast_channel_unref(ast);
 		} else {
 			ast_log(AST_LOG_WARNING, "Cannot allocate the channel for variables substitution\n");
 		}
@@ -4983,7 +4983,7 @@ static int sendpage(char *srcemail, char *pager, int msgnum, char *context, char
 			} else {
 				fprintf(p, "Subject: %s" ENDL, ast_str_buffer(str1));
 			}
-			ast = ast_channel_release(ast);
+			ast = ast_channel_unref(ast);
 		} else {
 			ast_log(AST_LOG_WARNING, "Cannot allocate the channel for variables substitution\n");
 		}
@@ -5001,7 +5001,7 @@ static int sendpage(char *srcemail, char *pager, int msgnum, char *context, char
 			prep_email_sub_vars(ast, vmu, msgnum + 1, context, mailbox, fromfolder, cidnum, cidname, dur, date, category, flag);
 			ast_str_substitute_variables(&str1, 0, ast, pagerbody);
 			fprintf(p, "%s" ENDL, ast_str_buffer(str1));
-			ast = ast_channel_release(ast);
+			ast = ast_channel_unref(ast);
 		} else {
 			ast_log(AST_LOG_WARNING, "Cannot allocate the channel for variables substitution\n");
 		}
@@ -12699,6 +12699,9 @@ AST_TEST_DEFINE(test_voicemail_msgcount)
 		!(vmu = find_or_create(testcontext, testmailbox))) {
 		ast_test_status_update(test, "Cannot create vmu structure\n");
 		ast_unreplace_sigchld();
+#ifdef IMAP_STORAGE
+		chan = ast_channel_unref(chan);
+#endif
 		return AST_TEST_FAIL;
 	}
 
@@ -12718,6 +12721,9 @@ AST_TEST_DEFINE(test_voicemail_msgcount)
 				ast_test_status_update(test, "Unable to create test voicemail: %s\n",
 					syserr > 0 ? strerror(syserr) : "unable to fork()");
 				ast_unreplace_sigchld();
+#ifdef IMAP_STORAGE
+				chan = ast_channel_unref(chan);
+#endif
 				return AST_TEST_FAIL;
 			}
 		}
@@ -12791,7 +12797,7 @@ AST_TEST_DEFINE(test_voicemail_msgcount)
 	}
 
 #ifdef IMAP_STORAGE
-	chan = ast_channel_release(chan);
+	chan = ast_channel_unref(chan);
 #endif
 
 	/* And remove test directory */

cdr/cdr_custom.c

@@ -129,7 +129,6 @@ static int custom_log(struct ast_cdr *cdr)
 	}
 
 	dummy = ast_dummy_channel_alloc();
-
 	if (!dummy) {
 		ast_log(LOG_ERROR, "Unable to allocate channel for variable subsitution.\n");
 		return -1;
@@ -169,7 +168,7 @@ static int custom_log(struct ast_cdr *cdr)
 
 	AST_RWLIST_UNLOCK(&sinks);
 
-	ast_channel_release(dummy);
+	ast_channel_unref(dummy);
 
 	return 0;
 }

cdr/cdr_manager.c

@@ -167,7 +167,7 @@ static int manager_log(struct ast_cdr *cdr)
 		}
 		dummy->cdr = ast_cdr_dup(cdr);
 		pbx_substitute_variables_helper(dummy, ast_str_buffer(customfields), buf, sizeof(buf) - 1);
-		ast_channel_release(dummy);
+		ast_channel_unref(dummy);
 	}
 	ast_rwlock_unlock(&customfields_lock);
 

cdr/cdr_sqlite3_custom.c

@@ -261,7 +261,7 @@ static int write_cdr(struct ast_cdr *cdr)
 		}
 		sql = sqlite3_mprintf("INSERT INTO %q (%s) VALUES (%s)", table, columns, ast_str_buffer(value_string));
 		ast_debug(1, "About to log: %s\n", sql);
-		ast_channel_release(dummy);
+		ast_channel_unref(dummy);
 		ast_free(value_string);
 	}
 

cdr/cdr_syslog.c

@@ -116,7 +116,7 @@ static int syslog_log(struct ast_cdr *cdr)
 
 	AST_RWLIST_UNLOCK(&sinks);
 
-	ast_channel_release(dummy);
+	ast_channel_unref(dummy);
 
 	return 0;
 }

cel/cel_custom.c

@@ -128,7 +128,6 @@ static void custom_log(const struct ast_event *event, void *userdata)
 	}
 
 	dummy = ast_cel_fabricate_channel_from_event(event);
-
 	if (!dummy) {
 		ast_log(LOG_ERROR, "Unable to fabricate channel from CEL event.\n");
 		return;
@@ -163,7 +162,7 @@ static void custom_log(const struct ast_event *event, void *userdata)
 
 	AST_RWLIST_UNLOCK(&sinks);
 
-	ast_channel_release(dummy);
+	ast_channel_unref(dummy);
 }
 
 static int unload_module(void)

cel/cel_sqlite3_custom.c

@@ -262,7 +262,7 @@ static void write_cel(const struct ast_event *event, void *userdata)
 		}
 		sql = sqlite3_mprintf("INSERT INTO %q (%s) VALUES (%s)", table, columns, ast_str_buffer(value_string));
 		ast_debug(1, "About to log: %s\n", sql);
-		dummy = ast_channel_release(dummy);
+		dummy = ast_channel_unref(dummy);
 		ast_free(value_string);
 	}
 

funcs/func_odbc.c

@@ -267,7 +267,7 @@ static int acf_odbc_write(struct ast_channel *chan, const char *cmd, char *s, co
 			ast_autoservice_stop(chan);
 			pbx_builtin_setvar_helper(chan, "ODBCSTATUS", status);
 		} else {
-			ast_channel_release(chan);
+			ast_channel_unref(chan);
 		}
 		return -1;
 	}
@@ -292,7 +292,7 @@ static int acf_odbc_write(struct ast_channel *chan, const char *cmd, char *s, co
 	ast_str_substitute_variables(&insertbuf, 0, chan, query->sql_insert);
 
 	if (bogus_chan) {
-		chan = ast_channel_release(chan);
+		chan = ast_channel_unref(chan);
 	} else {
 		/* Restore prior values */
 		for (i = 0; i < args.argc; i++) {
@@ -474,7 +474,7 @@ static int acf_odbc_read(struct ast_channel *chan, const char *cmd, char *s, cha
 	ast_str_substitute_variables(&sql, 0, chan, query->sql_read);
 
 	if (bogus_chan) {
-		chan = ast_channel_release(chan);
+		chan = ast_channel_unref(chan);
 	} else {
 		/* Restore prior values */
 		for (x = 0; x < args.argc; x++) {
@@ -596,11 +596,9 @@ static int acf_odbc_read(struct ast_channel *chan, const char *cmd, char *s, cha
 				SQLFreeHandle(SQL_HANDLE_STMT, stmt);
 				ast_odbc_release_obj(obj);
 				obj = NULL;
-				pbx_builtin_setvar_helper(chan, "ODBCSTATUS", "MEMERROR");
-				if (chan)
+				if (!bogus_chan) {
+					pbx_builtin_setvar_helper(chan, "ODBCSTATUS", "MEMERROR");
 					ast_autoservice_stop(chan);
-				if (bogus_chan) {
-					ast_channel_release(chan);
 				}
 				return -1;
 			}
@@ -631,9 +629,11 @@ static int acf_odbc_read(struct ast_channel *chan, const char *cmd, char *s, cha
 						SQLFreeHandle(SQL_HANDLE_STMT, stmt);
 						ast_odbc_release_obj(obj);
 						obj = NULL;
-						pbx_builtin_setvar_helper(chan, "ODBCROWS", rowcount);
-						pbx_builtin_setvar_helper(chan, "ODBCSTATUS", "MEMERROR");
-						ast_autoservice_stop(chan);
+						if (!bogus_chan) {
+							pbx_builtin_setvar_helper(chan, "ODBCROWS", rowcount);
+							pbx_builtin_setvar_helper(chan, "ODBCSTATUS", "MEMERROR");
+							ast_autoservice_stop(chan);
+						}
 						return -1;
 					}
 					resultset = tmp;
@@ -1142,6 +1142,10 @@ static char *cli_odbc_read(struct ast_cli_entry *e, int cmd, struct ast_cli_args
 	char_args = ast_strdupa(a->argv[3]);
 
 	chan = ast_dummy_channel_alloc();
+	if (!chan) {
+		AST_RWLIST_UNLOCK(&queries);
+		return CLI_FAILURE;
+	}
 
 	AST_STANDARD_APP_ARGS(args, char_args);
 	for (i = 0; i < args.argc; i++) {
@@ -1150,7 +1154,7 @@ static char *cli_odbc_read(struct ast_cli_entry *e, int cmd, struct ast_cli_args
 	}
 
 	ast_str_substitute_variables(&sql, 0, chan, query->sql_read);
-	chan = ast_channel_release(chan);
+	chan = ast_channel_unref(chan);
 
 	if (a->argc == 5 && !strcmp(a->argv[4], "exec")) {
 		/* Execute the query */
@@ -1352,6 +1356,10 @@ static char *cli_odbc_write(struct ast_cli_entry *e, int cmd, struct ast_cli_arg
 	char_values = ast_strdupa(a->argv[4]);
 
 	chan = ast_dummy_channel_alloc();
+	if (!chan) {
+		AST_RWLIST_UNLOCK(&queries);
+		return CLI_FAILURE;
+	}
 
 	AST_STANDARD_APP_ARGS(args, char_args);
 	for (i = 0; i < args.argc; i++) {
@@ -1370,7 +1378,8 @@ static char *cli_odbc_write(struct ast_cli_entry *e, int cmd, struct ast_cli_arg
 	pbx_builtin_pushvar_helper(chan, "VALUE", S_OR(a->argv[4], ""));
 	ast_str_substitute_variables(&sql, 0, chan, query->sql_write);
 	ast_debug(1, "SQL is %s\n", ast_str_buffer(sql));
-	chan = ast_channel_release(chan);
+
+	chan = ast_channel_unref(chan);
 
 	if (a->argc == 6 && !strcmp(a->argv[5], "exec")) {
 		/* Execute the query */

include/asterisk/cel.h

@@ -183,8 +183,11 @@ void ast_cel_check_retire_linkedid(struct ast_channel *chan);
 /*!
  * \brief Create a fake channel from data in a CEL event
  *
- * This function creates a fake channel containing the serialized channel data 
- * in the given cel event.  It must be released with ast_channel_release.
+ * \note
+ * This function creates a fake channel containing the
+ * serialized channel data in the given cel event.  It should be
+ * released with ast_channel_unref() but could be released with
+ * ast_channel_release().
  *
  * \param event the CEL event
  *

include/asterisk/channel.h

@@ -1130,6 +1130,7 @@ struct ast_channel * attribute_malloc __attribute__((format(printf, 13, 14)))
 	__ast_channel_alloc(needqueue, state, cid_num, cid_name, acctcode, exten, context, linkedid, amaflag, \
 			    __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__)
 
+#if defined(REF_DEBUG) || defined(__AST_DEBUG_MALLOC)
 /*!
  * \brief Create a fake channel structure
  *
@@ -1139,11 +1140,32 @@ struct ast_channel * attribute_malloc __attribute__((format(printf, 13, 14)))
  * \note This function should ONLY be used to create a fake channel
  *       that can then be populated with data for use in variable
  *       substitution when a real channel does not exist.
+ *
+ * \note The created dummy channel should be destroyed by
+ * ast_channel_unref().  Using ast_channel_release() needlessly
+ * grabs the channel container lock and can cause a deadlock as
+ * a result.  Also grabbing the channel container lock reduces
+ * system performance.
  */
-#if defined(REF_DEBUG) || defined(__AST_DEBUG_MALLOC)
 #define ast_dummy_channel_alloc()	__ast_dummy_channel_alloc(__FILE__, __LINE__, __PRETTY_FUNCTION__)
 struct ast_channel *__ast_dummy_channel_alloc(const char *file, int line, const char *function);
 #else
+/*!
+ * \brief Create a fake channel structure
+ *
+ * \retval NULL failure
+ * \retval non-NULL successfully allocated channel
+ *
+ * \note This function should ONLY be used to create a fake channel
+ *       that can then be populated with data for use in variable
+ *       substitution when a real channel does not exist.
+ *
+ * \note The created dummy channel should be destroyed by
+ * ast_channel_unref().  Using ast_channel_release() needlessly
+ * grabs the channel container lock and can cause a deadlock as
+ * a result.  Also grabbing the channel container lock reduces
+ * system performance.
+ */
 struct ast_channel *ast_dummy_channel_alloc(void);
 #endif
 
@@ -1589,6 +1611,7 @@ int __ast_answer(struct ast_channel *chan, unsigned int delay, int cdr_answer);
 
 /*!
  * \brief Make a call
+ * \note Absolutely _NO_ channel locks should be held before calling this function.
  * \param chan which channel to make the call on
  * \param addr destination of the call
  * \param timeout time to wait on for connect

main/cel.c

@@ -417,7 +417,7 @@ struct ast_channel *ast_cel_fabricate_channel_from_event(const struct ast_event
 
 	/* first, get the variables from the event */
 	if (ast_cel_fill_record(event, &record)) {
-		ast_channel_release(tchan);
+		ast_channel_unref(tchan);
 		return NULL;
 	}
 

main/logger.c

@@ -646,12 +646,15 @@ static int rotate_file(const char *filename)
 	if (!ast_strlen_zero(exec_after_rotate)) {
 		struct ast_channel *c = ast_dummy_channel_alloc();
 		char buf[512];
+
 		pbx_builtin_setvar_helper(c, "filename", filename);
 		pbx_substitute_variables_helper(c, exec_after_rotate, buf, sizeof(buf));
+		if (c) {
+			c = ast_channel_unref(c);
+		}
 		if (ast_safe_system(buf) == -1) {
 			ast_log(LOG_WARNING, "error executing '%s'\n", buf);
 		}
-		c = ast_channel_release(c);
 	}
 	return res;
 }

main/manager.c

@@ -3078,7 +3078,7 @@ static int action_getvar(struct mansession *s, const struct message *m)
 	const char *name = astman_get_header(m, "Channel");
 	const char *varname = astman_get_header(m, "Variable");
 	char *varval;
-	char workspace[1024] = "";
+	char workspace[1024];
 
 	if (ast_strlen_zero(varname)) {
 		astman_send_error(s, m, "No variable specified");
@@ -3092,12 +3092,12 @@ static int action_getvar(struct mansession *s, const struct message *m)
 		}
 	}
 
+	workspace[0] = '\0';
 	if (varname[strlen(varname) - 1] == ')') {
 		if (!c) {
 			c = ast_dummy_channel_alloc();
 			if (c) {
 				ast_func_read(c, (char *) varname, workspace, sizeof(workspace));
-				c = ast_channel_release(c);
 			} else
 				ast_log(LOG_ERROR, "Unable to allocate bogus channel for variable substitution.  Function results may be blank.\n");
 		} else {

main/pbx.c

@@ -3721,7 +3721,7 @@ void ast_str_substitute_variables_full(struct ast_str **buf, ssize_t maxlen, str
 						cp4 = ast_func_read2(c, finalvars, &substr3, 0) ? NULL : ast_str_buffer(substr3);
 						/* Don't deallocate the varshead that was passed in */
 						memcpy(&bogus->varshead, &old, sizeof(bogus->varshead));
-						ast_channel_release(bogus);
+						ast_channel_unref(bogus);
 					} else {
 						ast_log(LOG_ERROR, "Unable to allocate bogus channel for variable substitution.  Function results may be blank.\n");
 					}
@@ -3920,7 +3920,7 @@ void pbx_substitute_variables_helper_full(struct ast_channel *c, struct varshead
 						cp4 = ast_func_read(c, vars, workspace, VAR_BUF_SIZE) ? NULL : workspace;
 						/* Don't deallocate the varshead that was passed in */
 						memcpy(&c->varshead, &old, sizeof(c->varshead));
-						c = ast_channel_release(c);
+						c = ast_channel_unref(c);
 					} else {
 						ast_log(LOG_ERROR, "Unable to allocate bogus channel for variable substitution.  Function results may be blank.\n");
 					}
@@ -8321,12 +8321,16 @@ static int ast_add_extension2_lockopt(struct ast_context *con,
 	/* If we are adding a hint evalulate in variables and global variables */
 	if (priority == PRIORITY_HINT && strstr(application, "${") && !strstr(extension, "_")) {
 		struct ast_channel *c = ast_dummy_channel_alloc();
-		ast_copy_string(c->exten, extension, sizeof(c->exten));
-		ast_copy_string(c->context, con->name, sizeof(c->context));
 
+		if (c) {
+			ast_copy_string(c->exten, extension, sizeof(c->exten));
+			ast_copy_string(c->context, con->name, sizeof(c->context));
+		}
 		pbx_substitute_variables_helper(c, application, expand_buf, sizeof(expand_buf));
 		application = expand_buf;
-		ast_channel_release(c);
+		if (c) {
+			ast_channel_unref(c);
+		}
 	}
 
 	length = sizeof(struct ast_exten);
@@ -8578,7 +8582,7 @@ static int ast_pbx_outgoing_cdr_failed(void)
 	chan->cdr = ast_cdr_alloc();
 	if (!chan->cdr) {
 		/* allocation of the cdr failed */
-		chan = ast_channel_release(chan);   /* free the channel */
+		chan = ast_channel_unref(chan);   /* free the channel */
 		return -1;                /* return failure */
 	}
 
@@ -8589,7 +8593,7 @@ static int ast_pbx_outgoing_cdr_failed(void)
 	ast_cdr_failed(chan->cdr);      /* set the status to failed */
 	ast_cdr_detach(chan->cdr);      /* post and free the record */
 	chan->cdr = NULL;
-	chan = ast_channel_release(chan);         /* free the channel */
+	chan = ast_channel_unref(chan);         /* free the channel */
 
 	return 0;  /* success */
 }

tests/test_gosub.c

@@ -102,7 +102,7 @@ AST_TEST_DEFINE(test_gosub)
 
 	if (!(str = ast_str_create(16))) {
 		ast_test_status_update(test, "Unable to allocate dynamic string buffer\n");
-		ast_channel_release(chan);
+		ast_channel_unref(chan);
 		return AST_TEST_FAIL;
 	}
 
@@ -110,7 +110,7 @@ AST_TEST_DEFINE(test_gosub)
 	if (!(con = ast_context_find_or_create(NULL, NULL, "tests_test_gosub_virtual_context", "test_gosub"))) {
 		ast_test_status_update(test, "Unable to create test dialplan context");
 		ast_free(str);
-		ast_channel_release(chan);
+		ast_channel_unref(chan);
 		return AST_TEST_FAIL;
 	}
 
@@ -144,7 +144,7 @@ AST_TEST_DEFINE(test_gosub)
 	}
 
 	ast_free(str);
-	ast_channel_release(chan);
+	ast_channel_unref(chan);
 	ast_context_remove_extension2(con, "s", 1, NULL, 0);
 	ast_context_destroy(con, "test_gosub");