[Asterisk-code-review] Restrict CLI/AMI commands on shutdown. (asterisk[13])

[Mark Michelson]

Mark Michelson has uploaded a new change for review.

  https://gerrit.asterisk.org/2283

Change subject: Restrict CLI/AMI commands on shutdown.
......................................................................

Restrict CLI/AMI commands on shutdown.

During stress testing, we have frequently seen crashes occur because a
CLI or AMI command attempts to access information that is in the process
of being destroyed.

When addressing how to fix this issue, we initially considered fixing
individual crashes we observed. However, the changes required to fix
those problems would introduce considerable overhead to the nominal
case. This is not reasonable in order to prevent a crash from occurring
while Asterisk is already shutting down.

Instead, this change makes it so AMI and CLI commands cannot be executed
if Asterisk is being shut down. For AMI, this is absolute. For CLI,
though, certain commands can be registered so that they may be run
during Asterisk shutdown.

Change-Id: I6f5b8e665bd4d0108014a8b6589729ecd3677eef
---
M include/asterisk/cli.h
M main/asterisk.c
M main/cli.c
M main/manager.c
M main/utils.c
5 files changed, 23 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/83/2283/1

diff --git a/include/asterisk/cli.h b/include/asterisk/cli.h
index 458ebc8..6788c14 100644
--- a/include/asterisk/cli.h
+++ b/include/asterisk/cli.h
@@ -187,14 +187,17 @@
 	char *(*handler)(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a);
 	/*! For linking */
 	AST_LIST_ENTRY(ast_cli_entry) list;
+	int allow_on_shutdown;
 };
 
 #if defined(__cplusplus) || defined(c_plusplus)
-#define AST_CLI_DEFINE(fn, txt) { { "" }, txt, NULL, 0, NULL, NULL, 0, 0, NULL, fn }
+#define AST_CLI_DEFINE(fn, txt) { { "" }, txt, NULL, 0, NULL, NULL, 0, 0, NULL, fn , 0}
+#define AST_CLI_DEFINE_SHUTDOWN(fn, txt) { { "" }, txt, NULL, 0, NULL, NULL, 0, 0, NULL, fn, 1}
 #else
 /* XXX the parser in gcc 2.95 gets confused if you don't put a space
  * between the last arg before VA_ARGS and the comma */
 #define AST_CLI_DEFINE(fn, txt , ... )	{ .handler = fn, .summary = txt, ## __VA_ARGS__ }
+#define AST_CLI_DEFINE_SHUTDOWN(fn, txt) AST_CLI_DEFINE(fn, txt, .allow_on_shutdown = 1)
 #endif
 
 /*!
diff --git a/main/asterisk.c b/main/asterisk.c
index 0cecc51..bfbdc61 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -2680,7 +2680,7 @@
  * unregistered.
  */
 static struct ast_cli_entry cli_asterisk_shutdown[] = {
-	AST_CLI_DEFINE(handle_stop_now, "Shut down Asterisk immediately"),
+	AST_CLI_DEFINE_SHUTDOWN(handle_stop_now, "Shut down Asterisk immediately"),
 	AST_CLI_DEFINE(handle_stop_gracefully, "Gracefully shut down Asterisk"),
 	AST_CLI_DEFINE(handle_stop_when_convenient, "Shut down Asterisk at empty call volume"),
 	AST_CLI_DEFINE(handle_restart_now, "Restart Asterisk immediately"),
@@ -2689,7 +2689,7 @@
 };
 
 static struct ast_cli_entry cli_asterisk[] = {
-	AST_CLI_DEFINE(handle_abort_shutdown, "Cancel a running shutdown"),
+	AST_CLI_DEFINE_SHUTDOWN(handle_abort_shutdown, "Cancel a running shutdown"),
 	AST_CLI_DEFINE(show_warranty, "Show the warranty (if any) for this copy of Asterisk"),
 	AST_CLI_DEFINE(show_license, "Show the license(s) for this copy of Asterisk"),
 	AST_CLI_DEFINE(handle_version, "Display version info"),
diff --git a/main/cli.c b/main/cli.c
index 7f86eab..1655c47 100644
--- a/main/cli.c
+++ b/main/cli.c
@@ -2698,11 +2698,15 @@
 
 	AST_RWLIST_RDLOCK(&helpers);
 	e = find_cli(args + 1, 0);
-	if (e)
-		ast_atomic_fetchadd_int(&e->inuse, 1);
 	AST_RWLIST_UNLOCK(&helpers);
+
 	if (e == NULL) {
 		ast_cli(fd, "No such command '%s' (type 'core show help %s' for other possible commands)\n", s, find_best(args + 1));
+		goto done;
+	}
+
+	if (ast_shutting_down() && !e->allow_on_shutdown) {
+		ast_cli(fd, "Command '%s' cannot be run during shutdown\n", s);
 		goto done;
 	}
 
@@ -2714,6 +2718,7 @@
 		return 0;
 	}
 
+	ast_atomic_fetchadd_int(&e->inuse, 1);
 	/*
 	 * Within the handler, argv[-1] contains a pointer to the ast_cli_entry.
 	 * Remember that the array returned by parse_args is NULL-terminated.
diff --git a/main/manager.c b/main/manager.c
index de00381..6daf5f0 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -6106,6 +6106,7 @@
 	const char *action;
 
 	action = __astman_get_header(m, "Action", GET_HEADER_SKIP_EMPTY);
+
 	if (ast_strlen_zero(action)) {
 		report_req_bad_format(s, "NONE");
 		mansession_lock(s);
@@ -6114,6 +6115,14 @@
 		return 0;
 	}
 
+	if (ast_shutting_down()) {
+		ast_log(LOG_ERROR, "Unable to process manager action '%s'. Asterisk is shutting down.\n", action);
+		mansession_lock(s);
+		astman_send_error(s, m, "Asterisk is shutting down");
+		mansession_unlock(s);
+		return 0;
+	}
+
 	if (!s->session->authenticated
 		&& strcasecmp(action, "Login")
 		&& strcasecmp(action, "Logoff")
diff --git a/main/utils.c b/main/utils.c
index 0b527f0..84b78e8 100644
--- a/main/utils.c
+++ b/main/utils.c
@@ -1177,7 +1177,7 @@
 }
 
 static struct ast_cli_entry utils_cli[] = {
-	AST_CLI_DEFINE(handle_show_locks, "Show which locks are held by which thread"),
+	AST_CLI_DEFINE_SHUTDOWN(handle_show_locks, "Show which locks are held by which thread"),
 };
 
 #endif /* DEBUG_THREADS */

-- 
To view, visit https://gerrit.asterisk.org/2283
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6f5b8e665bd4d0108014a8b6589729ecd3677eef
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>