From: Richard Mudgett Date: Mon, 26 Sep 2011 19:30:39 +0000 (+0000) Subject: Fix deadlock when using dummy channels. X-Git-Tag: 1.8.8.0-rc1~32 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f2e164043566d934c5e7da4f9ce0feb2728a7626;p=thirdparty%2Fasterisk.git 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 --- diff --git a/apps/app_dial.c b/apps/app_dial.c index 6bea10fc53..a9d1079a13 100644 --- a/apps/app_dial.c +++ b/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) diff --git a/apps/app_minivm.c b/apps/app_minivm.c index 993a5a366b..edffd4d3f4 100644 --- a/apps/app_minivm.c +++ b/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; diff --git a/apps/app_voicemail.c b/apps/app_voicemail.c index 9465cf2c6f..ef5a13eb3e 100644 --- a/apps/app_voicemail.c +++ b/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 */ diff --git a/cdr/cdr_custom.c b/cdr/cdr_custom.c index 6a2751d1f2..ee423d3e63 100644 --- a/cdr/cdr_custom.c +++ b/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; } diff --git a/cdr/cdr_manager.c b/cdr/cdr_manager.c index d5106b1588..1d9eb6a6ab 100644 --- a/cdr/cdr_manager.c +++ b/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); diff --git a/cdr/cdr_sqlite3_custom.c b/cdr/cdr_sqlite3_custom.c index 97d1a309ec..65ef5b8a2a 100644 --- a/cdr/cdr_sqlite3_custom.c +++ b/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); } diff --git a/cdr/cdr_syslog.c b/cdr/cdr_syslog.c index b6fc90e0dd..adf260594b 100644 --- a/cdr/cdr_syslog.c +++ b/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; } diff --git a/cel/cel_custom.c b/cel/cel_custom.c index 54b1e4f5ed..8714087163 100644 --- a/cel/cel_custom.c +++ b/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) diff --git a/cel/cel_sqlite3_custom.c b/cel/cel_sqlite3_custom.c index 2906ed9ee9..4596e770e7 100644 --- a/cel/cel_sqlite3_custom.c +++ b/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); } diff --git a/funcs/func_odbc.c b/funcs/func_odbc.c index e940021a35..eeb7d3be16 100644 --- a/funcs/func_odbc.c +++ b/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 */ diff --git a/include/asterisk/cel.h b/include/asterisk/cel.h index 353c061105..a0f82ca57b 100644 --- a/include/asterisk/cel.h +++ b/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 * diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h index 55482db855..02d1141852 100644 --- a/include/asterisk/channel.h +++ b/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 diff --git a/main/cel.c b/main/cel.c index 7f5c24f404..ef37a1196a 100644 --- a/main/cel.c +++ b/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; } diff --git a/main/logger.c b/main/logger.c index 017edca09a..60fd879a51 100644 --- a/main/logger.c +++ b/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; } diff --git a/main/manager.c b/main/manager.c index 357a85c3aa..4746409e83 100644 --- a/main/manager.c +++ b/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 { diff --git a/main/pbx.c b/main/pbx.c index ac6f026cdc..4745eaed51 100644 --- a/main/pbx.c +++ b/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 */ } diff --git a/tests/test_gosub.c b/tests/test_gosub.c index a02441e57a..e65ebbcbb9 100644 --- a/tests/test_gosub.c +++ b/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");