[asterisk-commits] rmudgett: branch certified-1.8.15 r406467 - in /certified/branches/1.8.15: ./...
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Fri Jan 24 17:55:51 CST 2014
Author: rmudgett
Date: Fri Jan 24 17:55:48 2014
New Revision: 406467
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=406467
Log:
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.
* 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
Modified:
certified/branches/1.8.15/ (props changed)
certified/branches/1.8.15/main/cel.c
Propchange: certified/branches/1.8.15/
------------------------------------------------------------------------------
Binary property 'branch-1.8-merged' - no diff available.
Modified: certified/branches/1.8.15/main/cel.c
URL: http://svnview.digium.com/svn/asterisk/certified/branches/1.8.15/main/cel.c?view=diff&rev=406467&r1=406466&r2=406467
==============================================================================
--- certified/branches/1.8.15/main/cel.c (original)
+++ certified/branches/1.8.15/main/cel.c Fri Jan 24 17:55:48 2014
@@ -47,6 +47,9 @@
#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;
@@ -77,13 +80,38 @@
#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 @@
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 @@
}
}
- 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 @@
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 @@
}
}
-AST_MUTEX_DEFINE_STATIC(reload_lock);
-
-static int do_reload(void)
+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(int is_reload)
{
struct ast_config *config;
const char *enabled_value;
@@ -302,17 +350,30 @@
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 (config == CONFIG_STATUS_FILEMISSING) {
+ if (!is_reload) {
+ /* Initialize all settings before first configuration load. */
+ set_defaults();
+ }
+
+ /*
+ * 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"))) {
@@ -368,24 +429,48 @@
void ast_cel_check_retire_linkedid(struct ast_channel *chan)
{
const char *linkedid = chan->linkedid;
- 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 cel_linkedid *lid;
+ struct cel_linkedid find_lid;
+
+ if (ast_strlen_zero(linkedid)) {
return;
}
- if (!(lid = ao2_find(linkedids, (void *) linkedid, OBJ_POINTER))) {
+ /* 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;
+ }
+
+ 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 @@
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))) {
+ /* 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;
}
- strcpy(lid, linkedid);
- if (!ao2_link(linkedids, lid)) {
- ao2_ref(lid, -1);
- return -1;
- }
- /* 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 */
+ lid->id = (char *) (lid + 1);
+ strcpy((char *) lid->id, linkedid);/* Safe */
+
+ ao2_link(linkedids, lid);
+ }
+ ++lid->count;
+ ast_mutex_unlock(&reload_lock);
+ ao2_ref(lid, -1);
+
return 0;
}
@@ -546,6 +654,12 @@
* is an event that we care about. We could lose an important event in this
* 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 */
@@ -697,39 +811,73 @@
static int app_cmp(void *obj, void *arg, int flags)
{
- const char *app1 = obj, *app2 = arg;
-
- return !strcasecmp(app1, app2) ? CMP_MATCH | CMP_STOP : 0;
-}
-
-#define lid_hash app_hash
-#define lid_cmp app_cmp
+ const char *app1 = obj;
+ const char *app2 = arg;
+
+ return !strcasecmp(app1, app2) ? CMP_MATCH : 0;
+}
+
+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;
}
-}
-
-int ast_cel_engine_init(void)
-{
- if (!(appset = ao2_container_alloc(NUM_APP_BUCKETS, app_hash, app_cmp))) {
- return -1;
- }
- if (!(linkedids = ao2_container_alloc(NUM_APP_BUCKETS, lid_hash, lid_cmp))) {
- ao2_ref(appset, -1);
- return -1;
- }
-
- if (do_reload() || ast_cli_register(&cli_status)) {
- 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)
+{
+ /*
+ * 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;
}
+ linkedids = ao2_container_alloc(NUM_APP_BUCKETS, lid_hash, lid_cmp);
+ if (!linkedids) {
+ ast_cel_engine_term();
+ return -1;
+ }
+
+ if (do_reload(0) || ast_cli_register(&cli_status)) {
+ ast_cel_engine_term();
+ return -1;
+ }
ast_register_atexit(ast_cel_engine_term);
@@ -738,6 +886,6 @@
int ast_cel_engine_reload(void)
{
- return do_reload();
-}
-
+ return do_reload(1);
+}
+
More information about the asterisk-commits
mailing list