From b7c22054021cb68fcbd2bc17765e9166bc237629 Mon Sep 17 00:00:00 2001 From: Sean Bright Date: Thu, 6 Aug 2020 12:41:33 -0400 Subject: [PATCH] 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. ASTERISK-28927 #close Change-Id: I479c5dcf88db670956e8cac177b5826c986b0217 --- res/res_musiconhold.c | 170 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 138 insertions(+), 32 deletions(-) diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c index 01a14b93fb..dd8dba5573 100644 --- a/res/res_musiconhold.c +++ b/res/res_musiconhold.c @@ -171,8 +171,8 @@ struct mohclass { 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; @@ -313,6 +313,7 @@ static void moh_files_release(struct ast_channel *chan, void *data) 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 @@ static int ast_moh_files_next(struct ast_channel *chan) 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 @@ static int ast_moh_files_next(struct ast_channel *chan) /* 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 @@ static int ast_moh_files_next(struct ast_channel *chan) } 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 @@ static int ast_moh_files_next(struct ast_channel *chan) } } + ao2_ref(files, -1); return 0; } @@ -504,7 +512,9 @@ static void *moh_files_alloc(struct ast_channel *chan, void *params) } } - 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,29 @@ static struct ast_generator mohgen = { .digit = moh_handle_digit, }; +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 +1114,16 @@ static void moh_parse_options(struct ast_variable *var, struct mohclass *mohclas 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,7 +1136,8 @@ static void moh_parse_options(struct ast_variable *var, struct mohclass *mohclas 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); } @@ -1150,7 +1191,21 @@ static void moh_parse_options(struct ast_variable *var, struct mohclass *mohclas } } - 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")) { + ast_log(LOG_NOTICE, "Ignoring playlist entries because we are in '%s' mode.\n", + mohclass->mode); + ao2_ref(playlist_entries, -1); + return; + } + + AST_VECTOR_COMPACT(playlist_entries); + + /* We don't need to lock here because we are the thread that + * created this mohclass and we haven't published it yet */ + ao2_replace(mohclass->files, playlist_entries); + } } static int moh_scan_files(struct mohclass *class) { @@ -1162,6 +1217,7 @@ static int moh_scan_files(struct mohclass *class) { char *ext; struct stat statbuf; int res; + struct ast_vector_string *files; if (class->dir[0] != '/') { snprintf(dir_path, sizeof(dir_path), "%s/%s", ast_config_AST_DATA_DIR, class->dir); @@ -1175,7 +1231,11 @@ static int moh_scan_files(struct mohclass *class) { return -1; } - AST_VECTOR_RESET(&class->files, ast_free); + files = moh_file_vector_alloc(16); /* 16 seems like a reasonable default */ + if (!files) { + closedir(files_DIR); + return -1; + } while ((files_dirent = readdir(files_DIR))) { char *filepath_copy; @@ -1204,7 +1264,7 @@ static int moh_scan_files(struct mohclass *class) { *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 (AST_VECTOR_GET_CMP(files, &filepath[0], !strcmp)) { continue; } @@ -1214,9 +1274,9 @@ static int moh_scan_files(struct mohclass *class) { } if (ast_test_flag(class, MOH_SORTALPHA)) { - res = AST_VECTOR_ADD_SORTED(&class->files, filepath_copy, strcasecmp); + res = AST_VECTOR_ADD_SORTED(files, filepath_copy, strcasecmp); } else { - res = AST_VECTOR_APPEND(&class->files, filepath_copy); + res = AST_VECTOR_APPEND(files, filepath_copy); } if (res) { @@ -1227,9 +1287,13 @@ static int moh_scan_files(struct mohclass *class) { closedir(files_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 +1419,13 @@ static int _moh_register(struct mohclass *moh, int reload, int unref, const char 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 +1574,13 @@ static struct mohclass *_moh_class_malloc(const char *file, int line, const char 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,6 +1723,14 @@ static int local_ast_moh_start(struct ast_channel *chan, const char *mclass, con mohclass->start -= respawn_time; if (!strcasecmp(mohclass->mode, "files")) { + /* + * XXX moh_scan_files returns -1 if it is unable to open the + * configured directory or there is a memory allocation + * failure. Otherwise it returns the number of files for this music + * class. This check is only checking if the number of files is zero + * and it ignores the -1 case. To avoid a behavior change we keep this + * as-is, but we should address what the 'correct' behavior should be. + */ if (!moh_scan_files(mohclass)) { mohclass = mohclass_unref(mohclass, "unreffing potential mohclass (moh_scan_files failed)"); return -1; @@ -1660,7 +1744,13 @@ static int local_ast_moh_start(struct ast_channel *chan, const char *mclass, con 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,6 +1813,13 @@ static int local_ast_moh_start(struct ast_channel *chan, const char *mclass, con /* 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")) { + /* + * XXX moh_scan_files returns -1 if it is unable to open the configured directory + * or there is a memory allocation failure. Otherwise it returns the number of + * files for this music class. This check is only checking if the number of files + * is zero and it ignores the -1 case. To avoid a behavior change we keep this + * as-is, but we should address what the 'correct' behavior should be. + */ if (!moh_scan_files(mohclass)) { mohclass = mohclass_unref(mohclass, "unreffing potential mohclass (moh_scan_files failed)"); return -1; @@ -1730,7 +1827,13 @@ static int local_ast_moh_start(struct ast_channel *chan, const char *mclass, con } 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 +1878,7 @@ static void moh_class_destructor(void *obj) 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 +1913,6 @@ static void moh_class_destructor(void *obj) 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 +2094,21 @@ static char *handle_cli_moh_show_files(struct ast_cli_entry *e, int cmd, struct 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);