From ee08f10d060c9b134139a007e4a23e341e4e4e1f Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Thu, 2 Nov 2017 18:40:20 -0500 Subject: [PATCH] Fix ast_(v)asprintf() malloc failure usage conditions. When (v)asprintf() fails, the state of the allocated buffer is undefined. The library had better not leave an allocated buffer as a result or no one will know to free it. The most likely way it can return failure is for an allocation failure. If the printf conversion fails then you actually have a threading problem which is much worse because another thread modified the parameter values. * Made __ast_asprintf()/__ast_vasprintf() set the returned buffer to NULL on failure. That is much more useful than either an uninitialized pointer or a pointer that has already been freed. Many uses won't have to check for failure to ensure that the buffer won't be double freed or prevent an attempt to free an uninitialized pointer. * stasis.c: Fixed memory leak in multi_object_blob_to_ami() allocated by ast_asprintf(). * ari/resource_bridges.c:ari_bridges_play_helper(): Remove assignment to the wrong thing which is now not needed even if assigning to the right thing. Change-Id: Ib5252fb8850ecf0f78ed0ee2ca0796bda7e91c23 --- apps/app_mixmonitor.c | 2 ++ bridges/bridge_softmix.c | 1 - include/asterisk/utils.h | 8 +++++++- main/db.c | 8 +++++--- main/json.c | 2 +- main/stasis.c | 7 ++++--- main/utils.c | 8 +++++++- res/ari/resource_bridges.c | 1 - res/res_xmpp.c | 7 +++++-- 9 files changed, 31 insertions(+), 13 deletions(-) diff --git a/apps/app_mixmonitor.c b/apps/app_mixmonitor.c index 2922e0cd37..bb47cfc340 100644 --- a/apps/app_mixmonitor.c +++ b/apps/app_mixmonitor.c @@ -813,6 +813,8 @@ static int setup_mixmonitor_ds(struct mixmonitor *mixmonitor, struct ast_channel if (ast_asprintf(datastore_id, "%p", mixmonitor_ds) == -1) { ast_log(LOG_ERROR, "Failed to allocate memory for MixMonitor ID.\n"); + ast_free(mixmonitor_ds); + return -1; } ast_mutex_init(&mixmonitor_ds->lock); diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c index 9197df6fb5..f490967e2a 100644 --- a/bridges/bridge_softmix.c +++ b/bridges/bridge_softmix.c @@ -502,7 +502,6 @@ static int append_source_streams(struct ast_stream_topology *dest, if (ast_asprintf(&stream_clone_name, "%s_%s_%s", SOFTBRIDGE_VIDEO_DEST_PREFIX, channel_name, ast_stream_get_name(stream)) < 0) { - ast_free(stream_clone_name); return -1; } diff --git a/include/asterisk/utils.h b/include/asterisk/utils.h index 423d73b26a..0a12b1d8a5 100644 --- a/include/asterisk/utils.h +++ b/include/asterisk/utils.h @@ -622,7 +622,13 @@ int __ast_vasprintf(char **ret, const char *fmt, va_list ap, const char *file, i DEBUG_CHAOS_RETURN(DEBUG_CHAOS_ALLOC_CHANCE, -1); - if ((res = vasprintf(ret, fmt, ap)) == -1) { + res = vasprintf(ret, fmt, ap); + if (res < 0) { + /* + * *ret is undefined so set to NULL to ensure it is + * initialized to something useful. + */ + *ret = NULL; MALLOC_FAILURE_MSG; } diff --git a/main/db.c b/main/db.c index 94324355f2..fa125d749d 100644 --- a/main/db.c +++ b/main/db.c @@ -191,9 +191,11 @@ static int convert_bdb_to_sqlite3(void) char *cmd; int res; - ast_asprintf(&cmd, "%s/astdb2sqlite3 '%s'\n", ast_config_AST_SBIN_DIR, ast_config_AST_DB); - res = ast_safe_system(cmd); - ast_free(cmd); + res = ast_asprintf(&cmd, "%s/astdb2sqlite3 '%s'\n", ast_config_AST_SBIN_DIR, ast_config_AST_DB); + if (0 <= res) { + res = ast_safe_system(cmd); + ast_free(cmd); + } return res; } diff --git a/main/json.c b/main/json.c index 9004978b46..56df7f4012 100644 --- a/main/json.c +++ b/main/json.c @@ -460,7 +460,7 @@ struct ast_json *ast_json_vstringf(const char *format, va_list args) if (format) { int err = ast_vasprintf(&str, format, args); - if (err > 0) { + if (err >= 0) { ret = json_string(str); ast_free(str); } diff --git a/main/stasis.c b/main/stasis.c index a82e938f4f..6dc5dbf288 100644 --- a/main/stasis.c +++ b/main/stasis.c @@ -1342,7 +1342,7 @@ static struct ast_str *multi_object_blob_to_ami(void *obj) for (type = 0; type < STASIS_UMOS_MAX; ++type) { for (i = 0; i < AST_VECTOR_SIZE(&multi->snapshots[type]); ++i) { - char *name = ""; + char *name = NULL; void *snapshot = AST_VECTOR_GET(&multi->snapshots[type], i); ami_snapshot = NULL; @@ -1352,11 +1352,11 @@ static struct ast_str *multi_object_blob_to_ami(void *obj) switch (type) { case STASIS_UMOS_CHANNEL: - ami_snapshot = ast_manager_build_channel_state_string_prefix(snapshot, name); + ami_snapshot = ast_manager_build_channel_state_string_prefix(snapshot, name ?: ""); break; case STASIS_UMOS_BRIDGE: - ami_snapshot = ast_manager_build_bridge_state_string_prefix(snapshot, name); + ami_snapshot = ast_manager_build_bridge_state_string_prefix(snapshot, name ?: ""); break; case STASIS_UMOS_ENDPOINT: @@ -1367,6 +1367,7 @@ static struct ast_str *multi_object_blob_to_ami(void *obj) ast_str_append(&ami_str, 0, "%s", ast_str_buffer(ami_snapshot)); ast_free(ami_snapshot); } + ast_free(name); } } diff --git a/main/utils.c b/main/utils.c index a73bf9d42a..c553207b67 100644 --- a/main/utils.c +++ b/main/utils.c @@ -2340,7 +2340,13 @@ int __ast_asprintf(const char *file, int lineno, const char *func, char **ret, c va_list ap; va_start(ap, fmt); - if ((res = vasprintf(ret, fmt, ap)) == -1) { + res = vasprintf(ret, fmt, ap); + if (res < 0) { + /* + * *ret is undefined so set to NULL to ensure it is + * initialized to something useful. + */ + *ret = NULL; MALLOC_FAILURE_MSG; } va_end(ap); diff --git a/res/ari/resource_bridges.c b/res/ari/resource_bridges.c index aab78ce189..cd289aa128 100644 --- a/res/ari/resource_bridges.c +++ b/res/ari/resource_bridges.c @@ -392,7 +392,6 @@ static int ari_bridges_play_helper(const char **args_media, if (ast_asprintf(playback_url, "/playbacks/%s", stasis_app_playback_get_id(playback)) == -1) { - playback_url = NULL; ast_ari_response_alloc_failed(response); return -1; } diff --git a/res/res_xmpp.c b/res/res_xmpp.c index 8a06a6c35a..f683557a5a 100644 --- a/res/res_xmpp.c +++ b/res/res_xmpp.c @@ -3909,8 +3909,11 @@ static int fetch_access_token(struct ast_xmpp_client_config *cfg) struct ast_json_error error; RAII_VAR(struct ast_json *, jobj, NULL, ast_json_unref); - ast_asprintf(&cmd, "CURL(%s,client_id=%s&client_secret=%s&refresh_token=%s&grant_type=refresh_token)", - url, cfg->oauth_clientid, cfg->oauth_secret, cfg->refresh_token); + if (ast_asprintf(&cmd, + "CURL(%s,client_id=%s&client_secret=%s&refresh_token=%s&grant_type=refresh_token)", + url, cfg->oauth_clientid, cfg->oauth_secret, cfg->refresh_token) < 0) { + return -1; + } ast_debug(2, "Performing OAuth 2.0 authentication for client '%s' using command: %s\n", cfg->name, cmd);