From 34002d567b5f3019f0bbd3bd5cd6026473727d98 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Mon, 5 Nov 2007 22:07:54 +0000 Subject: [PATCH] After seeing crashes related to channel variables, I went looking around at the ways that channel variables are handled. In general, they were not handled in a thread-safe way. The channel _must_ be locked when reading or writing from/to the channel variable list. What I have done to improve this situation is to make pbx_builtin_setvar_helper() and friends lock the channel when doing their thing. Asterisk API calls almost all lock the channel for you as necessary, but this family of functions did not. (closes issue #10923, reported by atis) (closes issue #11159, reported by 850t) git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.4@88805 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- include/asterisk/pbx.h | 24 +++++++++++++++++++++ main/pbx.c | 48 ++++++++++++++++++++++++++++++++---------- 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/include/asterisk/pbx.h b/include/asterisk/pbx.h index c558289c7c..699aa50826 100644 --- a/include/asterisk/pbx.h +++ b/include/asterisk/pbx.h @@ -764,13 +764,37 @@ struct ast_ignorepat *ast_walk_context_ignorepats(struct ast_context *con, struct ast_ignorepat *ip); struct ast_sw *ast_walk_context_switches(struct ast_context *con, struct ast_sw *sw); +/*! + * \note Will lock the channel. + */ int pbx_builtin_serialize_variables(struct ast_channel *chan, char *buf, size_t size); + +/*! + * \note Will lock the channel. + */ const char *pbx_builtin_getvar_helper(struct ast_channel *chan, const char *name); + +/*! + * \note Will lock the channel. + */ void pbx_builtin_pushvar_helper(struct ast_channel *chan, const char *name, const char *value); + +/*! + * \note Will lock the channel. + */ void pbx_builtin_setvar_helper(struct ast_channel *chan, const char *name, const char *value); + +/*! + * \note Will lock the channel. + */ void pbx_retrieve_variable(struct ast_channel *c, const char *var, char **ret, char *workspace, int workspacelen, struct varshead *headp); void pbx_builtin_clear_globals(void); + +/*! + * \note Will lock the channel. + */ int pbx_builtin_setvar(struct ast_channel *chan, void *data); + void pbx_substitute_variables_helper(struct ast_channel *c,const char *cp1,char *cp2,int count); void pbx_substitute_variables_varshead(struct varshead *headp, const char *cp1, char *cp2, int count); diff --git a/main/pbx.c b/main/pbx.c index 120cfa7d4a..6138684a48 100644 --- a/main/pbx.c +++ b/main/pbx.c @@ -1135,6 +1135,7 @@ void pbx_retrieve_variable(struct ast_channel *c, const char *var, char **ret, c struct varshead *places[2] = { headp, &globals }; /* list of places where we may look */ if (c) { + ast_channel_lock(c); places[0] = &c->varshead; } /* @@ -1232,6 +1233,9 @@ void pbx_retrieve_variable(struct ast_channel *c, const char *var, char **ret, c if (need_substring) *ret = substring(*ret, offset, length, workspace, workspacelen); } + + if (c) + ast_channel_unlock(c); } /*! \brief CLI function to show installed custom functions @@ -5722,6 +5726,8 @@ int pbx_builtin_serialize_variables(struct ast_channel *chan, char *buf, size_t memset(buf, 0, size); + ast_channel_lock(chan); + AST_LIST_TRAVERSE(&chan->varshead, variables, entries) { if ((var=ast_var_name(variables)) && (val=ast_var_value(variables)) /* && !ast_strlen_zero(var) && !ast_strlen_zero(val) */ @@ -5735,6 +5741,8 @@ int pbx_builtin_serialize_variables(struct ast_channel *chan, char *buf, size_t break; } + ast_channel_unlock(chan); + return total; } @@ -5747,8 +5755,11 @@ const char *pbx_builtin_getvar_helper(struct ast_channel *chan, const char *name if (!name) return NULL; - if (chan) + + if (chan) { + ast_channel_lock(chan); places[0] = &chan->varshead; + } for (i = 0; i < 2; i++) { if (!places[i]) @@ -5767,6 +5778,9 @@ const char *pbx_builtin_getvar_helper(struct ast_channel *chan, const char *name break; } + if (chan) + ast_channel_unlock(chan); + return ret; } @@ -5783,18 +5797,25 @@ void pbx_builtin_pushvar_helper(struct ast_channel *chan, const char *name, cons return; } - headp = (chan) ? &chan->varshead : &globals; + if (chan) { + ast_channel_lock(chan); + headp = &chan->varshead; + } else { + ast_mutex_lock(&globalslock); + headp = &globals; + } if (value) { if ((option_verbose > 1) && (headp == &globals)) ast_verbose(VERBOSE_PREFIX_2 "Setting global variable '%s' to '%s'\n", name, value); newvariable = ast_var_assign(name, value); - if (headp == &globals) - ast_mutex_lock(&globalslock); AST_LIST_INSERT_HEAD(headp, newvariable, entries); - if (headp == &globals) - ast_mutex_unlock(&globalslock); } + + if (chan) + ast_channel_unlock(chan); + else + ast_mutex_unlock(&globalslock); } void pbx_builtin_setvar_helper(struct ast_channel *chan, const char *name, const char *value) @@ -5803,7 +5824,6 @@ void pbx_builtin_setvar_helper(struct ast_channel *chan, const char *name, const struct varshead *headp; const char *nametail = name; - /* XXX may need locking on the channel ? */ if (name[strlen(name)-1] == ')') { char *function = ast_strdupa(name); @@ -5811,7 +5831,13 @@ void pbx_builtin_setvar_helper(struct ast_channel *chan, const char *name, const return; } - headp = (chan) ? &chan->varshead : &globals; + if (chan) { + ast_channel_lock(chan); + headp = &chan->varshead; + } else { + ast_mutex_lock(&globalslock); + headp = &globals; + } /* For comparison purposes, we have to strip leading underscores */ if (*nametail == '_') { @@ -5820,8 +5846,6 @@ void pbx_builtin_setvar_helper(struct ast_channel *chan, const char *name, const nametail++; } - if (headp == &globals) - ast_mutex_lock(&globalslock); AST_LIST_TRAVERSE (headp, newvariable, entries) { if (strcasecmp(ast_var_name(newvariable), nametail) == 0) { /* there is already such a variable, delete it */ @@ -5838,7 +5862,9 @@ void pbx_builtin_setvar_helper(struct ast_channel *chan, const char *name, const AST_LIST_INSERT_HEAD(headp, newvariable, entries); } - if (headp == &globals) + if (chan) + ast_channel_unlock(chan); + else ast_mutex_unlock(&globalslock); }