]> git.ipfire.org Git - thirdparty/asterisk.git/commitdiff
CEL: Protect data structures during reload and shutdown.
authorRichard Mudgett <rmudgett@digium.com>
Fri, 24 Jan 2014 23:55:48 +0000 (23:55 +0000)
committerRichard Mudgett <rmudgett@digium.com>
Fri, 24 Jan 2014 23:55:48 +0000 (23:55 +0000)
The CEL data structures need to be protected during a configuration reload
and shutdown.  Asterisk crashed during a shutdown because CEL events were
still in flight and the CEL data structures were already destroyed.

* Protected the appset and linkedids ao2 containers using the reload_lock.

* Added NULL checks before use of the appset and linkedids ao2 containers
in case the CEL module is already shutdown.

* Fixed overloading of the linkedids held objects reference count.  During
shutdown any held objects would be leaked.

* Fixed memory leak of linkedids held objects if the LINKEDID_END is not
being tracked.  The objects in the linkedids container were not removed if
the LINKEDID_END event is not used.

* Added access protection to the appset container during the CLI "cel show
status" command.

* Made CEL config reload not set defaults if the cel.conf file is invalid.

(closes issue AST-1253)
Reported by: Guenther Kelleter

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

Merged revisions 406417 from http://svn.asterisk.org/svn/asterisk/branches/1.8

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

main/cel.c

index a5d82a606fd3f1d5320057bc4bc482b370415a36..8600b5f0b13d0e21e55cd756b2adda68bf51d741 100644 (file)
@@ -47,6 +47,9 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include "asterisk/cli.h"
 #include "asterisk/astobj2.h"
 
+/*! Config file to load for the CEL feature. */
+static const char cel_conf_file[] = "cel.conf";
+
 /*! Is the CEL subsystem enabled ? */
 static unsigned char cel_enabled;
 
@@ -76,14 +79,39 @@ static int64_t eventset;
  */
 #define NUM_APP_BUCKETS                97
 
+/*!
+ * \brief Lock protecting CEL.
+ *
+ * \note It protects during reloads, shutdown, and accesses to
+ * the appset and linkedids containers.
+ */
+AST_MUTEX_DEFINE_STATIC(reload_lock);
+
 /*!
  * \brief Container of Asterisk application names
  *
  * The apps in this container are the applications that were specified
  * in the configuration as applications that CEL events should be generated
  * for when they start and end on a channel.
+ *
+ * \note Accesses to the appset container must be done while
+ * holding the reload_lock.
  */
 static struct ao2_container *appset;
+
+struct cel_linkedid {
+       /*! Linkedid stored after struct. */
+       const char *id;
+       /*! Number of channels with this linkedid. */
+       unsigned int count;
+};
+
+/*!
+ * \brief Container of channel references to a linkedid for CEL purposes.
+ *
+ * \note Accesses to the linkedids container must be done while
+ * holding the reload_lock.
+ */
 static struct ao2_container *linkedids;
 
 /*!
@@ -138,15 +166,6 @@ unsigned int ast_cel_check_enabled(void)
        return cel_enabled;
 }
 
-static int print_app(void *obj, void *arg, int flags)
-{
-       struct ast_cli_args *a = arg;
-
-       ast_cli(a->fd, "CEL Tracking Application: %s\n", (const char *) obj);
-
-       return 0;
-}
-
 static void print_cel_sub(const struct ast_event *event, void *data)
 {
        struct ast_cli_args *a = data;
@@ -196,7 +215,28 @@ static char *handle_cli_status(struct ast_cli_entry *e, int cmd, struct ast_cli_
                }
        }
 
-       ao2_callback(appset, OBJ_NODATA, print_app, a);
+       /* Accesses to the appset container must be done while holding the reload_lock. */
+       ast_mutex_lock(&reload_lock);
+       if (appset) {
+               struct ao2_iterator iter;
+               char *app;
+
+               iter = ao2_iterator_init(appset, 0);
+               for (;;) {
+                       app = ao2_iterator_next(&iter);
+                       if (!app) {
+                               break;
+                       }
+                       ast_mutex_unlock(&reload_lock);
+
+                       ast_cli(a->fd, "CEL Tracking Application: %s\n", app);
+
+                       ao2_ref(app, -1);
+                       ast_mutex_lock(&reload_lock);
+               }
+               ao2_iterator_destroy(&iter);
+       }
+       ast_mutex_unlock(&reload_lock);
 
        if (!(sub = ast_event_subscribe_new(AST_EVENT_SUB, print_cel_sub, a))) {
                return CLI_FAILURE;
@@ -278,10 +318,12 @@ static void parse_apps(const char *val)
                        continue;
                }
 
-               if (!(app = ao2_alloc(strlen(cur_app) + 1, NULL))) {
+               /* The app object is immutable so it doesn't need a lock of its own. */
+               app = ao2_alloc(strlen(cur_app) + 1, NULL);
+               if (!app) {
                        continue;
                }
-               strcpy(app, cur_app);
+               strcpy(app, cur_app);/* Safe */
 
                ao2_link(appset, app);
                ao2_ref(app, -1);
@@ -289,9 +331,15 @@ static void parse_apps(const char *val)
        }
 }
 
-AST_MUTEX_DEFINE_STATIC(reload_lock);
+static void set_defaults(void)
+{
+       cel_enabled = CEL_ENABLED_DEFAULT;
+       eventset = CEL_DEFAULT_EVENTS;
+       *cel_dateformat = '\0';
+       ao2_callback(appset, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, NULL, NULL);
+}
 
-static int do_reload(void)
+static int do_reload(int is_reload)
 {
        struct ast_config *config;
        const char *enabled_value;
@@ -302,19 +350,32 @@ static int do_reload(void)
 
        ast_mutex_lock(&reload_lock);
 
-       /* Reset all settings before reloading configuration */
-       cel_enabled = CEL_ENABLED_DEFAULT;
-       eventset = CEL_DEFAULT_EVENTS;
-       *cel_dateformat = '\0';
-       ao2_callback(appset, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, NULL, NULL);
-
-       config = ast_config_load2("cel.conf", "cel", config_flags);
+       if (!is_reload) {
+               /* Initialize all settings before first configuration load. */
+               set_defaults();
+       }
 
-       if (config == CONFIG_STATUS_FILEMISSING) {
+       /*
+        * Unfortunately we have to always load the config file because
+        * other modules read the same file.
+        */
+       config = ast_config_load2(cel_conf_file, "cel", config_flags);
+       if (!config || config == CONFIG_STATUS_FILEINVALID) {
+               ast_log(LOG_WARNING, "Could not load %s\n", cel_conf_file);
+               config = NULL;
+               goto return_cleanup;
+       }
+       if (config == CONFIG_STATUS_FILEUNCHANGED) {
+               /* This should never happen because we always load the config file. */
                config = NULL;
                goto return_cleanup;
        }
 
+       if (is_reload) {
+               /* Reset all settings before reloading configuration */
+               set_defaults();
+       }
+
        if ((enabled_value = ast_variable_retrieve(config, "general", "enable"))) {
                cel_enabled = ast_true(enabled_value);
        }
@@ -368,24 +429,48 @@ const char *ast_cel_get_ama_flag_name(enum ast_cel_ama_flag flag)
 void ast_cel_check_retire_linkedid(struct ast_channel *chan)
 {
        const char *linkedid = chan->linkedid;
-       char *lid;
+       struct cel_linkedid *lid;
+       struct cel_linkedid find_lid;
 
-       /* make sure we need to do all this work */
+       if (ast_strlen_zero(linkedid)) {
+               return;
+       }
 
-       if (ast_strlen_zero(linkedid) || !ast_cel_track_event(AST_CEL_LINKEDID_END)) {
+       /* Get the lock in case any CEL events are still in flight when we shutdown. */
+       ast_mutex_lock(&reload_lock);
+
+       if (!cel_enabled || !ast_cel_track_event(AST_CEL_LINKEDID_END)
+               || !linkedids) {
+               /*
+                * CEL is disabled or we are not tracking linkedids
+                * or the CEL module is shutdown.
+                */
+               ast_mutex_unlock(&reload_lock);
                return;
        }
 
-       if (!(lid = ao2_find(linkedids, (void *) linkedid, OBJ_POINTER))) {
+       find_lid.id = linkedid;
+       lid = ao2_find(linkedids, &find_lid, OBJ_POINTER);
+       if (!lid) {
+               ast_mutex_unlock(&reload_lock);
+
+               /*
+                * The user may have done a reload to start tracking linkedids
+                * when a call was already in progress.  This is an unusual kind
+                * of change to make after starting Asterisk.
+                */
                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
-        * before unreffing the channel we have a refcount of 3, we're done. Unlink and report. */
-       if (ao2_ref(lid, -1) == 3) {
+       if (!--lid->count) {
+               /* No channels use this linkedid anymore. */
                ao2_unlink(linkedids, lid);
+               ast_mutex_unlock(&reload_lock);
+
                ast_cel_report_event(chan, AST_CEL_LINKEDID_END, NULL, NULL, NULL);
+       } else {
+               ast_mutex_unlock(&reload_lock);
        }
        ao2_ref(lid, -1);
 }
@@ -511,26 +596,49 @@ struct ast_channel *ast_cel_fabricate_channel_from_event(const struct ast_event
 
 int ast_cel_linkedid_ref(const char *linkedid)
 {
-       char *lid;
+       struct cel_linkedid *lid;
+       struct cel_linkedid find_lid;
 
        if (ast_strlen_zero(linkedid)) {
                ast_log(LOG_ERROR, "The linkedid should never be empty\n");
                return -1;
        }
 
-       if (!(lid = ao2_find(linkedids, (void *) linkedid, OBJ_POINTER))) {
-               if (!(lid = ao2_alloc(strlen(linkedid) + 1, NULL))) {
-                       return -1;
-               }
-               strcpy(lid, linkedid);
-               if (!ao2_link(linkedids, lid)) {
-                       ao2_ref(lid, -1);
+       /* Get the lock in case any CEL events are still in flight when we shutdown. */
+       ast_mutex_lock(&reload_lock);
+
+       if (!cel_enabled || !ast_cel_track_event(AST_CEL_LINKEDID_END)) {
+               /* CEL is disabled or we are not tracking linkedids. */
+               ast_mutex_unlock(&reload_lock);
+               return 0;
+       }
+       if (!linkedids) {
+               /* The CEL module is shutdown.  Abort. */
+               ast_mutex_unlock(&reload_lock);
+               return -1;
+       }
+
+       find_lid.id = linkedid;
+       lid = ao2_find(linkedids, &find_lid, OBJ_POINTER);
+       if (!lid) {
+               /*
+                * Changes to the lid->count member are protected by the
+                * reload_lock so the lid object does not need its own lock.
+                */
+               lid = ao2_alloc(sizeof(*lid) + strlen(linkedid) + 1, NULL);
+               if (!lid) {
+                       ast_mutex_unlock(&reload_lock);
                        return -1;
                }
-               /* Leave both the link and the alloc refs to show a count of 1 + the link */
+               lid->id = (char *) (lid + 1);
+               strcpy((char *) lid->id, linkedid);/* Safe */
+
+               ao2_link(linkedids, lid);
        }
-       /* 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 */
+       ++lid->count;
+       ast_mutex_unlock(&reload_lock);
+       ao2_ref(lid, -1);
+
        return 0;
 }
 
@@ -547,6 +655,12 @@ int ast_cel_report_event(struct ast_channel *chan, enum ast_cel_event_type event
         * process otherwise. */
        ast_mutex_lock(&reload_lock);
 
+       if (!appset) {
+               /* The CEL module is shutdown.  Abort. */
+               ast_mutex_unlock(&reload_lock);
+               return -1;
+       }
+
        /* 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) {
@@ -697,37 +811,71 @@ static int app_hash(const void *obj, const int flags)
 
 static int app_cmp(void *obj, void *arg, int flags)
 {
-       const char *app1 = obj, *app2 = arg;
+       const char *app1 = obj;
+       const char *app2 = arg;
 
-       return !strcasecmp(app1, app2) ? CMP_MATCH | CMP_STOP : 0;
+       return !strcasecmp(app1, app2) ? CMP_MATCH : 0;
 }
 
-#define lid_hash app_hash
-#define lid_cmp app_cmp
+static int lid_hash(const void *obj, const int flags)
+{
+       const struct cel_linkedid *lid = obj;
+       const char *key;
+
+       key = lid->id;
+
+       return ast_str_case_hash(key);
+}
+
+static int lid_cmp(void *obj, void *arg, int flags)
+{
+       struct cel_linkedid *lid1 = obj;
+       struct cel_linkedid *lid2 = arg;
+       const char *key;
+
+       key = lid2->id;
+
+       return !strcasecmp(lid1->id, key) ? CMP_MATCH : 0;
+}
 
 static void ast_cel_engine_term(void)
 {
+       /* Get the lock in case any CEL events are still in flight when we shutdown. */
+       ast_mutex_lock(&reload_lock);
+
        if (appset) {
                ao2_ref(appset, -1);
                appset = NULL;
        }
+       if (linkedids) {
+               ao2_ref(linkedids, -1);
+               linkedids = NULL;
+       }
+
+       ast_mutex_unlock(&reload_lock);
+
+       ast_cli_unregister(&cli_status);
 }
 
 int ast_cel_engine_init(void)
 {
-       if (!(appset = ao2_container_alloc(NUM_APP_BUCKETS, app_hash, app_cmp))) {
+       /*
+        * Accesses to the appset and linkedids containers have to be
+        * protected by the reload_lock so they don't need a lock of
+        * their own.
+        */
+       appset = ao2_container_alloc(NUM_APP_BUCKETS, app_hash, app_cmp);
+       if (!appset) {
                return -1;
        }
-       if (!(linkedids = ao2_container_alloc(NUM_APP_BUCKETS, lid_hash, lid_cmp))) {
-               ao2_ref(appset, -1);
+       linkedids = ao2_container_alloc(NUM_APP_BUCKETS, lid_hash, lid_cmp);
+       if (!linkedids) {
+               ast_cel_engine_term();
                return -1;
        }
 
-       if (do_reload() || ast_cli_register(&cli_status)) {
-               ao2_ref(appset, -1);
-               appset = NULL;
-               ao2_ref(linkedids, -1);
-               linkedids = NULL;
+       if (do_reload(0) || ast_cli_register(&cli_status)) {
+               ast_cel_engine_term();
                return -1;
        }
 
@@ -738,6 +886,6 @@ int ast_cel_engine_init(void)
 
 int ast_cel_engine_reload(void)
 {
-       return do_reload();
+       return do_reload(1);
 }