[Asterisk-code-review] res_musiconhold: Prevent crash with realtime MoH (asterisk[13])

Sean Bright asteriskteam at digium.com
Mon Jul 20 09:01:53 CDT 2020


Sean Bright has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/14684 )


Change subject: res_musiconhold: Prevent crash with realtime MoH
......................................................................

res_musiconhold: Prevent crash with realtime MoH

The MoH class internal file vector is potentially being manipulated by
multiple threads at the same time without sufficient locking. Switch to a
reference counted list and operate on copies where necessary.

Two other minor changes:

* Use struct dirent::d_type if it is available to avoid a stat() call

* Make the music class directory absolute at initialization time instead
  of each time we look for files

ASTERISK-28927 #close

Change-Id: I479c5dcf88db670956e8cac177b5826c986b0217
---
M res/res_musiconhold.c
1 file changed, 150 insertions(+), 82 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/84/14684/1

diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c
index 60682fb..c8a5254 100644
--- a/res/res_musiconhold.c
+++ b/res/res_musiconhold.c
@@ -165,13 +165,13 @@
 
 struct mohclass {
 	char name[MAX_MUSICCLASS];
-	char dir[256];
+	char dir[PATH_MAX];
 	char args[256];
 	char announcement[256];
 	char mode[80];
 	char digit;
-	/*! A vector of filenames in "files" mode */
-	struct ast_vector_string files;
+	/*! An immutable vector of filenames in "files" mode */
+	struct ast_vector_string *files;
 	unsigned int flags;
 	/*! The format from the MOH source, not applicable to "files" mode */
 	struct ast_format *format;
@@ -311,6 +311,7 @@
 static int ast_moh_files_next(struct ast_channel *chan)
 {
 	struct moh_files_state *state = ast_channel_music_state(chan);
+	struct ast_vector_string *files;
 	int tries;
 	size_t file_count;
 
@@ -330,16 +331,21 @@
 		state->announcement = 0;
 	}
 
-	file_count = AST_VECTOR_SIZE(&state->class->files);
+	ao2_lock(state->class);
+	files = ao2_bump(state->class->files);
+	ao2_unlock(state->class);
+
+	file_count = AST_VECTOR_SIZE(files);
 	if (!file_count) {
 		ast_log(LOG_WARNING, "No files available for class '%s'\n", state->class->name);
+		ao2_ref(files, -1);
 		return -1;
 	}
 
 	if (state->pos == 0 && ast_strlen_zero(state->save_pos_filename)) {
 		/* First time so lets play the file. */
 		state->save_pos = -1;
-	} else if (state->save_pos >= 0 && state->save_pos < file_count && !strcmp(AST_VECTOR_GET(&state->class->files, state->save_pos), state->save_pos_filename)) {
+	} else if (state->save_pos >= 0 && state->save_pos < file_count && !strcmp(AST_VECTOR_GET(files, state->save_pos), state->save_pos_filename)) {
 		/* If a specific file has been saved confirm it still exists and that it is still valid */
 		state->pos = state->save_pos;
 		state->save_pos = -1;
@@ -347,7 +353,7 @@
 		/* Get a random file and ensure we can open it */
 		for (tries = 0; tries < 20; tries++) {
 			state->pos = ast_random() % file_count;
-			if (ast_fileexists(AST_VECTOR_GET(&state->class->files, state->pos), NULL, NULL) > 0) {
+			if (ast_fileexists(AST_VECTOR_GET(files, state->pos), NULL, NULL) > 0) {
 				break;
 			}
 		}
@@ -362,21 +368,22 @@
 	}
 
 	for (tries = 0; tries < file_count; ++tries) {
-		if (ast_openstream_full(chan, AST_VECTOR_GET(&state->class->files, state->pos), ast_channel_language(chan), 1)) {
+		if (ast_openstream_full(chan, AST_VECTOR_GET(files, state->pos), ast_channel_language(chan), 1)) {
 			break;
 		}
 
-		ast_log(LOG_WARNING, "Unable to open file '%s': %s\n", AST_VECTOR_GET(&state->class->files, state->pos), strerror(errno));
+		ast_log(LOG_WARNING, "Unable to open file '%s': %s\n", AST_VECTOR_GET(files, state->pos), strerror(errno));
 		state->pos++;
 		state->pos %= file_count;
 	}
 
 	if (tries == file_count) {
+		ao2_ref(files, -1);
 		return -1;
 	}
 
 	/* Record the pointer to the filename for position resuming later */
-	ast_copy_string(state->save_pos_filename, AST_VECTOR_GET(&state->class->files, state->pos), sizeof(state->save_pos_filename));
+	ast_copy_string(state->save_pos_filename, AST_VECTOR_GET(files, state->pos), sizeof(state->save_pos_filename));
 
 	ast_debug(1, "%s Opened file %d '%s'\n", ast_channel_name(chan), state->pos, state->save_pos_filename);
 
@@ -393,6 +400,7 @@
 		}
 	}
 
+	ao2_ref(files, -1);
 	return 0;
 }
 
@@ -502,7 +510,9 @@
 		}
 	}
 
-	file_count = AST_VECTOR_SIZE(&class->files);
+	ao2_lock(class);
+	file_count = AST_VECTOR_SIZE(class->files);
+	ao2_unlock(class);
 
 	/* Resume MOH from where we left off last time or start from scratch? */
 	if (state->save_total != file_count || strcmp(state->name, class->name) != 0) {
@@ -1076,83 +1086,117 @@
 	.digit    = moh_handle_digit,
 };
 
-static int moh_scan_files(struct mohclass *class) {
+static void moh_file_vector_destructor(void *obj)
+{
+	struct ast_vector_string *files = obj;
+	AST_VECTOR_RESET(files, ast_free);
+	AST_VECTOR_FREE(files);
+}
 
-	DIR *files_DIR;
-	struct dirent *files_dirent;
-	char dir_path[PATH_MAX - sizeof(class->dir)];
-	char filepath[PATH_MAX];
-	char *ext;
-	struct stat statbuf;
-	int res;
-
-	if (class->dir[0] != '/') {
-		snprintf(dir_path, sizeof(dir_path), "%s/%s", ast_config_AST_DATA_DIR, class->dir);
-	} else {
-		ast_copy_string(dir_path, class->dir, sizeof(dir_path));
+static struct ast_vector_string *moh_file_vector_alloc(int initial_capacity)
+{
+	struct ast_vector_string *files = ao2_alloc_options(
+		sizeof(struct ast_vector_string),
+		moh_file_vector_destructor,
+		AO2_ALLOC_OPT_LOCK_NOLOCK);
+	if (files) {
+		AST_VECTOR_INIT(files, initial_capacity);
 	}
-	ast_debug(4, "Scanning '%s' for files for class '%s'\n", dir_path, class->name);
-	files_DIR = opendir(dir_path);
-	if (!files_DIR) {
-		ast_log(LOG_WARNING, "Cannot open dir %s or dir does not exist\n", dir_path);
+	return files;
+}
+
+static int moh_scan_files(struct mohclass *class)
+{
+	DIR *dir;
+	struct dirent *entry;
+	struct ast_vector_string *files;
+
+	ast_debug(4, "Scanning '%s' for files for class '%s'\n", class->dir, class->name);
+	dir = opendir(class->dir);
+	if (!dir) {
+		ast_log(LOG_WARNING, "Cannot open '%s': %s\n", class->dir, strerror(errno));
 		return -1;
 	}
 
-	AST_VECTOR_RESET(&class->files, ast_free);
+	files = moh_file_vector_alloc(16); /* 16 seems like a reasonable default */
+	if (!files) {
+		closedir(dir);
+		return -1;
+	}
 
-	while ((files_dirent = readdir(files_DIR))) {
-		char *filepath_copy;
-
-		/* The file name must be at least long enough to have the file type extension */
-		if ((strlen(files_dirent->d_name) < 4))
-			continue;
+	while ((entry = readdir(dir))) {
+		char *path, *extension;
 
 		/* Skip files that starts with a dot */
-		if (files_dirent->d_name[0] == '.')
-			continue;
-
-		/* Skip files without extensions... they are not audio */
-		if (!strchr(files_dirent->d_name, '.'))
-			continue;
-
-		snprintf(filepath, sizeof(filepath), "%s/%s", dir_path, files_dirent->d_name);
-
-		if (stat(filepath, &statbuf))
-			continue;
-
-		if (!S_ISREG(statbuf.st_mode))
-			continue;
-
-		if ((ext = strrchr(filepath, '.')))
-			*ext = '\0';
-
-		/* if the file is present in multiple formats, ensure we only put it into the list once */
-		if (AST_VECTOR_GET_CMP(&class->files, &filepath[0], !strcmp)) {
+		if (entry->d_name[0] == '.') {
 			continue;
 		}
 
-		filepath_copy = ast_strdup(filepath);
-		if (!filepath_copy) {
+		/* The filename must be at least long enough to have the file type extension */
+		if (strlen(entry->d_name) < 4) {
+			continue;
+		}
+
+		/* Skip files without extensions */
+		if (!strrchr(entry->d_name, '.')) {
+			continue;
+		}
+
+		/* Whoever frees the vector is responsible for freeing this string */
+		if (ast_asprintf(&path, "%s/%s", class->dir, entry->d_name) < 0) {
 			break;
 		}
 
+		/* Only regular files are candidates */
+#ifdef _DIRENT_HAVE_D_TYPE
+		if (entry->d_type != DT_UNKNOWN) {
+			if (entry->d_type != DT_REG) {
+				ast_free(path);
+				continue;
+			}
+		} else
+#endif
+		{
+			struct stat statbuf;
+			if (stat(path, &statbuf) || !S_ISREG(statbuf.st_mode)) {
+				ast_free(path);
+				continue;
+			}
+		}
+
+		/* Truncate off the extension */
+		if ((extension = strrchr(path, '.'))) {
+			*extension = '\0';
+		}
+
+		/* If the file is present in multiple formats, ensure we only put it into the list once */
+		if (AST_VECTOR_GET_CMP(files, path, !strcmp)) {
+			ast_free(path);
+			continue;
+		}
+
 		if (ast_test_flag(class, MOH_SORTALPHA)) {
-			res = AST_VECTOR_ADD_SORTED(&class->files, filepath_copy, strcasecmp);
+			if (AST_VECTOR_ADD_SORTED(files, path, strcasecmp)) {
+				ast_free(path);
+				break;
+			}
 		} else {
-			res = AST_VECTOR_APPEND(&class->files, filepath_copy);
-		}
-
-		if (res) {
-			ast_free(filepath_copy);
-			break;
+			if (AST_VECTOR_APPEND(files, path)) {
+				ast_free(path);
+				break;
+			}
 		}
 	}
 
-	closedir(files_DIR);
+	closedir(dir);
 
-	AST_VECTOR_COMPACT(&class->files);
+	AST_VECTOR_COMPACT(files);
 
-	return AST_VECTOR_SIZE(&class->files);
+	ao2_lock(class);
+	ao2_replace(class->files, files);
+	ao2_unlock(class);
+
+	return AST_VECTOR_SIZE(files);
 }
 
 static int init_files_class(struct mohclass *class)
@@ -1428,7 +1472,13 @@
 		class->format = ao2_bump(ast_format_slin);
 		class->srcfd = -1;
 		class->kill_delay = 100000;
-		AST_VECTOR_INIT(&class->files, 0);
+
+		/* We create an empty one by default */
+		class->files = moh_file_vector_alloc(0);
+		if (!class->files) {
+			ao2_ref(class, -1);
+			return NULL;
+		}
 	}
 
 	return class;
@@ -1446,6 +1496,15 @@
 	return var;
 }
 
+static void make_directory_path_absolute(struct mohclass *clz, const char *value)
+{
+	if (*value == '/') {
+		ast_copy_string(clz->dir, value, sizeof(clz->dir));
+	} else {
+		snprintf(clz->dir, sizeof(clz->dir), "%s/%s", ast_config_AST_DATA_DIR, value);
+	}
+}
+
 static int local_ast_moh_start(struct ast_channel *chan, const char *mclass, const char *interpclass)
 {
 	struct mohclass *mohclass = NULL;
@@ -1506,7 +1565,7 @@
 				else if (!strcasecmp(tmp->name, "mode"))
 					ast_copy_string(mohclass->mode, tmp->value, sizeof(mohclass->mode));
 				else if (!strcasecmp(tmp->name, "directory"))
-					ast_copy_string(mohclass->dir, tmp->value, sizeof(mohclass->dir));
+					make_directory_path_absolute(mohclass, tmp->value);
 				else if (!strcasecmp(tmp->name, "application"))
 					ast_copy_string(mohclass->args, tmp->value, sizeof(mohclass->args));
 				else if (!strcasecmp(tmp->name, "digit") && (isdigit(*tmp->value) || strchr("*#", *tmp->value)))
@@ -1570,7 +1629,7 @@
 				mohclass->start -= respawn_time;
 
 				if (!strcasecmp(mohclass->mode, "files")) {
-					if (!moh_scan_files(mohclass)) {
+					if (moh_scan_files(mohclass) <= 0) {
 						mohclass = mohclass_unref(mohclass, "unreffing potential mohclass (moh_scan_files failed)");
 						return -1;
 					}
@@ -1635,14 +1694,20 @@
 
 	/* If we are using a cached realtime class with files, re-scan the files */
 	if (!var && ast_test_flag(global_flags, MOH_CACHERTCLASSES) && mohclass->realtime && !strcasecmp(mohclass->mode, "files")) {
-		if (!moh_scan_files(mohclass)) {
+		if (moh_scan_files(mohclass) <= 0) {
 			mohclass = mohclass_unref(mohclass, "unreffing potential mohclass (moh_scan_files failed)");
 			return -1;
 		}
 	}
 
 	if (!state || !state->class || strcmp(mohclass->name, state->class->name)) {
-		if (AST_VECTOR_SIZE(&mohclass->files)) {
+		size_t file_count;
+
+		ao2_lock(mohclass);
+		file_count = AST_VECTOR_SIZE(mohclass->files);
+		ao2_unlock(mohclass);
+
+		if (file_count) {
 			res = ast_activate_generator(chan, &moh_file_stream, mohclass);
 		} else {
 			res = ast_activate_generator(chan, &mohgen, mohclass);
@@ -1687,6 +1752,7 @@
 	while ((member = AST_LIST_REMOVE_HEAD(&class->members, list))) {
 		ast_free(member);
 	}
+	ao2_cleanup(class->files);
 	ao2_unlock(class);
 
 	/* Kill the thread first, so it cannot restart the child process while the
@@ -1721,9 +1787,6 @@
 		class->srcfd = -1;
 	}
 
-	AST_VECTOR_RESET(&class->files, ast_free);
-	AST_VECTOR_FREE(&class->files);
-
 	if (class->timer) {
 		ast_timer_close(class->timer);
 		class->timer = NULL;
@@ -1817,7 +1880,7 @@
 			if (!strcasecmp(var->name, "mode")) {
 				ast_copy_string(class->mode, var->value, sizeof(class->mode));
 			} else if (!strcasecmp(var->name, "directory")) {
-				ast_copy_string(class->dir, var->value, sizeof(class->dir));
+				make_directory_path_absolute(class, var->value);
 			} else if (!strcasecmp(var->name, "application")) {
 				ast_copy_string(class->args, var->value, sizeof(class->args));
 			} else if (!strcasecmp(var->name, "announcement")) {
@@ -1946,16 +2009,21 @@
 
 	i = ao2_iterator_init(mohclasses, 0);
 	for (; (class = ao2_t_iterator_next(&i, "Show files iterator")); mohclass_unref(class, "Unref iterator in moh show files")) {
-		int x;
+		struct ast_vector_string *files;
 
-		if (!AST_VECTOR_SIZE(&class->files)) {
-			continue;
+		ao2_lock(class);
+		files = ao2_bump(class->files);
+		ao2_unlock(class);
+
+		if (AST_VECTOR_SIZE(files)) {
+			int x;
+			ast_cli(a->fd, "Class: %s\n", class->name);
+			for (x = 0; x < AST_VECTOR_SIZE(files); x++) {
+				ast_cli(a->fd, "\tFile: %s\n", AST_VECTOR_GET(files, x));
+			}
 		}
 
-		ast_cli(a->fd, "Class: %s\n", class->name);
-		for (x = 0; x < AST_VECTOR_SIZE(&class->files); x++) {
-			ast_cli(a->fd, "\tFile: %s\n", AST_VECTOR_GET(&class->files, x));
-		}
+		ao2_ref(files, -1);
 	}
 	ao2_iterator_destroy(&i);
 

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/14684
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I479c5dcf88db670956e8cac177b5826c986b0217
Gerrit-Change-Number: 14684
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200720/fd35ca63/attachment-0001.html>


More information about the asterisk-code-review mailing list