Fix crash during CDR update.

The ast_cdr_setcid() and ast_cdr_update() were shown in ASTERISK-18836 to
be called by different threads for the same channel.  The channel driver
thread and the PBX thread running dialplan.

* Add lock protection around CDR API calls that access an ast_channel
pointer.

(closes issue ASTERISK-18836)
Reported by: gpluser

Review: https://reviewboard.asterisk.org/r/1628/

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

diff --git a/apps/app_authenticate.c b/apps/app_authenticate.c
index 540e3df1cd4f94a7c17368259244595ff6261380..3bdf483906c241058261cfad470b16bb71b02921 100644 (file)
--- a/apps/app_authenticate.c
+++ b/apps/app_authenticate.c
@@ -213,14 +213,20 @@ static int auth_exec(struct ast_channel *chan, const char *data)
                                                continue;
                                        ast_md5_hash(md5passwd, passwd);
                                        if (!strcmp(md5passwd, md5secret)) {
-                                               if (ast_test_flag(&flags,OPT_ACCOUNT))
+                                               if (ast_test_flag(&flags,OPT_ACCOUNT)) {
+                                                       ast_channel_lock(chan);
                                                        ast_cdr_setaccount(chan, buf);
+                                                       ast_channel_unlock(chan);
+                                               }
                                                break;
                                        }
                                } else {
                                        if (!strcmp(passwd, buf)) {
-                                               if (ast_test_flag(&flags, OPT_ACCOUNT))
+                                               if (ast_test_flag(&flags, OPT_ACCOUNT)) {
+                                                       ast_channel_lock(chan);
                                                        ast_cdr_setaccount(chan, buf);
+                                                       ast_channel_unlock(chan);
+                                               }
                                                break;
                                        }
                                }
@@ -242,8 +248,11 @@ static int auth_exec(struct ast_channel *chan, const char *data)
        }
 
        if ((retries < 3) && !res) {
-               if (ast_test_flag(&flags,OPT_ACCOUNT) && !ast_test_flag(&flags,OPT_MULTIPLE))
+               if (ast_test_flag(&flags,OPT_ACCOUNT) && !ast_test_flag(&flags,OPT_MULTIPLE)) {
+                       ast_channel_lock(chan);
                        ast_cdr_setaccount(chan, passwd);
+                       ast_channel_unlock(chan);
+               }
                if (!(res = ast_streamfile(chan, "auth-thankyou", chan->language)))
                        res = ast_waitstream(chan, "");
        } else {

diff --git a/apps/app_followme.c b/apps/app_followme.c
index 0c83047b7a2bb2a694761eb28f840bef3f788b9e..992e2c6f8766f5cd438b888565b0b916cc33a8a4 100644 (file)
--- a/apps/app_followme.c
+++ b/apps/app_followme.c
@@ -478,10 +478,12 @@ static void clear_caller(struct findme_user *tmpuser)
 
        if (tmpuser && tmpuser->ochan && tmpuser->state >= 0) {
                outbound = tmpuser->ochan;
+               ast_channel_lock(outbound);
                if (!outbound->cdr) {
                        outbound->cdr = ast_cdr_alloc();
-                       if (outbound->cdr)
+                       if (outbound->cdr) {
                                ast_cdr_init(outbound->cdr, outbound);
+                       }
                }
                if (outbound->cdr) {
                        char tmp[256];
@@ -492,11 +494,15 @@ static void clear_caller(struct findme_user *tmpuser)
                        ast_cdr_start(outbound->cdr);
                        ast_cdr_end(outbound->cdr);
                        /* If the cause wasn't handled properly */
-                       if (ast_cdr_disposition(outbound->cdr, outbound->hangupcause))
+                       if (ast_cdr_disposition(outbound->cdr, outbound->hangupcause)) {
                                ast_cdr_failed(outbound->cdr);
-               } else
+                       }
+               } else {
                        ast_log(LOG_WARNING, "Unable to create Call Detail Record\n");
-               ast_hangup(tmpuser->ochan);
+               }
+               ast_channel_unlock(outbound);
+               ast_hangup(outbound);
+               tmpuser->ochan = NULL;
        }
 }
 
@@ -876,6 +882,7 @@ static void findmeexec(struct fm_args *tpargs)
                                        AST_LIST_INSERT_TAIL(findme_user_list, tmpuser, entry);
                                } else {
                                        ast_verb(3, "couldn't reach at this number.\n");
+                                       ast_channel_lock(outbound);
                                        if (!outbound->cdr) {
                                                outbound->cdr = ast_cdr_alloc();
                                        }
@@ -895,6 +902,7 @@ static void findmeexec(struct fm_args *tpargs)
                                        } else {
                                                ast_log(LOG_ERROR, "Unable to create Call Detail Record\n");
                                        }
+                                       ast_channel_unlock(outbound);
                                        ast_hangup(outbound);
                                        ast_free(tmpuser);
                                }

diff --git a/apps/app_queue.c b/apps/app_queue.c
index 1349d04b2f484d7797afb407e7d9e26652b9b4f2..4db586e290bf983dbae8fe5dc4cff6ed2dc21c90 100644 (file)
--- a/apps/app_queue.c
+++ b/apps/app_queue.c
@@ -4988,9 +4988,9 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
                        if ((strcasecmp(cdr->uniqueid, qe->chan->uniqueid)) &&
                            (strcasecmp(cdr->linkedid, qe->chan->uniqueid)) &&
                            (newcdr = ast_cdr_dup(cdr))) {
+                               ast_channel_lock(qe->chan);
                                ast_cdr_init(newcdr, qe->chan);
                                ast_cdr_reset(newcdr, 0);
-                               ast_channel_lock(qe->chan);
                                cdr = ast_cdr_append(cdr, newcdr);
                                cdr = cdr->next;
                                ast_channel_unlock(qe->chan);

diff --git a/funcs/func_cdr.c b/funcs/func_cdr.c
index 34fdd6736eb3b070c284df6d0c4186b537da8cb6..dc710965984d55c071e0e0a50cda5e3eb122568d 100644 (file)
--- a/funcs/func_cdr.c
+++ b/funcs/func_cdr.c
@@ -197,17 +197,21 @@ static int cdr_read(struct ast_channel *chan, const char *cmd, char *parse,
 {
        char *ret;
        struct ast_flags flags = { 0 };
-       struct ast_cdr *cdr = chan ? chan->cdr : NULL;
+       struct ast_cdr *cdr;
        AST_DECLARE_APP_ARGS(args,
                             AST_APP_ARG(variable);
                             AST_APP_ARG(options);
        );
 
-       if (ast_strlen_zero(parse))
+       if (ast_strlen_zero(parse) || !chan)
                return -1;
 
-       if (!cdr)
+       ast_channel_lock(chan);
+       cdr = chan->cdr;
+       if (!cdr) {
+               ast_channel_unlock(chan);
                return -1;
+       }
 
        AST_STANDARD_APP_ARGS(args, parse);
 
@@ -255,13 +259,14 @@ static int cdr_read(struct ast_channel *chan, const char *cmd, char *parse,
                                   ast_test_flag(&flags, OPT_UNPARSED));
        }
 
+       ast_channel_unlock(chan);
        return ret ? 0 : -1;
 }
 
 static int cdr_write(struct ast_channel *chan, const char *cmd, char *parse,
                     const char *value)
 {
-       struct ast_cdr *cdr = chan ? chan->cdr : NULL;
+       struct ast_cdr *cdr;
        struct ast_flags flags = { 0 };
        AST_DECLARE_APP_ARGS(args,
                             AST_APP_ARG(variable);
@@ -271,8 +276,12 @@ static int cdr_write(struct ast_channel *chan, const char *cmd, char *parse,
        if (ast_strlen_zero(parse) || !value || !chan)
                return -1;
 
-       if (!cdr)
+       ast_channel_lock(chan);
+       cdr = chan->cdr;
+       if (!cdr) {
+               ast_channel_unlock(chan);
                return -1;
+       }
 
        AST_STANDARD_APP_ARGS(args, parse);
 
@@ -296,6 +305,7 @@ static int cdr_write(struct ast_channel *chan, const char *cmd, char *parse,
                /* No need to worry about the u flag, as all fields for which setting
                 * 'u' would do anything are marked as readonly. */
 
+       ast_channel_unlock(chan);
        return 0;
 }
 

diff --git a/include/asterisk/cdr.h b/include/asterisk/cdr.h
index 5442fdb0cf4dd5431717d4ebbc136f60e6abc6bc..45fb3ed798746c92ff2d6236a1f04b4961779c96 100644 (file)
--- a/include/asterisk/cdr.h
+++ b/include/asterisk/cdr.h
@@ -211,6 +211,7 @@ void ast_cdr_discard(struct ast_cdr *cdr);
  * \param cdr Call Detail Record to use for channel
  * \param chan Channel to bind CDR with
  * Initializes a CDR and associates it with a particular channel
+ * \note The channel should be locked before calling.
  * \return 0 by default
  */
 int ast_cdr_init(struct ast_cdr *cdr, struct ast_channel *chan);
@@ -220,6 +221,7 @@ int ast_cdr_init(struct ast_cdr *cdr, struct ast_channel *chan);
  * \param cdr Call Detail Record to use for channel
  * \param chan Channel to bind CDR with
  * Initializes a CDR and associates it with a particular channel
+ * \note The channel should be locked before calling.
  * \return 0 by default
  */
 int ast_cdr_setcid(struct ast_cdr *cdr, struct ast_channel *chan);
@@ -403,22 +405,40 @@ char *ast_cdr_flags2str(int flags);
  */
 void ast_cdr_merge(struct ast_cdr *to, struct ast_cdr *from);
 
-/*! \brief Set account code, will generate AMI event */
+/*!
+ * \brief Set account code, will generate AMI event
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_setaccount(struct ast_channel *chan, const char *account);
 
-/*! \brief Set the peer account */
+/*!
+ * \brief Set the peer account
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_setpeeraccount(struct ast_channel *chan, const char *account);
 
-/*! \brief Set AMA flags for channel */
+/*!
+ * \brief Set AMA flags for channel
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_setamaflags(struct ast_channel *chan, const char *amaflags);
 
-/*! \brief Set CDR user field for channel (stored in CDR) */
+/*!
+ * \brief Set CDR user field for channel (stored in CDR)
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_setuserfield(struct ast_channel *chan, const char *userfield);
-/*! \brief Append to CDR user field for channel (stored in CDR) */
+/*!
+ * \brief Append to CDR user field for channel (stored in CDR)
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_appenduserfield(struct ast_channel *chan, const char *userfield);
 
 
-/*! Update CDR on a channel */
+/*!
+ * \brief Update CDR on a channel
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_update(struct ast_channel *chan);
 
 

diff --git a/main/channel.c b/main/channel.c
index 4499be8ed7c710790d9c397cefd70ddbe7173d70..b8e7e38e02c54ad8e1ae1cacc582703c30fad52d 100644 (file)
--- a/main/channel.c
+++ b/main/channel.c
@@ -5242,7 +5242,9 @@ struct ast_channel *ast_call_forward(struct ast_channel *caller, struct ast_chan
                        ast_channel_datastore_inherit(oh->parent_channel, new);
                }
                if (oh->account) {
+                       ast_channel_lock(new);
                        ast_cdr_setaccount(new, oh->account);
+                       ast_channel_lock(new);
                }
        } else if (caller) { /* no outgoing helper so use caller if avaliable */
                ast_channel_update_redirecting(caller, apr, NULL);
@@ -5308,8 +5310,11 @@ struct ast_channel *__ast_request_and_dial(const char *type, format_t format, co
                        ast_channel_inherit_variables(oh->parent_channel, chan);
                        ast_channel_datastore_inherit(oh->parent_channel, chan);
                }
-               if (oh->account)
-                       ast_cdr_setaccount(chan, oh->account);  
+               if (oh->account) {
+                       ast_channel_lock(chan);
+                       ast_cdr_setaccount(chan, oh->account);
+                       ast_channel_unlock(chan);
+               }
        }
 
        ast_set_callerid(chan, cid_num, cid_name, cid_num);
@@ -5421,21 +5426,27 @@ struct ast_channel *__ast_request_and_dial(const char *type, format_t format, co
                *outstate = AST_CONTROL_ANSWER;
 
        if (res <= 0) {
-               if ( AST_CONTROL_RINGING == last_subclass ) 
+               ast_channel_lock(chan);
+               if (AST_CONTROL_RINGING == last_subclass) {
                        chan->hangupcause = AST_CAUSE_NO_ANSWER;
-               if (!chan->cdr && (chan->cdr = ast_cdr_alloc()))
+               }
+               if (!chan->cdr && (chan->cdr = ast_cdr_alloc())) {
                        ast_cdr_init(chan->cdr, chan);
+               }
                if (chan->cdr) {
                        char tmp[256];
+
                        snprintf(tmp, sizeof(tmp), "%s/%s", type, (char *)data);
-                       ast_cdr_setapp(chan->cdr,"Dial",tmp);
+                       ast_cdr_setapp(chan->cdr, "Dial", tmp);
                        ast_cdr_update(chan);
                        ast_cdr_start(chan->cdr);
                        ast_cdr_end(chan->cdr);
                        /* If the cause wasn't handled properly */
-                       if (ast_cdr_disposition(chan->cdr,chan->hangupcause))
+                       if (ast_cdr_disposition(chan->cdr, chan->hangupcause)) {
                                ast_cdr_failed(chan->cdr);
+                       }
                }
+               ast_channel_unlock(chan);
                ast_hangup(chan);
                chan = NULL;
        }

diff --git a/main/features.c b/main/features.c
index 133daba87f1d859744cac163badcd9efe8c08368..c482f9c25541a848854df9ed63a40c3a2550dd93 100644 (file)
--- a/main/features.c
+++ b/main/features.c
@@ -2303,6 +2303,7 @@ static int builtin_blindtransfer(struct ast_channel *chan, struct ast_channel *p
        pbx_builtin_setvar_helper(transferer, "BLINDTRANSFER", transferee->name);
        pbx_builtin_setvar_helper(transferee, "BLINDTRANSFER", transferer->name);
        finishup(transferee);
+       ast_channel_lock(transferer);
        if (!transferer->cdr) { /* this code should never get called (in a perfect world) */
                transferer->cdr = ast_cdr_alloc();
                if (transferer->cdr) {
@@ -2310,6 +2311,7 @@ static int builtin_blindtransfer(struct ast_channel *chan, struct ast_channel *p
                        ast_cdr_start(transferer->cdr);
                }
        }
+       ast_channel_unlock(transferer);
        if (transferer->cdr) {
                struct ast_cdr *swap = transferer->cdr;
 
@@ -3897,11 +3899,15 @@ int ast_bridge_call(struct ast_channel *chan, struct ast_channel *peer, struct a
        /* copy the userfield from the B-leg to A-leg if applicable */
        if (chan->cdr && peer->cdr && !ast_strlen_zero(peer->cdr->userfield)) {
                char tmp[256];
+
+               ast_channel_lock(chan);
                if (!ast_strlen_zero(chan->cdr->userfield)) {
                        snprintf(tmp, sizeof(tmp), "%s;%s", chan->cdr->userfield, peer->cdr->userfield);
                        ast_cdr_appenduserfield(chan, tmp);
-               } else
+               } else {
                        ast_cdr_setuserfield(chan, peer->cdr->userfield);
+               }
+               ast_channel_unlock(chan);
                /* Don't delete the CDR; just disable it. */
                ast_set_flag(peer->cdr, AST_CDR_FLAG_POST_DISABLED);
                we_disabled_peer_cdr = 1;
@@ -3910,7 +3916,7 @@ int ast_bridge_call(struct ast_channel *chan, struct ast_channel *peer, struct a
        ast_copy_string(orig_peername,peer->name,sizeof(orig_peername));
 
        if (!chan_cdr || (chan_cdr && !ast_test_flag(chan_cdr, AST_CDR_FLAG_POST_DISABLED))) {
-               
+               ast_channel_lock_both(chan, peer);
                if (chan_cdr) {
                        ast_set_flag(chan_cdr, AST_CDR_FLAG_MAIN);
                        ast_cdr_update(chan);
@@ -3925,7 +3931,6 @@ int ast_bridge_call(struct ast_channel *chan, struct ast_channel *peer, struct a
                                ast_copy_string(bridge_cdr->userfield, peer_cdr->userfield, sizeof(bridge_cdr->userfield));
                        }
                        ast_cdr_setaccount(peer, chan->accountcode);
-
                } else {
                        /* better yet, in a xfer situation, find out why the chan cdr got zapped (pun unintentional) */
                        bridge_cdr = ast_cdr_alloc(); /* this should be really, really rare/impossible? */
@@ -3948,6 +3953,9 @@ int ast_bridge_call(struct ast_channel *chan, struct ast_channel *peer, struct a
                                ast_cdr_start(bridge_cdr);
                        }
                }
+               ast_channel_unlock(chan);
+               ast_channel_unlock(peer);
+
                ast_debug(4,"bridge answer set, chan answer set\n");
                /* peer_cdr->answer will be set when a macro runs on the peer;
                   in that case, the bridge answer will be delayed while the

diff --git a/main/pbx.c b/main/pbx.c
index c0144db524b2adb5ca647f8b3d5945fd4b8ee120..c23da65620c00afbc98707fb5e2e85ce38a339e6 100644 (file)
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -4952,10 +4952,12 @@ static enum ast_pbx_result __ast_pbx_run(struct ast_channel *c,
                        ast_copy_string(c->context, "default", sizeof(c->context));
                }
        }
+       ast_channel_lock(c);
        if (c->cdr) {
                /* allow CDR variables that have been collected after channel was created to be visible during call */
                ast_cdr_update(c);
        }
+       ast_channel_unlock(c);
        for (;;) {
                char dst_exten[256];    /* buffer to accumulate digits */
                int pos = 0;            /* XXX should check bounds */
@@ -5062,8 +5064,11 @@ static enum ast_pbx_result __ast_pbx_run(struct ast_channel *c,
                                        }
                                        /* Call timed out with no special extension to jump to. */
                                }
-                               if (c->cdr)
+                               ast_channel_lock(c);
+                               if (c->cdr) {
                                        ast_cdr_update(c);
+                               }
+                               ast_channel_unlock(c);
                                error = 1;
                                break;
                        }
@@ -5167,10 +5172,12 @@ static enum ast_pbx_result __ast_pbx_run(struct ast_channel *c,
                                        }
                                }
                        }
+                       ast_channel_lock(c);
                        if (c->cdr) {
                                ast_verb(2, "CDR updated on %s\n",c->name);
                                ast_cdr_update(c);
                        }
+                       ast_channel_unlock(c);
                }
        }
 
@@ -9409,7 +9416,9 @@ static int pbx_builtin_resetcdr(struct ast_channel *chan, const char *data)
 static int pbx_builtin_setamaflags(struct ast_channel *chan, const char *data)
 {
        /* Copy the AMA Flags as specified */
+       ast_channel_lock(chan);
        ast_cdr_setamaflags(chan, data ? data : "");
+       ast_channel_unlock(chan);
        return 0;
 }
 

diff --git a/res/res_monitor.c b/res/res_monitor.c
index 61dc4a2d0395149e149d304297d63c4626ba42ae..9dac05177a50305937eebdac14b394f9da4ac558 100644 (file)
--- a/res/res_monitor.c
+++ b/res/res_monitor.c
@@ -684,9 +684,13 @@ static int start_monitor_exec(struct ast_channel *chan, const char *data)
        if (urlprefix) {
                snprintf(tmp, sizeof(tmp), "%s/%s.%s", urlprefix, args.fname_base,
                        ((strcmp(args.format, "gsm")) ? "wav" : "gsm"));
-               if (!chan->cdr && !(chan->cdr = ast_cdr_alloc()))
+               ast_channel_lock(chan);
+               if (!chan->cdr && !(chan->cdr = ast_cdr_alloc())) {
+                       ast_channel_unlock(chan);
                        return -1;
+               }
                ast_cdr_setuserfield(chan, tmp);
+               ast_channel_unlock(chan);
        }
        if (waitforbridge) {
                /* We must remove the "b" option if listed.  In principle none of