From c0bbfe63f9493a9717e41f15f529262604e157ee Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Fri, 24 Jan 2014 23:07:08 +0000 Subject: [PATCH] CEL: Protect data structures during reload and shutdown. 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. As a result appset, linkedids, and held objects don't need a 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/branches/11@406418 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- main/cel.c | 273 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 212 insertions(+), 61 deletions(-) diff --git a/main/cel.c b/main/cel.c index 1ed111e1a8..f540ac8672 100644 --- a/main/cel.c +++ b/main/cel.c @@ -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 { + /*! Number of channels with this linkedid. */ + unsigned int count; + /*! Linkedid stored at end of struct. */ + char id[0]; +}; + +/*! + * \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_options(strlen(cur_app) + 1, NULL, AO2_ALLOC_OPT_LOCK_NOLOCK); + 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 || config == CONFIG_STATUS_FILEUNCHANGED || config == CONFIG_STATUS_FILEINVALID) { + /* + * 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,46 @@ 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 = ast_channel_linkedid(chan); - char *lid; + struct cel_linkedid *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))) { + lid = ao2_find(linkedids, (void *) linkedid, OBJ_KEY); + 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); } @@ -517,26 +600,47 @@ 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; 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; + } + + lid = ao2_find(linkedids, (void *) linkedid, OBJ_KEY); + 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_options(sizeof(*lid) + strlen(linkedid) + 1, NULL, + AO2_ALLOC_OPT_LOCK_NOLOCK); + 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 */ + strcpy(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; } @@ -554,6 +658,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 && linkedid) { @@ -702,42 +812,83 @@ 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 void ast_cel_engine_term(void) +static int lid_hash(const void *obj, const int flags) { - if (appset) { - ao2_ref(appset, -1); - appset = NULL; + const struct cel_linkedid *lid = obj; + const char *key = obj; + + switch (flags & (OBJ_POINTER | OBJ_KEY)) { + case OBJ_POINTER: + default: + key = lid->id; + break; + case OBJ_KEY: + break; } - if (linkedids) { - ao2_ref(linkedids, -1); - linkedids = NULL; + + 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 = arg; + + switch (flags & (OBJ_POINTER | OBJ_KEY)) { + case OBJ_POINTER: + default: + key = lid2->id; + break; + case OBJ_KEY: + break; } + + 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); + + ao2_cleanup(appset); + appset = NULL; + ao2_cleanup(linkedids); + 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_options(AO2_ALLOC_OPT_LOCK_NOLOCK, 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_options(AO2_ALLOC_OPT_LOCK_NOLOCK, 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; } @@ -748,6 +899,6 @@ int ast_cel_engine_init(void) int ast_cel_engine_reload(void) { - return do_reload(); + return do_reload(1); }