Avoid unnecessary cleanups during immediate shutdown

This patch addresses issues during immediate shutdowns, where modules
are not unloaded, but Asterisk atexit handlers are run.

In the typical case, this usually isn't a big deal. But the
introduction of the Stasis message bus makes it much more likely for
asynchronous activity to be happening off in some thread during
shutdown.

During an immediate shutdown, Asterisk skips unloading modules. But
while it is processing the atexit handlers, there is a window of time
where some of the core message types have been cleaned up, but the
message bus is still running. Specifically, it's still running
module subscriptions that might be using the core message types. If a
message is received by that subscription in that window, it will
attempt to use a message type that has been cleaned up.

To solve this problem, this patch introduces ast_register_cleanup().
This function operates identically to ast_register_atexit(), except
that cleanup calls are not invoked on an immediate shutdown. All of
the core message type and topic cleanup was moved from atexit handlers
to cleanup handlers.

This ensures that core type and topic cleanup only happens if the
modules that used them are first unloaded.

This patch also changes the ast_assert() when accessing a cleaned up
or uninitialized message type to an error log message. Message type
functions are actually NULL safe across the board, so the assert was a
bit heavy handed. Especially for anyone with DO_CRASH enabled.

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


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

18 files changed, 97 insertions, 55 deletions

diff --git a/include/asterisk.h b/include/asterisk.h
index 58d2df89c..87ebec032 100644
--- a/include/asterisk.h
+++ b/include/asterisk.h
@@ -90,6 +90,22 @@ int ast_pbx_init(void);				/*!< Provided by pbx.c */
 int ast_register_atexit(void (*func)(void));
 
 /*!
+ * \since 12
+ * \brief Register a function to be executed before Asterisk gracefully exits.
+ *
+ * If Asterisk is immediately shutdown (core stop now, or sending the TERM
+ * signal), the callback is not run. When the callbacks are run, they are run in
+ * sequence with ast_register_atexit() callbacks, in the reverse order of
+ * registration.
+ *
+ * \param func The callback function to use.
+ *
+ * \retval 0 on success.
+ * \retval -1 on error.
+ */
+int ast_register_cleanup(void (*func)(void));
+
+/*!
  * \brief Unregister a function registered with ast_register_atexit().
  * \param func The callback function to unregister.
  */
diff --git a/include/asterisk/security_events.h b/include/asterisk/security_events.h
index a971444a4..547b54708 100644
--- a/include/asterisk/security_events.h
+++ b/include/asterisk/security_events.h
@@ -87,12 +87,6 @@ struct stasis_message_type *ast_security_event_type(void);
 int ast_security_stasis_init(void);
 
 /*!
- * \brief removes stasis topic/event types for \ref ast_security_topic and \ref ast_security_event_type
- * \since 12
- */
-void ast_security_stasis_cleanup(void);
-
-/*!
  * \brief Get the list of required IEs for a given security event sub-type
  *
  * \param[in] event_type security event sub-type
diff --git a/include/asterisk/stasis.h b/include/asterisk/stasis.h
index e6ea6fa13..edb38ad1d 100644
--- a/include/asterisk/stasis.h
+++ b/include/asterisk/stasis.h
@@ -633,6 +633,12 @@ struct ao2_container *stasis_cache_dump(struct stasis_caching_topic *caching_top
 /*! @{ */
 
 /*!
+ * \internal
+ * \brief Log a message about invalid attempt to access a type.
+ */
+void stasis_log_bad_type_access(const char *name);
+
+/*!
  * \brief Boiler-plate removing macro for defining message types.
  *
  * \param name Name of message type.
@@ -641,7 +647,9 @@ struct ao2_container *stasis_cache_dump(struct stasis_caching_topic *caching_top
 #define STASIS_MESSAGE_TYPE_DEFN(name)				\
 	static struct stasis_message_type *_priv_ ## name;	\
 	struct stasis_message_type *name(void) {		\
-		ast_assert(_priv_ ## name != NULL);		\
+		if (_priv_ ## name == NULL) {			\
+			stasis_log_bad_type_access(#name);	\
+		}						\
 		return _priv_ ## name;				\
 	}
 
@@ -663,6 +671,15 @@ struct ao2_container *stasis_cache_dump(struct stasis_caching_topic *caching_top
 /*!
  * \brief Boiler-plate removing macro for cleaning up message types.
  *
+ * Note that if your type is defined in core instead of a loadable module, you
+ * should call message type cleanup from an ast_register_cleanup() handler
+ * instead of an ast_register_atexit() handler.
+ *
+ * The reason is that during an immediate shutdown, loadable modules (which may
+ * refer to core message types) are not unloaded. While the atexit handlers are
+ * run, there's a window of time where a module subscription might reference a
+ * core message type after it's been cleaned up. Which is bad.
+ *
  * \param name Name of message type.
  * \since 12
  */
diff --git a/include/asterisk/stasis_bridging.h b/include/asterisk/stasis_bridging.h
index 1b547a7d5..94bc4bc39 100644
--- a/include/asterisk/stasis_bridging.h
+++ b/include/asterisk/stasis_bridging.h
@@ -220,11 +220,6 @@ void ast_bridge_publish_leave(struct ast_bridge *bridge, struct ast_channel *cha
 struct ast_json *ast_bridge_snapshot_to_json(const struct ast_bridge_snapshot *snapshot);
 
 /*!
- * \brief Dispose of the stasis bridging topics and message types
- */
-void ast_stasis_bridging_shutdown(void);
-
-/*!
  * \brief Initialize the stasis bridging topic and message types
  * \retval 0 on success
  * \retval -1 on failure
diff --git a/include/asterisk/stasis_channels.h b/include/asterisk/stasis_channels.h
index e3beb03ce..e521e05eb 100644
--- a/include/asterisk/stasis_channels.h
+++ b/include/asterisk/stasis_channels.h
@@ -462,11 +462,6 @@ int ast_channel_snapshot_caller_id_equal(
 	const struct ast_channel_snapshot *new_snapshot);
 
 /*!
- * \brief Dispose of the stasis channel topics and message types
- */
-void ast_stasis_channels_shutdown(void);
-
-/*!
  * \brief Initialize the stasis channel topic and message types
  */
 void ast_stasis_channels_init(void);
diff --git a/main/app.c b/main/app.c
index 9fa501fe5..f82f7589c 100644
--- a/main/app.c
+++ b/main/app.c
@@ -2816,7 +2816,7 @@ struct stasis_message *ast_mwi_blob_create(struct ast_mwi_state *mwi_state,
 	return msg;
 }
 
-static void app_exit(void)
+static void app_cleanup(void)
 {
 	ao2_cleanup(mwi_topic_all);
 	mwi_topic_all = NULL;
@@ -2829,6 +2829,8 @@ static void app_exit(void)
 
 int app_init(void)
 {
+	ast_register_atexit(app_cleanup);
+
 	if (STASIS_MESSAGE_TYPE_INIT(ast_mwi_state_type) != 0) {
 		return -1;
 	}
@@ -2848,7 +2850,6 @@ int app_init(void)
 		return -1;
 	}
 
-	ast_register_atexit(app_exit);
 	return 0;
 }
 
diff --git a/main/asterisk.c b/main/asterisk.c
index eb403e97f..1310f6cf1 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -347,6 +347,7 @@ struct console {
 
 struct ast_atexit {
 	void (*func)(void);
+	int is_cleanup;
 	AST_LIST_ENTRY(ast_atexit) list;
 };
 
@@ -1111,7 +1112,7 @@ static void stasis_system_topic_cleanup(void)
 /*! \brief Initialize the system level items for \ref stasis */
 static int stasis_system_topic_init(void)
 {
-	ast_register_atexit(stasis_system_topic_cleanup);
+	ast_register_cleanup(stasis_system_topic_cleanup);
 
 	system_topic = stasis_topic_create("ast_system");
 	if (!system_topic) {
@@ -1170,13 +1171,13 @@ static void publish_fully_booted(void)
 	publish_system_message("FullyBooted", json_object);
 }
 
-static void ast_run_atexits(void)
+static void ast_run_atexits(int run_cleanups)
 {
 	struct ast_atexit *ae;
 
 	AST_LIST_LOCK(&atexits);
 	while ((ae = AST_LIST_REMOVE_HEAD(&atexits, list))) {
-		if (ae->func) {
+		if (ae->func && (!ae->is_cleanup || run_cleanups)) {
 			ae->func();
 		}
 		ast_free(ae);
@@ -1198,7 +1199,7 @@ static void __ast_unregister_atexit(void (*func)(void))
 	AST_LIST_TRAVERSE_SAFE_END;
 }
 
-int ast_register_atexit(void (*func)(void))
+static int register_atexit(void (*func)(void), int is_cleanup)
 {
 	struct ast_atexit *ae;
 
@@ -1207,6 +1208,7 @@ int ast_register_atexit(void (*func)(void))
 		return -1;
 	}
 	ae->func = func;
+	ae->is_cleanup = is_cleanup;
 
 	AST_LIST_LOCK(&atexits);
 	__ast_unregister_atexit(func);
@@ -1216,6 +1218,16 @@ int ast_register_atexit(void (*func)(void))
 	return 0;
 }
 
+int ast_register_atexit(void (*func)(void))
+{
+	return register_atexit(func, 0);
+}
+
+int ast_register_cleanup(void (*func)(void))
+{
+	return register_atexit(func, 1);
+}
+
 void ast_unregister_atexit(void (*func)(void))
 {
 	AST_LIST_LOCK(&atexits);
@@ -1980,8 +1992,9 @@ static void really_quit(int num, shutdown_nice_t niceness, int restart)
 {
 	int active_channels;
 	RAII_VAR(struct ast_json *, json_object, NULL, ast_json_unref);
+	int run_cleanups = niceness >= SHUTDOWN_NICE;
 
-	if (niceness >= SHUTDOWN_NICE) {
+	if (run_cleanups) {
 		ast_module_shutdown();
 	}
 
@@ -2021,7 +2034,7 @@ static void really_quit(int num, shutdown_nice_t niceness, int restart)
 		active_channels ? "uncleanly" : "cleanly", num);
 
 	ast_verb(0, "Executing last minute cleanups\n");
-	ast_run_atexits();
+	ast_run_atexits(run_cleanups);
 
 	ast_debug(1, "Asterisk ending (%d).\n", num);
 	if (ast_socket > -1) {
@@ -4330,7 +4343,6 @@ int main(int argc, char *argv[])
 	}
 
 	if (ast_security_stasis_init()) {		/* Initialize Security Stasis Topic and Events */
-		ast_security_stasis_cleanup();
 		printf("%s", term_quit());
 		exit(1);
 	}
diff --git a/main/bridging.c b/main/bridging.c
index 2c32577c3..c437be335 100644
--- a/main/bridging.c
+++ b/main/bridging.c
@@ -6247,26 +6247,24 @@ static void bridge_shutdown(void)
 	bridges = NULL;
 	ao2_cleanup(bridge_manager);
 	bridge_manager = NULL;
-	ast_stasis_bridging_shutdown();
 }
 
 int ast_bridging_init(void)
 {
+	ast_register_atexit(bridge_shutdown);
+
 	if (ast_stasis_bridging_init()) {
-		bridge_shutdown();
 		return -1;
 	}
 
 	bridge_manager = bridge_manager_create();
 	if (!bridge_manager) {
-		bridge_shutdown();
 		return -1;
 	}
 
 	bridges = ao2_container_alloc_rbtree(AO2_ALLOC_OPT_LOCK_MUTEX,
 		AO2_CONTAINER_ALLOC_OPT_DUPS_REPLACE, bridge_sort_cmp, NULL);
 	if (!bridges) {
-		bridge_shutdown();
 		return -1;
 	}
 
@@ -6275,6 +6273,5 @@ int ast_bridging_init(void)
 /* BUGBUG need AMI action equivalents to the CLI commands. */
 	ast_cli_register_multiple(bridge_cli, ARRAY_LEN(bridge_cli));
 
-	ast_register_atexit(bridge_shutdown);
 	return 0;
 }
diff --git a/main/channel.c b/main/channel.c
index e9ec45c49..6549131bf 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -8603,9 +8603,6 @@ struct varshead *ast_channel_get_manager_vars(struct ast_channel *chan)
 
 static void channels_shutdown(void)
 {
-
-	ast_stasis_channels_shutdown();
-
 	free_channelvars();
 
 	ast_data_unregister(NULL);
diff --git a/main/devicestate.c b/main/devicestate.c
index f331b1d19..2f753bdd7 100644
--- a/main/devicestate.c
+++ b/main/devicestate.c
@@ -772,7 +772,7 @@ static const char *device_state_get_id(struct stasis_message *message)
 	return device_state->cache_id;
 }
 
-static void devstate_exit(void)
+static void devstate_cleanup(void)
 {
 	ao2_cleanup(device_state_topic_all);
 	device_state_topic_all = NULL;
@@ -784,6 +784,8 @@ static void devstate_exit(void)
 
 int devstate_init(void)
 {
+	ast_register_cleanup(devstate_cleanup);
+
 	if (STASIS_MESSAGE_TYPE_INIT(ast_device_state_message_type) != 0) {
 		return -1;
 	}
@@ -807,6 +809,5 @@ int devstate_init(void)
 		return -1;
 	}
 
-	ast_register_atexit(devstate_exit);
 	return 0;
 }
diff --git a/main/named_acl.c b/main/named_acl.c
index 092aa94a6..d374e3a71 100644
--- a/main/named_acl.c
+++ b/main/named_acl.c
@@ -360,7 +360,7 @@ struct ast_ha *ast_named_acl_find(const char *name, int *is_realtime, int *is_un
 /*! \brief Message type for named ACL changes */
 STASIS_MESSAGE_TYPE_DEFN(ast_named_acl_change_type);
 
-static void acl_stasis_shutdown(void)
+static void acl_stasis_cleanup(void)
 {
 	STASIS_MESSAGE_TYPE_CLEANUP(ast_named_acl_change_type);
 }
@@ -371,7 +371,7 @@ static void acl_stasis_shutdown(void)
  */
 static void ast_acl_stasis_init(void)
 {
-	ast_register_atexit(acl_stasis_shutdown);
+	ast_register_cleanup(acl_stasis_cleanup);
 	STASIS_MESSAGE_TYPE_INIT(ast_named_acl_change_type);
 }
 
diff --git a/main/presencestate.c b/main/presencestate.c
index feb6474ca..2eab99177 100644
--- a/main/presencestate.c
+++ b/main/presencestate.c
@@ -321,6 +321,8 @@ static void presence_state_engine_cleanup(void)
 
 int ast_presence_state_engine_init(void)
 {
+	ast_register_cleanup(presence_state_engine_cleanup);
+
 	if (STASIS_MESSAGE_TYPE_INIT(ast_presence_state_message_type) != 0) {
 		return -1;
 	}
@@ -334,7 +336,6 @@ int ast_presence_state_engine_init(void)
 	if (!presence_state_topic_cached) {
 		return -1;
 	}
-	ast_register_atexit(presence_state_engine_cleanup);
 
 	return 0;
 }
diff --git a/main/security_events.c b/main/security_events.c
index d42bea64a..eb2dae2fe 100644
--- a/main/security_events.c
+++ b/main/security_events.c
@@ -54,8 +54,18 @@ struct stasis_topic *ast_security_topic(void)
 /*! \brief Message type for security events */
 STASIS_MESSAGE_TYPE_DEFN(ast_security_event_type);
 
+static void security_stasis_cleanup(void)
+{
+	ao2_cleanup(security_topic);
+	security_topic = NULL;
+
+	STASIS_MESSAGE_TYPE_CLEANUP(ast_security_event_type);
+}
+
 int ast_security_stasis_init(void)
 {
+	ast_register_cleanup(security_stasis_cleanup);
+
 	security_topic = stasis_topic_create("ast_security");
 	if (!security_topic) {
 		return -1;
@@ -65,21 +75,10 @@ int ast_security_stasis_init(void)
 		return -1;
 	}
 
-	if (ast_register_atexit(ast_security_stasis_cleanup)) {
-		return -1;
-	}
 
 	return 0;
 }
 
-void ast_security_stasis_cleanup(void)
-{
-	STASIS_MESSAGE_TYPE_CLEANUP(ast_security_event_type);
-
-	ao2_cleanup(security_topic);
-	security_topic = NULL;
-}
-
 static const struct {
 	const char *name;
 	uint32_t version;
diff --git a/main/stasis.c b/main/stasis.c
index d0ded401c..e810dd852 100644
--- a/main/stasis.c
+++ b/main/stasis.c
@@ -619,11 +619,21 @@ struct stasis_topic *stasis_topic_pool_get_topic(struct stasis_topic_pool *pool,
 	return topic_pool_entry->topic;
 }
 
+void stasis_log_bad_type_access(const char *name)
+{
+	ast_log(LOG_ERROR, "Use of %s() before init/after destruction\n", name);
+}
+
 /*! \brief Cleanup function */
 static void stasis_exit(void)
 {
 	ast_threadpool_shutdown(pool);
 	pool = NULL;
+}
+
+/*! \brief Cleanup function for graceful shutdowns */
+static void stasis_cleanup(void)
+{
 	STASIS_MESSAGE_TYPE_CLEANUP(stasis_subscription_change_type);
 }
 
@@ -640,6 +650,8 @@ int stasis_init(void)
 		.max_size = 200
 	};
 
+	/* Be sure the types are cleaned up after the message bus */
+	ast_register_cleanup(stasis_cleanup);
 	ast_register_atexit(stasis_exit);
 
 	if (pool) {
diff --git a/main/stasis_bridging.c b/main/stasis_bridging.c
index 49e1fcfdf..56c1dfbcd 100644
--- a/main/stasis_bridging.c
+++ b/main/stasis_bridging.c
@@ -319,7 +319,7 @@ struct ast_json *ast_bridge_snapshot_to_json(const struct ast_bridge_snapshot *s
 	return ast_json_ref(json_chan);
 }
 
-void ast_stasis_bridging_shutdown(void)
+static void stasis_bridging_cleanup(void)
 {
 	ao2_cleanup(bridge_topic_all);
 	bridge_topic_all = NULL;
@@ -347,6 +347,8 @@ static const char *bridge_snapshot_get_id(struct stasis_message *msg)
 
 int ast_stasis_bridging_init(void)
 {
+	ast_register_cleanup(stasis_bridging_cleanup);
+
 	STASIS_MESSAGE_TYPE_INIT(ast_bridge_snapshot_type);
 	STASIS_MESSAGE_TYPE_INIT(ast_bridge_merge_message_type);
 	STASIS_MESSAGE_TYPE_INIT(ast_channel_entered_bridge_type);
diff --git a/main/stasis_cache.c b/main/stasis_cache.c
index 154b4f020..ac34959db 100644
--- a/main/stasis_cache.c
+++ b/main/stasis_cache.c
@@ -446,7 +446,7 @@ struct stasis_caching_topic *stasis_caching_topic_create(struct stasis_topic *or
 	return caching_topic;
 }
 
-static void stasis_cache_exit(void)
+static void stasis_cache_cleanup(void)
 {
 	STASIS_MESSAGE_TYPE_CLEANUP(stasis_cache_clear_type);
 	STASIS_MESSAGE_TYPE_CLEANUP(stasis_cache_update_type);
@@ -454,7 +454,7 @@ static void stasis_cache_exit(void)
 
 int stasis_cache_init(void)
 {
-	ast_register_atexit(stasis_cache_exit);
+	ast_register_cleanup(stasis_cache_cleanup);
 
 	if (STASIS_MESSAGE_TYPE_INIT(stasis_cache_clear_type) != 0) {
 		return -1;
diff --git a/main/stasis_channels.c b/main/stasis_channels.c
index 0ec40bbfb..c08bbd99f 100644
--- a/main/stasis_channels.c
+++ b/main/stasis_channels.c
@@ -575,7 +575,7 @@ int ast_channel_snapshot_caller_id_equal(
 		strcmp(old_snapshot->caller_name, new_snapshot->caller_name) == 0;
 }
 
-void ast_stasis_channels_shutdown(void)
+static void stasis_channels_cleanup(void)
 {
 	channel_topic_all_cached = stasis_caching_unsubscribe_and_join(channel_topic_all_cached);
 	ao2_cleanup(channel_topic_all);
@@ -601,6 +601,8 @@ void ast_stasis_channels_shutdown(void)
 
 void ast_stasis_channels_init(void)
 {
+	ast_register_cleanup(stasis_channels_cleanup);
+
 	STASIS_MESSAGE_TYPE_INIT(ast_channel_snapshot_type);
 	STASIS_MESSAGE_TYPE_INIT(ast_channel_dial_type);
 	STASIS_MESSAGE_TYPE_INIT(ast_channel_varset_type);
diff --git a/main/test.c b/main/test.c
index 3a13d4d39..2109c9478 100644
--- a/main/test.c
+++ b/main/test.c
@@ -1005,6 +1005,8 @@ static void test_cleanup(void)
 int ast_test_init(void)
 {
 #ifdef TEST_FRAMEWORK
+	ast_register_cleanup(test_cleanup);
+
 	/* Create stasis topic */
 	test_suite_topic = stasis_topic_create("test_suite_topic");
 	if (!test_suite_topic) {
@@ -1017,7 +1019,6 @@ int ast_test_init(void)
 
 	/* Register cli commands */
 	ast_cli_register_multiple(test_cli, ARRAY_LEN(test_cli));
-	ast_register_atexit(test_cleanup);
 #endif
 
 	return 0;