From a04d946eaa36e1a1cc414fc02147379768a96120 Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Mon, 19 Oct 2015 15:27:40 -0500 Subject: [PATCH 1/2] Add missing failure checks to ast_str_set_va() callers. Change-Id: I0c2cdcd53727bdc6634095c61294807255bd278f --- main/manager.c | 12 ++++++++++-- main/xmldoc.c | 6 +++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/main/manager.c b/main/manager.c index 2ea6fae4cc..6e9ae00104 100644 --- a/main/manager.c +++ b/main/manager.c @@ -2819,6 +2819,7 @@ AST_THREADSTORAGE(userevent_buf); */ void astman_append(struct mansession *s, const char *fmt, ...) { + int res; va_list ap; struct ast_str *buf; @@ -2827,8 +2828,11 @@ void astman_append(struct mansession *s, const char *fmt, ...) } va_start(ap, fmt); - ast_str_set_va(&buf, 0, fmt, ap); + res = ast_str_set_va(&buf, 0, fmt, ap); va_end(ap); + if (res == AST_DYNSTR_BUILD_FAILED) { + return; + } if (s->f != NULL || s->session->f != NULL) { send_string(s, ast_str_buffer(buf)); @@ -2888,6 +2892,7 @@ void astman_send_error(struct mansession *s, const struct message *m, char *erro void astman_send_error_va(struct mansession *s, const struct message *m, const char *fmt, ...) { + int res; va_list ap; struct ast_str *buf; char *msg; @@ -2897,8 +2902,11 @@ void astman_send_error_va(struct mansession *s, const struct message *m, const c } va_start(ap, fmt); - ast_str_set_va(&buf, 0, fmt, ap); + res = ast_str_set_va(&buf, 0, fmt, ap); va_end(ap); + if (res == AST_DYNSTR_BUILD_FAILED) { + return; + } /* astman_append will use the same underlying buffer, so copy the message out * before sending the response */ diff --git a/main/xmldoc.c b/main/xmldoc.c index 399a7be978..86c3f65121 100644 --- a/main/xmldoc.c +++ b/main/xmldoc.c @@ -2646,14 +2646,18 @@ struct ast_xml_xpath_results *__attribute__((format(printf, 1, 2))) ast_xmldoc_q struct documentation_tree *doctree; RAII_VAR(struct ast_str *, xpath_str, ast_str_create(128), ast_free); va_list ap; + int res; if (!xpath_str) { return NULL; } va_start(ap, fmt); - ast_str_set_va(&xpath_str, 0, fmt, ap); + res = ast_str_set_va(&xpath_str, 0, fmt, ap); va_end(ap); + if (res == AST_DYNSTR_BUILD_FAILED) { + return NULL; + } AST_RWLIST_RDLOCK(&xmldoc_tree); AST_LIST_TRAVERSE(&xmldoc_tree, doctree, entry) { From 1ce62b2545b08c2136ee35d9be6fad5c04f81ec0 Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Mon, 19 Oct 2015 15:28:46 -0500 Subject: [PATCH 2/2] strings.c: Fix __ast_str_helper() to always return a terminated string. Users of functions which call __ast_str_helper() such as the ones listed below are likely to not check the return value for failure so ensuring that the string is always nil terminated is a good safety measure. ast_str_set_va() ast_str_append_va() ast_str_set() ast_str_append() Change-Id: I36ab2d14bb6015868b49329dda8639d70fbcae07 --- main/strings.c | 105 ++++++++++++++++++++++++++++++------------------- 1 file changed, 64 insertions(+), 41 deletions(-) diff --git a/main/strings.c b/main/strings.c index 7aaff7992a..495011ec5c 100644 --- a/main/strings.c +++ b/main/strings.c @@ -60,55 +60,78 @@ int __ast_str_helper(struct ast_str **buf, ssize_t max_len, int append, const char *fmt, va_list ap) #endif { - int res, need; + int res; + int added; + int need; int offset = (append && (*buf)->__AST_STR_LEN) ? (*buf)->__AST_STR_USED : 0; va_list aq; + if (max_len < 0) { + max_len = (*buf)->__AST_STR_LEN; /* don't exceed the allocated space */ + } + do { - if (max_len < 0) { - max_len = (*buf)->__AST_STR_LEN; /* don't exceed the allocated space */ - } - /* - * Ask vsnprintf how much space we need. Remember that vsnprintf - * does not count the final '\\0' so we must add 1. - */ va_copy(aq, ap); res = vsnprintf((*buf)->__AST_STR_STR + offset, (*buf)->__AST_STR_LEN - offset, fmt, aq); - - need = res + offset + 1; - /* - * If there is not enough space and we are below the max length, - * reallocate the buffer and return a message telling to retry. - */ - if (need > (*buf)->__AST_STR_LEN && (max_len == 0 || (*buf)->__AST_STR_LEN < max_len) ) { - int len = (int)(*buf)->__AST_STR_LEN; - if (max_len && max_len < need) { /* truncate as needed */ - need = max_len; - } else if (max_len == 0) { /* if unbounded, give more room for next time */ - need += 16 + need / 4; - } - if ( -#if (defined(MALLOC_DEBUG) && !defined(STANDALONE)) - _ast_str_make_space(buf, need, file, lineno, function) -#else - ast_str_make_space(buf, need) -#endif - ) { - ast_log_safe(LOG_VERBOSE, "failed to extend from %d to %d\n", len, need); - va_end(aq); - return AST_DYNSTR_BUILD_FAILED; - } - (*buf)->__AST_STR_STR[offset] = '\0'; /* Truncate the partial write. */ - - /* Restart va_copy before calling vsnprintf() again. */ - va_end(aq); - continue; - } va_end(aq); - break; + + if (res < 0) { + /* + * vsnprintf write to string failed. + * I don't think this is possible with a memory buffer. + */ + res = AST_DYNSTR_BUILD_FAILED; + added = 0; + break; + } + + /* + * vsnprintf returns how much space we used or would need. + * Remember that vsnprintf does not count the nil terminator + * so we must add 1. + */ + added = res; + need = offset + added + 1; + if (need <= (*buf)->__AST_STR_LEN + || (max_len && max_len <= (*buf)->__AST_STR_LEN)) { + /* + * There was enough room for the string or we are not + * allowed to try growing the string buffer. + */ + break; + } + + /* Reallocate the buffer and try again. */ + if (max_len == 0) { + /* unbounded, give more room for next time */ + need += 16 + need / 4; + } else if (max_len < need) { + /* truncate as needed */ + need = max_len; + } + + if ( +#if (defined(MALLOC_DEBUG) && !defined(STANDALONE)) + _ast_str_make_space(buf, need, file, lineno, function) +#else + ast_str_make_space(buf, need) +#endif + ) { + ast_log_safe(LOG_VERBOSE, "failed to extend from %d to %d\n", + (int) (*buf)->__AST_STR_LEN, need); + + res = AST_DYNSTR_BUILD_FAILED; + break; + } } while (1); - /* update space used, keep in mind the truncation */ - (*buf)->__AST_STR_USED = (res + offset > (*buf)->__AST_STR_LEN) ? (*buf)->__AST_STR_LEN - 1: res + offset; + + /* Update space used, keep in mind truncation may be necessary. */ + (*buf)->__AST_STR_USED = ((*buf)->__AST_STR_LEN <= offset + added) + ? (*buf)->__AST_STR_LEN - 1 + : offset + added; + + /* Ensure that the string is terminated. */ + (*buf)->__AST_STR_STR[(*buf)->__AST_STR_USED] = '\0'; return res; }