logger: Prevent duplicate dynamic channels from being added.

There was a problem observed where the "logger add channel" CLI command
would allow for a channel with the same name to be added multiple times.
This would result in each message being written out to the same file
multiple times.

The problem was due to the difference in how logger channel filenames
are stored versus the format they are allowed to be presented when they
are added. For instance, if adding the logger channel "foo" through the
CLI, the result would be a logger channel with the file name
/var/log/asterisk/foo being stored. So when trying to add another "foo"
channel, "foo" would not match "/var/log/asterisk/foo" so we'd happily
add the duplicate channel.

The fix presented here is to introduce two new methods in the logger
code:
 * make_filename(): given a logger channel name, this creates the
   filename for that logger channel.
 * find_logchannel(): given a logger channel name, this calls
   make_filename() and then traverses the list of logchannels in order
   to find a match.

This change has made use of make_filename() and find_logchannel()
throughout to more consistently behave.

ASTERISK-25305 #close
Reported by Mark Michelson

Change-Id: I892d52954d6007d8bc453c3cbdd9235dec9c4a36

1 files changed, 105 insertions, 107 deletions

diff --git a/main/logger.c b/main/logger.c
index 12410cab6..b02e08ef3 100644
--- a/main/logger.c
+++ b/main/logger.c
@@ -278,6 +278,70 @@ static void make_components(struct logchannel *chan)
 	chan->logmask = logmask;
 }
 
+/*!
+ * \brief create the filename that will be used for a logger channel.
+ *
+ * \param channel The name of the logger channel
+ * \param[out] filename The filename for the logger channel
+ * \param size The size of the filename buffer
+ */
+static void make_filename(const char *channel, char *filename, size_t size)
+{
+	const char *log_dir_prefix = "";
+	const char *log_dir_separator = "";
+
+	*filename = '\0';
+
+	if (!strcasecmp(channel, "console")) {
+		return;
+	}
+
+	if (!strncasecmp(channel, "syslog", 6)) {
+		ast_copy_string(filename, channel, size);
+		return;
+	}
+
+	/* It's a filename */
+
+	if (channel[0] != '/') {
+		log_dir_prefix = ast_config_AST_LOG_DIR;
+		log_dir_separator = "/";
+	}
+
+	if (!ast_strlen_zero(hostname)) {
+		snprintf(filename, size, "%s%s%s.%s",
+			log_dir_prefix, log_dir_separator, channel, hostname);
+	} else {
+		snprintf(filename, size, "%s%s%s",
+			log_dir_prefix, log_dir_separator, channel);
+	}
+}
+
+/*!
+ * \brief Find a particular logger channel by name
+ *
+ * \pre logchannels list is locked
+ *
+ * \param channel The name of the logger channel to find
+ * \retval non-NULL The corresponding logger channel
+ * \retval NULL Unable to find a logger channel with that particular name
+ */
+static struct logchannel *find_logchannel(const char *channel)
+{
+	char filename[PATH_MAX];
+	struct logchannel *chan;
+
+	make_filename(channel, filename, sizeof(filename));
+
+	AST_RWLIST_TRAVERSE(&logchannels, chan, list) {
+		if (!strcmp(chan->filename, filename)) {
+			return chan;
+		}
+	}
+
+	return NULL;
+}
+
 static struct logchannel *make_logchannel(const char *channel, const char *components, int lineno, int dynamic)
 {
 	struct logchannel *chan;
@@ -293,6 +357,8 @@ static struct logchannel *make_logchannel(const char *channel, const char *compo
 	chan->lineno = lineno;
 	chan->dynamic = dynamic;
 
+	make_filename(channel, chan->filename, sizeof(chan->filename));
+
 	if (!strcasecmp(channel, "console")) {
 		chan->type = LOGTYPE_CONSOLE;
 	} else if (!strncasecmp(channel, "syslog", 6)) {
@@ -314,24 +380,7 @@ static struct logchannel *make_logchannel(const char *channel, const char *compo
 		}
 
 		chan->type = LOGTYPE_SYSLOG;
-		ast_copy_string(chan->filename, channel, sizeof(chan->filename));
 	} else {
-		const char *log_dir_prefix = "";
-		const char *log_dir_separator = "";
-
-		if (channel[0] != '/') {
-			log_dir_prefix = ast_config_AST_LOG_DIR;
-			log_dir_separator = "/";
-		}
-
-		if (!ast_strlen_zero(hostname)) {
-			snprintf(chan->filename, sizeof(chan->filename), "%s%s%s.%s",
-				log_dir_prefix, log_dir_separator, channel, hostname);
-		} else {
-			snprintf(chan->filename, sizeof(chan->filename), "%s%s%s",
-				log_dir_prefix, log_dir_separator, channel);
-		}
-
 		if (!(chan->fileptr = fopen(chan->filename, "a"))) {
 			/* Can't do real logging here since we're called with a lock
 			 * so log to any attached consoles */
@@ -928,13 +977,9 @@ int ast_logger_rotate_channel(const char *log_channel)
 {
 	struct logchannel *f;
 	int success = AST_LOGGER_FAILURE;
+	char filename[PATH_MAX];
 
-	struct ast_str *filename = ast_str_create(64);
-	if (!filename) {
-		return AST_LOGGER_ALLOC_ERROR;
-	}
-
-	ast_str_append(&filename, 0, "%s/%s", ast_config_AST_LOG_DIR, log_channel);
+	make_filename(log_channel, filename, sizeof(filename));
 
 	AST_RWLIST_WRLOCK(&logchannels);
 
@@ -949,7 +994,7 @@ int ast_logger_rotate_channel(const char *log_channel)
 		if (f->fileptr && (f->fileptr != stdout) && (f->fileptr != stderr)) {
 			fclose(f->fileptr);	/* Close file */
 			f->fileptr = NULL;
-			if (strcmp(ast_str_buffer(filename), f->filename) == 0) {
+			if (strcmp(filename, f->filename) == 0) {
 				rotate_file(f->filename);
 				success = AST_LOGGER_SUCCESS;
 			}
@@ -959,7 +1004,6 @@ int ast_logger_rotate_channel(const char *log_channel)
 	init_logger_chain(1 /* locked */, NULL);
 
 	AST_RWLIST_UNLOCK(&logchannels);
-	ast_free(filename);
 
 	return success;
 }
@@ -1090,45 +1134,35 @@ static char *handle_logger_show_channels(struct ast_cli_entry *e, int cmd, struc
 int ast_logger_create_channel(const char *log_channel, const char *components)
 {
 	struct logchannel *chan;
-	struct ast_str *filename = ast_str_create(64);
-	int chan_exists = AST_LOGGER_SUCCESS;
 
 	if (ast_strlen_zero(components)) {
 		return AST_LOGGER_DECLINE;
 	}
 
-	if (!filename) {
-		return AST_LOGGER_ALLOC_ERROR;
-	}
-
-	ast_str_append(&filename, 0, "%s/%s", ast_config_AST_LOG_DIR, log_channel);
-
 	AST_RWLIST_WRLOCK(&logchannels);
 
-	AST_RWLIST_TRAVERSE(&logchannels, chan, list) {
-		if (!strcmp(ast_str_buffer(filename), chan->filename)) {
-			chan_exists = AST_LOGGER_FAILURE;
-			break;
-		}
+	chan = find_logchannel(log_channel);
+	if (chan) {
+		AST_RWLIST_UNLOCK(&logchannels);
+		return AST_LOGGER_FAILURE;
 	}
 
-	if (!chan_exists) {
-		chan = make_logchannel(log_channel, components, 0, 1);
-		if (chan) {
-			AST_RWLIST_INSERT_HEAD(&logchannels, chan, list);
-			global_logmask |= chan->logmask;
-			chan_exists = AST_LOGGER_SUCCESS;
-		}
+	chan = make_logchannel(log_channel, components, 0, 1);
+	if (!chan) {
+		AST_RWLIST_UNLOCK(&logchannels);
+		return AST_LOGGER_ALLOC_ERROR;
 	}
+
+	AST_RWLIST_INSERT_HEAD(&logchannels, chan, list);
+	global_logmask |= chan->logmask;
+
 	AST_RWLIST_UNLOCK(&logchannels);
 
-	return chan_exists;
+	return AST_LOGGER_SUCCESS;
 }
 
 static char *handle_logger_add_channel(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
 {
-	struct logchannel *chan;
-
 	switch (cmd) {
 	case CLI_INIT:
 		e->command = "logger add channel";
@@ -1147,57 +1181,34 @@ static char *handle_logger_add_channel(struct ast_cli_entry *e, int cmd, struct
 		return CLI_SHOWUSAGE;
 	}
 
-	AST_RWLIST_WRLOCK(&logchannels);
-	AST_RWLIST_TRAVERSE(&logchannels, chan, list) {
-		if (!strcmp(chan->filename, a->argv[3])) {
-			break;
-		}
-	}
-
-	if (chan) {
-		AST_RWLIST_UNLOCK(&logchannels);
-		ast_cli(a->fd, "Logger channel '%s' already exists\n", a->argv[3]);
+	switch (ast_logger_create_channel(a->argv[3], a->argv[4])) {
+	case AST_LOGGER_SUCCESS:
 		return CLI_SUCCESS;
-	}
-
-	chan = make_logchannel(a->argv[3], a->argv[4], 0, 1);
-	if (chan) {
-		AST_RWLIST_INSERT_HEAD(&logchannels, chan, list);
-		global_logmask |= chan->logmask;
-		AST_RWLIST_UNLOCK(&logchannels);
+	case AST_LOGGER_FAILURE:
+		ast_cli(a->fd, "Logger channel '%s' already exists\n", a->argv[3]);
 		return CLI_SUCCESS;
+	case AST_LOGGER_DECLINE:
+	case AST_LOGGER_ALLOC_ERROR:
+	default:
+		ast_cli(a->fd, "ERROR: Unable to create log channel '%s'\n", a->argv[3]);
+		return CLI_FAILURE;
 	}
-
-	AST_RWLIST_UNLOCK(&logchannels);
-	ast_cli(a->fd, "ERROR: Unable to create log channel '%s'\n", a->argv[3]);
-
-	return CLI_FAILURE;
 }
 
 int ast_logger_remove_channel(const char *log_channel)
 {
 	struct logchannel *chan;
-	struct ast_str *filename = ast_str_create(64);
-
-	if (!filename) {
-		return AST_LOGGER_ALLOC_ERROR;
-	}
-
-	ast_str_append(&filename, 0, "%s/%s", ast_config_AST_LOG_DIR, log_channel);
 
 	AST_RWLIST_WRLOCK(&logchannels);
-	AST_RWLIST_TRAVERSE_SAFE_BEGIN(&logchannels, chan, list) {
-		if (chan->dynamic && !strcmp(chan->filename, ast_str_buffer(filename))) {
-			AST_RWLIST_REMOVE_CURRENT(list);
-			break;
-		}
-	}
-	AST_RWLIST_TRAVERSE_SAFE_END;
-	AST_RWLIST_UNLOCK(&logchannels);
 
-	if (!chan) {
+	chan = find_logchannel(log_channel);
+	if (chan && chan->dynamic) {
+		AST_RWLIST_REMOVE(&logchannels, chan, list);
+	} else {
+		AST_RWLIST_UNLOCK(&logchannels);
 		return AST_LOGGER_FAILURE;
 	}
+	AST_RWLIST_UNLOCK(&logchannels);
 
 	if (chan->fileptr) {
 		fclose(chan->fileptr);
@@ -1245,30 +1256,17 @@ static char *handle_logger_remove_channel(struct ast_cli_entry *e, int cmd, stru
 		return CLI_SHOWUSAGE;
 	}
 
-	AST_RWLIST_WRLOCK(&logchannels);
-	AST_RWLIST_TRAVERSE_SAFE_BEGIN(&logchannels, chan, list) {
-		if (chan->dynamic && !strcmp(chan->filename, a->argv[3])) {
-			AST_RWLIST_REMOVE_CURRENT(list);
-			break;
-		}
-	}
-	AST_RWLIST_TRAVERSE_SAFE_END;
-	AST_RWLIST_UNLOCK(&logchannels);
-
-	if (!chan) {
+	switch (ast_logger_remove_channel(a->argv[3])) {
+	case AST_LOGGER_SUCCESS:
+		ast_cli(a->fd, "Removed dynamic logger channel '%s'\n", a->argv[3]);
+		return CLI_SUCCESS;
+	case AST_LOGGER_FAILURE:
 		ast_cli(a->fd, "Unable to find dynamic logger channel '%s'\n", a->argv[3]);
 		return CLI_SUCCESS;
+	default:
+		ast_cli(a->fd, "Internal failure attempting to delete dynamic logger channel '%s'\n", a->argv[3]);
+		return CLI_FAILURE;
 	}
-
-	ast_cli(a->fd, "Removed dynamic logger channel '%s'\n", chan->filename);
-	if (chan->fileptr) {
-		fclose(chan->fileptr);
-		chan->fileptr = NULL;
-	}
-	ast_free(chan);
-	chan = NULL;
-
-	return CLI_SUCCESS;
 }
 
 struct verb {