Merge topic 'fix_oom_crash' into 13

* changes:
  strings.c: Fix __ast_str_helper() to always return a terminated string.
  Add missing failure checks to ast_str_set_va() callers.

3 files changed, 72 insertions, 37 deletions

diff --git a/main/manager.c b/main/manager.c
index 75b1d4d48..8295303c7 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -2811,6 +2811,7 @@ AST_THREADSTORAGE(userevent_buf);
  */
 void astman_append(struct mansession *s, const char *fmt, ...)
 {
+	int res;
 	va_list ap;
 	struct ast_str *buf;
 
@@ -2819,8 +2820,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));
@@ -2880,6 +2884,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;
@@ -2889,8 +2894,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/strings.c b/main/strings.c
index da63cdfc0..53d50954f 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 <code>'\\0'</code> so we must add 1.
-		 */
 		va_copy(aq, ap);
 		res = vsnprintf((*buf)->__AST_STR_STR + offset, (*buf)->__AST_STR_LEN - offset, fmt, aq);
+		va_end(aq);
+
+		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;
+		}
 
-		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.
+		 * vsnprintf returns how much space we used or would need.
+		 * Remember that vsnprintf does not count the nil terminator
+		 * so we must add 1.
 		 */
-		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 (
+		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)
+			_ast_str_make_space(buf, need, file, lineno, function)
 #else
-					ast_str_make_space(buf, need)
+			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. */
+			) {
+			ast_log_safe(LOG_VERBOSE, "failed to extend from %d to %d\n",
+				(int) (*buf)->__AST_STR_LEN, need);
 
-			/* Restart va_copy before calling vsnprintf() again. */
-			va_end(aq);
-			continue;
+			res = AST_DYNSTR_BUILD_FAILED;
+			break;
 		}
-		va_end(aq);
-		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;
 }
diff --git a/main/xmldoc.c b/main/xmldoc.c
index fcf1b2cac..1a04e8168 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) {