[Asterisk-code-review] res_musiconhold.c: Prevent crash with realtime MoH (asterisk[16])
Sean Bright
asteriskteam at digium.com
Mon Jul 20 13:09:21 CDT 2020
Sean Bright has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/14686 )
Change subject: res_musiconhold.c: Prevent crash with realtime MoH
......................................................................
res_musiconhold.c: 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, 188 insertions(+), 87 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/86/14686/1
diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c
index 01a14b9..faeeaa3 100644
--- a/res/res_musiconhold.c
+++ b/res/res_musiconhold.c
@@ -166,13 +166,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" or "playlist" mode */
+ struct ast_vector_string *files;
unsigned int flags;
/*! The format from the MOH source, not applicable to "files" mode */
struct ast_format *format;
@@ -313,6 +313,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;
@@ -332,16 +333,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;
@@ -349,7 +355,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;
}
}
@@ -364,21 +370,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);
@@ -395,6 +402,7 @@
}
}
+ ao2_ref(files, -1);
return 0;
}
@@ -504,7 +512,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) {
@@ -1074,8 +1084,38 @@
.digit = moh_handle_digit,
};
+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 void moh_file_vector_destructor(void *obj)
+{
+ struct ast_vector_string *files = obj;
+ AST_VECTOR_RESET(files, ast_free);
+ AST_VECTOR_FREE(files);
+}
+
+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);
+ }
+ return files;
+}
+
static void moh_parse_options(struct ast_variable *var, struct mohclass *mohclass)
{
+ struct ast_vector_string *playlist_entries = NULL;
+
for (; var; var = var->next) {
if (!strcasecmp(var->name, "name")) {
ast_copy_string(mohclass->name, var->value, sizeof(mohclass->name));
@@ -1083,7 +1123,16 @@
ast_copy_string(mohclass->mode, var->value, sizeof(mohclass->mode));
} else if (!strcasecmp(var->name, "entry")) {
if (ast_begins_with(var->value, "/") || ast_begins_with(var->value, "http://") || ast_begins_with(var->value, "https://")) {
- char *dup = ast_strdup(var->value);
+ char *dup;
+
+ if (!playlist_entries) {
+ playlist_entries = moh_file_vector_alloc(16);
+ if (!playlist_entries) {
+ continue;
+ }
+ }
+
+ dup = ast_strdup(var->value);
if (!dup) {
continue;
}
@@ -1096,12 +1145,13 @@
dup);
}
}
- AST_VECTOR_APPEND(&mohclass->files, dup);
+
+ AST_VECTOR_APPEND(playlist_entries, dup);
} else {
ast_log(LOG_ERROR, "Playlist entries must be a URL or absolute path, '%s' provided.\n", var->value);
}
} else if (!strcasecmp(var->name, "directory")) {
- ast_copy_string(mohclass->dir, var->value, sizeof(mohclass->dir));
+ make_directory_path_absolute(mohclass, var->value);
} else if (!strcasecmp(var->name, "application")) {
ast_copy_string(mohclass->args, var->value, sizeof(mohclass->args));
} else if (!strcasecmp(var->name, "announcement")) {
@@ -1150,86 +1200,110 @@
}
}
- AST_VECTOR_COMPACT(&mohclass->files);
+ if (playlist_entries) {
+ /* If we aren't in playlist mode, drop any list we may have already built */
+ if (strcasecmp(mohclass->mode, "playlist")) {
+ ao2_ref(playlist_entries, -1);
+ return;
+ }
+
+ AST_VECTOR_COMPACT(playlist_entries);
+ ao2_replace(mohclass->files, playlist_entries);
+ }
}
-static int moh_scan_files(struct mohclass *class) {
+static int moh_scan_files(struct mohclass *class)
+{
+ DIR *dir;
+ struct dirent *entry;
+ struct ast_vector_string *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));
- }
- 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);
+ 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)
@@ -1355,7 +1429,13 @@
return -1;
}
} else if (!strcasecmp(moh->mode, "playlist")) {
- if (!AST_VECTOR_SIZE(&moh->files)) {
+ size_t file_count;
+
+ ao2_lock(moh);
+ file_count = AST_VECTOR_SIZE(moh->files);
+ ao2_unlock(moh);
+
+ if (!file_count) {
if (unref) {
moh = mohclass_unref(moh, "unreffing potential new moh class (no playlist entries)");
}
@@ -1504,7 +1584,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;
@@ -1647,7 +1733,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;
}
@@ -1660,7 +1746,13 @@
ast_set_flag(mohclass, MOH_RANDOMIZE);
}
} else if (!strcasecmp(mohclass->mode, "playlist")) {
- 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) {
mohclass = mohclass_unref(mohclass, "unreffing potential mohclass (no playlist entries)");
return -1;
}
@@ -1723,14 +1815,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);
@@ -1775,6 +1873,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
@@ -1809,9 +1908,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;
@@ -1993,16 +2089,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/+/14686
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I479c5dcf88db670956e8cac177b5826c986b0217
Gerrit-Change-Number: 14686
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/932f91ed/attachment-0001.html>
More information about the asterisk-code-review
mailing list