Fix a CEL LINKEDID_END race and local channel linkedids

This patch has the ;2 channel inherit the linkedid of the ;1 channel and fixes
the race condition by no longer scanning the channel list for "other" channels
with the same linkedid. Instead, cel.c has an ao2 container of linkedid strings
and uses the refcount of the string as a counter of how many channels with the
linkedid exist. Not only does this eliminate the race condition, but it also
allows us to look up the linkedid by the hashed key instead of traversing the
entire channel list.

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

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

diff --git a/channels/chan_local.c b/channels/chan_local.c
index a783949afc6a7c443e4e358a5860aec4eca3b63b..eca253831feab044ee2daff4fd44d56130fbdec7 100644 (file)
--- a/channels/chan_local.c
+++ b/channels/chan_local.c
@@ -1113,7 +1113,7 @@ static struct ast_channel *local_new(struct local_pvt *p, int state, const char
        else
                ama = 0;
        if (!(tmp = ast_channel_alloc(1, state, 0, 0, t, p->exten, p->context, linkedid, ama, "Local/%s@%s-%04x;1", p->exten, p->context, randnum)) 
-               || !(tmp2 = ast_channel_alloc(1, AST_STATE_RING, 0, 0, t, p->exten, p->context, linkedid, ama, "Local/%s@%s-%04x;2", p->exten, p->context, randnum))) {
+               || !(tmp2 = ast_channel_alloc(1, AST_STATE_RING, 0, 0, t, p->exten, p->context, tmp->linkedid, ama, "Local/%s@%s-%04x;2", p->exten, p->context, randnum))) {
                if (tmp) {
                        tmp = ast_channel_release(tmp);
                }

diff --git a/main/cel.c b/main/cel.c
index 35e851b03c86fe38bf71384ed7b5e20017a13e20..fbcaa104b3cfcb5142f831cff6d658de59fb9734 100644 (file)
--- a/main/cel.c
+++ b/main/cel.c
@@ -80,6 +80,7 @@ static int64_t eventset;
  * for when they start and end on a channel.
  */
 static struct ao2_container *appset;
+static struct ao2_container *linkedids;
 
 /*!
  * \brief Configured date format for event timestamps
@@ -360,42 +361,30 @@ const char *ast_cel_get_ama_flag_name(enum ast_cel_ama_flag flag)
 
 /* called whenever a channel is destroyed or a linkedid is changed to
  * potentially emit a CEL_LINKEDID_END event */
-
-struct channel_find_data {
-       const struct ast_channel *chan;
-       const char *linkedid;
-};
-
-static int linkedid_match(void *obj, void *arg, void *data, int flags)
-{
-       struct ast_channel *c = obj;
-       struct channel_find_data *find_dat = data;
-       int res;
-
-       ast_channel_lock(c);
-       res = (c != find_dat->chan && c->linkedid && !strcmp(find_dat->linkedid, c->linkedid));
-       ast_channel_unlock(c);
-
-       return res ? CMP_MATCH | CMP_STOP : 0;
-}
-
 void ast_cel_check_retire_linkedid(struct ast_channel *chan)
 {
        const char *linkedid = chan->linkedid;
-       struct channel_find_data find_dat;
+       char *lid;
 
        /* make sure we need to do all this work */
 
-       if (!ast_strlen_zero(linkedid) && ast_cel_track_event(AST_CEL_LINKEDID_END)) {
-               struct ast_channel *tmp = NULL;
-               find_dat.chan = chan;
-               find_dat.linkedid = linkedid;
-               if ((tmp = ast_channel_callback(linkedid_match, NULL, &find_dat, 0))) {
-                       tmp = ast_channel_unref(tmp);
-               } else {
-                       ast_cel_report_event(chan, AST_CEL_LINKEDID_END, NULL, NULL, NULL);
-               }
+       if (ast_strlen_zero(linkedid) || !ast_cel_track_event(AST_CEL_LINKEDID_END)) {
+               return;
+       }
+
+       if (!(lid = ao2_find(linkedids, (void *) linkedid, OBJ_POINTER))) {
+               ast_log(LOG_ERROR, "Something weird happened, couldn't find linkedid %s\n", linkedid);
+               return;
+       }
+
+       /* We have a ref for each channel with this linkedid, the link and the above find, so if after
+        * unreffing for the channel we have a ref of 2, we're done. Unlink and report. */
+       ao2_ref(lid, -1);
+       if (ao2_ref(lid, 0) == 2) {
+               ao2_unlink(linkedids, lid);
+               ast_cel_report_event(chan, AST_CEL_LINKEDID_END, NULL, NULL, NULL);
        }
+       ao2_ref(lid, -1);
 }
 
 struct ast_channel *ast_cel_fabricate_channel_from_event(const struct ast_event *event)
@@ -484,23 +473,30 @@ int ast_cel_report_event(struct ast_channel *chan, enum ast_cel_event_type event
        const char *peername = "";
        struct ast_channel *peer;
 
-       ast_channel_lock(chan);
-       peer = ast_bridged_channel(chan);
-       if (peer) {
-               ast_channel_ref(peer);
-       }
-       ast_channel_unlock(chan);
-
        /* Make sure a reload is not occurring while we're checking to see if this
         * is an event that we care about.  We could lose an important event in this
         * process otherwise. */
        ast_mutex_lock(&reload_lock);
 
+       /* Record the linkedid of new channels if we are tracking LINKEDID_END even if we aren't
+        * reporting on CHANNEL_START so we can track when to send LINKEDID_END */
+       if (cel_enabled && ast_cel_track_event(AST_CEL_LINKEDID_END) && event_type == AST_CEL_CHANNEL_START && chan->linkedid) {
+               char *lid;
+               if (!(lid = ao2_find(linkedids, (void *) chan->linkedid, OBJ_POINTER))) {
+                       if (!(lid = ao2_alloc(strlen(chan->linkedid) + 1, NULL))) {
+                               ast_mutex_unlock(&reload_lock);
+                               return -1;
+                       }
+                       strcpy(lid, chan->linkedid);
+                       ao2_link(linkedids, lid);
+                       /* Leave both the link and the alloc refs to show a count of 1 + the link */
+               }
+               /* If we've found, go ahead and keep the ref to increment count of how many channels
+                * have this linkedid. We'll clean it up in check_retire */
+       }
+
        if (!cel_enabled || !ast_cel_track_event(event_type)) {
                ast_mutex_unlock(&reload_lock);
-               if (peer) {
-                       ast_channel_unref(peer);
-               }
                return 0;
        }
 
@@ -508,9 +504,6 @@ int ast_cel_report_event(struct ast_channel *chan, enum ast_cel_event_type event
                char *app;
                if (!(app = ao2_find(appset, (char *) chan->appl, OBJ_POINTER))) {
                        ast_mutex_unlock(&reload_lock);
-                       if (peer) {
-                               ast_channel_unref(peer);
-                       }
                        return 0;
                }
                ao2_ref(app, -1);
@@ -518,6 +511,13 @@ int ast_cel_report_event(struct ast_channel *chan, enum ast_cel_event_type event
 
        ast_mutex_unlock(&reload_lock);
 
+       ast_channel_lock(chan);
+       peer = ast_bridged_channel(chan);
+       if (peer) {
+               ast_channel_ref(peer);
+       }
+       ast_channel_unlock(chan);
+
        if (peer) {
                ast_channel_lock(peer);
                peername = ast_strdupa(peer->name);
@@ -641,6 +641,9 @@ static int app_cmp(void *obj, void *arg, int flags)
        return !strcasecmp(app1, app2) ? CMP_MATCH | CMP_STOP : 0;
 }
 
+#define lid_hash app_hash
+#define lid_cmp app_cmp
+
 static void ast_cel_engine_term(void)
 {
        if (appset) {
@@ -654,16 +657,16 @@ int ast_cel_engine_init(void)
        if (!(appset = ao2_container_alloc(NUM_APP_BUCKETS, app_hash, app_cmp))) {
                return -1;
        }
-
-       if (do_reload()) {
+       if (!(linkedids = ao2_container_alloc(NUM_APP_BUCKETS, lid_hash, lid_cmp))) {
                ao2_ref(appset, -1);
-               appset = NULL;
                return -1;
        }
 
-       if (ast_cli_register(&cli_status)) {
+       if (do_reload() || ast_cli_register(&cli_status)) {
                ao2_ref(appset, -1);
                appset = NULL;
+               ao2_ref(linkedids, -1);
+               linkedids = NULL;
                return -1;
        }