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

diff --git a/include/asterisk/pbx.h b/include/asterisk/pbx.h
index c558289c7cca8bf06a6eacdf5366383079d9b165..699aa50826f9ce1171f3ca481d4f1443ce103123 100644 (file)
--- 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 120cfa7d4acb879dc6d48c8fa5ce7a98b8234351..6138684a48837224e01f9b38e731cd2312e74447 100644 (file)
--- 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);
 }