[Asterisk-code-review] res_musiconhold: Use stringfields instead of fixed sized buffers (...asterisk[master])
Sean Bright
asteriskteam at digium.com
Wed Jul 31 14:22:53 CDT 2019
Sean Bright has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/11661
Change subject: res_musiconhold: Use stringfields instead of fixed sized buffers
......................................................................
res_musiconhold: Use stringfields instead of fixed sized buffers
Change-Id: Icc6dd695cf254de2fc9968fa3bc7330c24cc7bb8
---
M res/res_musiconhold.c
1 file changed, 89 insertions(+), 43 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/61/11661/1
diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c
index a1352c2..d94da4c 100644
--- a/res/res_musiconhold.c
+++ b/res/res_musiconhold.c
@@ -126,7 +126,15 @@
static int respawn_time = 20;
+#define MOH_STATE_STRFIELD_INIT 256
+#define MOH_CLASS_STRFIELD_INIT 512
+
+/* If you add any fields to this struct, update moh_files_state_reset appropriately */
struct moh_files_state {
+ AST_DECLARE_STRING_FIELDS(
+ AST_STRING_FIELD(name);
+ AST_STRING_FIELD(save_pos_filename);
+ );
/*! Holds a reference to the MOH class. */
struct mohclass *class;
struct ast_format *origwfmt;
@@ -137,8 +145,6 @@
int pos;
int save_pos;
int save_total;
- char name[MAX_MUSICCLASS];
- char save_pos_filename[PATH_MAX];
};
#define MOH_QUIET (1 << 0)
@@ -165,11 +171,13 @@
};
struct mohclass {
- char name[MAX_MUSICCLASS];
- char dir[256];
- char args[256];
- char announcement[256];
- char mode[80];
+ AST_DECLARE_STRING_FIELDS(
+ AST_STRING_FIELD(name);
+ AST_STRING_FIELD(dir);
+ AST_STRING_FIELD(args);
+ AST_STRING_FIELD(announcement);
+ AST_STRING_FIELD(mode);
+ );
char digit;
/*! A vector of filenames in "files" mode */
struct ast_vector_string files;
@@ -378,7 +386,7 @@
}
/* 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_string_field_set(state, save_pos_filename, AST_VECTOR_GET(&state->class->files, state->pos));
ast_debug(1, "%s Opened file %d '%s'\n", ast_channel_name(chan), state->pos, state->save_pos_filename);
@@ -484,6 +492,22 @@
return res;
}
+static void moh_files_state_reset(struct moh_files_state *state)
+{
+ ast_string_field_init(state, 0);
+ state->class = NULL; /* Unref'd elsewhere */
+ ao2_cleanup(state->origwfmt);
+ state->origwfmt = NULL;
+ ao2_cleanup(state->mohwfmt);
+ state->mohwfmt = NULL;
+ state->announcement = 0;
+ state->samples = 0;
+ state->sample_queue = 0;
+ state->pos = 0;
+ state->save_pos = 0;
+ state->save_total = 0;
+}
+
static void *moh_files_alloc(struct ast_channel *chan, void *params)
{
struct moh_files_state *state;
@@ -491,7 +515,7 @@
size_t file_count;
state = ast_channel_music_state(chan);
- if (!state && (state = ast_calloc(1, sizeof(*state)))) {
+ if (!state && (state = ast_calloc_with_stringfields(1, struct moh_files_state, MOH_STATE_STRFIELD_INIT))) {
ast_channel_music_state_set(chan, state);
ast_module_ref(ast_module_info->self);
} else {
@@ -509,9 +533,7 @@
/* 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) {
/* Start MOH from scratch. */
- ao2_cleanup(state->origwfmt);
- ao2_cleanup(state->mohwfmt);
- memset(state, 0, sizeof(*state));
+ moh_files_state_reset(state);
if (ast_test_flag(class, MOH_RANDOMIZE) && file_count) {
state->pos = ast_random() % file_count;
}
@@ -522,7 +544,7 @@
ao2_replace(state->origwfmt, ast_channel_writeformat(chan));
ao2_replace(state->mohwfmt, ast_channel_writeformat(chan));
/* For comparison on restart of MOH (see above) */
- ast_copy_string(state->name, class->name, sizeof(state->name));
+ ast_string_field_set(state, name, class->name);
state->save_total = file_count;
moh_post_start(chan, class->name);
@@ -547,14 +569,14 @@
static void moh_handle_digit(struct ast_channel *chan, char digit)
{
struct mohclass *class;
- const char *classname = NULL;
if ((class = get_mohbydigit(digit))) {
- classname = ast_strdupa(class->name);
+ char *classname = ast_strdup(class->name);
class = mohclass_unref(class, "Unreffing ao2_find from finding by digit");
ast_channel_musicclass_set(chan, classname);
ast_moh_stop(chan);
ast_moh_start(chan, classname, NULL);
+ ast_free(classname);
}
}
@@ -925,7 +947,11 @@
.flags = 0,
};
- ast_copy_string(tmp_class.name, name, sizeof(tmp_class.name));
+ if (ast_string_field_init(&tmp_class, MOH_CLASS_STRFIELD_INIT)) {
+ return NULL;
+ }
+
+ ast_string_field_set(&tmp_class, name, name);
moh = __ao2_find(mohclasses, &tmp_class, flags,
"get_mohbyname", file, lineno, funcname);
@@ -1008,7 +1034,7 @@
/* Initiating music_state for current channel. Channel should know name of moh class */
state = ast_channel_music_state(chan);
- if (!state && (state = ast_calloc(1, sizeof(*state)))) {
+ if (!state && (state = ast_calloc_with_stringfields(1, struct moh_files_state, MOH_STATE_STRFIELD_INIT))) {
ast_channel_music_state_set(chan, state);
ast_module_ref(ast_module_info->self);
} else {
@@ -1019,9 +1045,7 @@
mohclass_unref(state->class, "Uh Oh. Restarting MOH with an active class");
ast_log(LOG_WARNING, "Uh Oh. Restarting MOH with an active class\n");
}
- ao2_cleanup(state->origwfmt);
- ao2_cleanup(state->mohwfmt);
- memset(state, 0, sizeof(*state));
+ moh_files_state_reset(state);
}
if ((res = mohalloc(class))) {
@@ -1078,15 +1102,15 @@
{
for (; var; var = var->next) {
if (!strcasecmp(var->name, "name")) {
- ast_copy_string(mohclass->name, var->value, sizeof(mohclass->name));
+ ast_string_field_set(mohclass, name, var->value);
} else if (!strcasecmp(var->name, "mode")) {
- ast_copy_string(mohclass->mode, var->value, sizeof(mohclass->mode));
+ ast_string_field_set(mohclass, mode, var->value);
} else if (!strcasecmp(var->name, "directory")) {
- ast_copy_string(mohclass->dir, var->value, sizeof(mohclass->dir));
+ ast_string_field_set(mohclass, dir, var->value);
} else if (!strcasecmp(var->name, "application")) {
- ast_copy_string(mohclass->args, var->value, sizeof(mohclass->args));
+ ast_string_field_set(mohclass, args, var->value);
} else if (!strcasecmp(var->name, "announcement")) {
- ast_copy_string(mohclass->announcement, var->value, sizeof(mohclass->announcement));
+ ast_string_field_set(mohclass, announcement, var->value);
ast_set_flag(mohclass, MOH_ANNOUNCEMENT);
} else if (!strcasecmp(var->name, "digit") && (isdigit(*var->value) || strchr("*#", *var->value))) {
mohclass->digit = *var->value;
@@ -1136,27 +1160,35 @@
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;
+ char *dir_path;
if (class->dir[0] != '/') {
- snprintf(dir_path, sizeof(dir_path), "%s/%s", ast_config_AST_DATA_DIR, class->dir);
+ if (ast_asprintf(&dir_path, "%s/%s", ast_config_AST_DATA_DIR, class->dir) < 0) {
+ dir_path = NULL;
+ }
} else {
- ast_copy_string(dir_path, class->dir, sizeof(dir_path));
+ dir_path = ast_strdup(class->dir);
}
+
+ if (!dir_path) {
+ return -1;
+ }
+
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_free(dir_path);
return -1;
}
AST_VECTOR_RESET(&class->files, ast_free);
while ((files_dirent = readdir(files_DIR))) {
+ struct stat statbuf;
+ char *filepath, *ext;
+ int res;
+
/* The file name must be at least long enough to have the file type extension */
if ((strlen(files_dirent->d_name) < 4))
continue;
@@ -1169,34 +1201,41 @@
if (!strchr(files_dirent->d_name, '.'))
continue;
- snprintf(filepath, sizeof(filepath), "%s/%s", dir_path, files_dirent->d_name);
+ /* We transfer this heap allocated buffer to the vector, so in the optimal case
+ we will not free it here */
+ if (ast_asprintf(&filepath, "%s/%s", dir_path, files_dirent->d_name) < 0) {
+ break;
+ }
- if (stat(filepath, &statbuf))
+ if (stat(filepath, &statbuf) || !S_ISREG(statbuf.st_mode)) {
+ ast_free(filepath);
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 (AST_VECTOR_GET_CMP(&class->files, filepath, !strcmp)) {
+ ast_free(filepath);
continue;
}
if (ast_test_flag(class, MOH_SORTALPHA)) {
- res = AST_VECTOR_ADD_SORTED(&class->files, ast_strdup(filepath), strcasecmp);
+ res = AST_VECTOR_ADD_SORTED(&class->files, filepath, strcasecmp);
} else {
- res = AST_VECTOR_APPEND(&class->files, ast_strdup(filepath));
+ res = AST_VECTOR_APPEND(&class->files, filepath);
}
if (res) {
+ /* Adding to the vector failed, so it's our responsibility to clean up filepath */
+ ast_free(filepath);
break;
}
}
closedir(files_DIR);
+ ast_free(dir_path);
AST_VECTOR_COMPACT(&class->files);
@@ -1365,6 +1404,7 @@
}
ao2_cleanup(state->origwfmt);
ao2_cleanup(state->mohwfmt);
+ ast_string_field_free_memory(state);
ast_free(state);
/* Only held a module reference if we had a music state */
ast_module_unref(ast_module_info->self);
@@ -1382,6 +1422,10 @@
class = __ao2_alloc(sizeof(*class), moh_class_destructor, AO2_ALLOC_OPT_LOCK_MUTEX,
"Allocating new moh class", file, line, funcname);
if (class) {
+ if (ast_string_field_init(class, MOH_CLASS_STRFIELD_INIT)) {
+ ao2_cleanup(class);
+ return NULL;
+ }
class->format = ao2_bump(ast_format_slin);
class->srcfd = -1;
class->kill_delay = 100000;
@@ -1461,7 +1505,7 @@
if (ast_strlen_zero(mohclass->dir)) {
if (!strcasecmp(mohclass->mode, "custom")) {
- strcpy(mohclass->dir, "nodir");
+ ast_string_field_set(mohclass, dir, "nodir");
} else {
ast_log(LOG_WARNING, "A directory must be specified for class '%s'!\n", mohclass->name);
mohclass = mohclass_unref(mohclass, "unreffing potential mohclass (no directory specified)");
@@ -1669,6 +1713,8 @@
ao2_cleanup(class->format);
+ ast_string_field_free_memory(class);
+
/* Finally, collect the exit status of the monitor thread */
if (tid > 0) {
pthread_join(tid, NULL);
@@ -1752,11 +1798,11 @@
moh_parse_options(ast_variable_browse(cfg, cat), class);
/* For compatibility with the past, we overwrite any name=name
* with the context [name]. */
- ast_copy_string(class->name, cat, sizeof(class->name));
+ ast_string_field_set(class, name, cat);
if (ast_strlen_zero(class->dir)) {
if (!strcasecmp(class->mode, "custom")) {
- strcpy(class->dir, "nodir");
+ ast_string_field_set(class, dir, "nodir");
} else {
ast_log(LOG_WARNING, "A directory must be specified for class '%s'!\n", class->name);
class = mohclass_unref(class, "unreffing potential mohclass (no directory)");
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11661
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Icc6dd695cf254de2fc9968fa3bc7330c24cc7bb8
Gerrit-Change-Number: 11661
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/20190731/40847a6c/attachment-0001.html>
More information about the asterisk-code-review
mailing list