[Asterisk-code-review] acl: Fix allocation related issues. (asterisk[master])

[Corey Farrell]

Corey Farrell has uploaded this change for review. ( https://gerrit.asterisk.org/7241


Change subject: acl: Fix allocation related issues.
......................................................................

acl: Fix allocation related issues.

Add checks for allocation errors, cleanup and report failure when they
occur.

* ast_duplicate_acl_list: Replace log warnings with errors, add missing
  line-feed.
* ast_append_acl: Add missing line-feed to logger message.
* ast_append_ha: Avoid ast_strdupa in loop by moving debug message to
  separate function.
* ast_ha_join: Use two separate calls to ast_str_append to avoid using
  ast_strdupa in a loop.

Change-Id: Ia19eaaeb0b139ff7ce7b971c7550e85c8b78ab76
---
M main/acl.c
1 file changed, 47 insertions(+), 12 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/41/7241/1

diff --git a/main/acl.c b/main/acl.c
index 6868ea1..6b1fbbb 100644
--- a/main/acl.c
+++ b/main/acl.c
@@ -281,6 +281,12 @@
 
 	while (start) {
 		current = ast_duplicate_ha(start);  /* Create copy of this object */
+		if (!current) {
+			ast_free_ha(ret);
+
+			return NULL;
+		}
+
 		if (prev) {
 			prev->next = current;           /* Link previous to this object */
 		}
@@ -318,7 +324,7 @@
 	}
 
 	if (!(clone = ast_calloc(1, sizeof(*clone)))) {
-		ast_log(LOG_WARNING, "Failed to allocate ast_acl_list struct while cloning an ACL\n");
+		ast_log(LOG_ERROR, "Failed to allocate ast_acl_list struct while cloning an ACL\n");
 		return NULL;
 	}
 	AST_LIST_HEAD_INIT(clone);
@@ -327,8 +333,10 @@
 
 	AST_LIST_TRAVERSE(original, current_cursor, list) {
 		if ((acl_new(&current_clone, current_cursor->name))) {
-			ast_log(LOG_WARNING, "Failed to allocate ast_acl struct while cloning an ACL.");
-			continue;
+			ast_log(LOG_ERROR, "Failed to allocate ast_acl struct while cloning an ACL.\n");
+			ast_free_acl_list(clone);
+			clone = NULL;
+			break;
 		}
 
 		/* Copy data from original ACL to clone ACL */
@@ -338,6 +346,15 @@
 		current_clone->is_realtime = current_cursor->is_realtime;
 
 		AST_LIST_INSERT_TAIL(clone, current_clone, list);
+
+		if (current_cursor->acl && !current_clone->acl) {
+			/* Deal with failure after adding to clone so we don't have to free
+			 * current_clone separately. */
+			ast_log(LOG_ERROR, "Failed to duplicate HA list while cloning ACL.\n");
+			ast_free_acl_list(clone);
+			clone = NULL;
+			break;
+		}
 	}
 
 	AST_LIST_UNLOCK(original);
@@ -448,6 +465,8 @@
 				if (error) {
 					*error = 1;
 				}
+				AST_LIST_UNLOCK(working_list);
+				return;
 			}
 			// Need to INSERT the ACL at the head here.
 			AST_LIST_INSERT_HEAD(working_list, acl, list);
@@ -457,6 +476,7 @@
 		}
 
 		/* With the proper ACL set for modification, we can just pass this off to the ast_ha append function. */
+		/* If this fails we set error, is that good enough? */
 		acl->acl = ast_append_ha(sense, stuff, acl->acl, error);
 
 		AST_LIST_UNLOCK(working_list);
@@ -477,7 +497,8 @@
 		AST_LIST_TRAVERSE(working_list, current, list) {
 			if (!strcasecmp(current->name, tmp)) { /* ACL= */
 				/* Inclusion of the same ACL multiple times isn't a catastrophic error, but it will raise the error flag and skip the entry. */
-				ast_log(LOG_ERROR, "Named ACL '%s' occurs multiple times in ACL definition. Please update your ACL configuration.", tmp);
+				ast_log(LOG_ERROR, "Named ACL '%s' occurs multiple times in ACL definition. "
+				                   "Please update your ACL configuration.\n", tmp);
 				if (error) {
 					*error = 1;
 				}
@@ -534,6 +555,20 @@
 	}
 
 	return 1;
+}
+
+/*!
+ * \internal
+ * \brief Used by ast_append_ha to avoid ast_strdupa in a loop.
+ */
+static void debug_ha_sense_appended(struct ast_ha *ha)
+{
+	const char *parsed_mask = ast_strdupa(ast_sockaddr_stringify(&ha->netmask));
+
+	ast_debug(3, "%s/%s sense %u appended to ACL\n",
+		ast_sockaddr_stringify(&ha->addr),
+		parsed_mask,
+		ha->sense);
 }
 
 struct ast_ha *ast_append_ha(const char *sense, const char *stuff, struct ast_ha *path, int *error)
@@ -653,10 +688,9 @@
 		}
 		prev = ha;
 
-		parsed_addr = ast_strdupa(ast_sockaddr_stringify(&ha->addr));
-		parsed_mask = ast_strdupa(ast_sockaddr_stringify(&ha->netmask));
-
-		ast_debug(3, "%s/%s sense %u appended to ACL\n", parsed_addr, parsed_mask, ha->sense);
+		if (DEBUG_ATLEAST(3)) {
+			debug_ha_sense_added();
+		}
 	}
 
 	return ret;
@@ -665,10 +699,11 @@
 void ast_ha_join(const struct ast_ha *ha, struct ast_str **buf)
 {
 	for (; ha; ha = ha->next) {
-		const char *addr = ast_strdupa(ast_sockaddr_stringify_addr(&ha->addr));
-		ast_str_append(buf, 0, "%s%s/%s",
-			       ha->sense == AST_SENSE_ALLOW ? "!" : "",
-			       addr, ast_sockaddr_stringify_addr(&ha->netmask));
+		ast_str_append(buf, 0, "%s%s/",
+			ha->sense == AST_SENSE_ALLOW ? "!" : "",
+			ast_sockaddr_stringify_addr(&ha->addr));
+		/* Separated to avoid duplicating stringified addresses. */
+		ast_str_append(buf, 0, "/%s", ast_sockaddr_stringify_addr(&ha->netmask));
 		if (ha->next) {
 			ast_str_append(buf, 0, ",");
 		}

-- 
To view, visit https://gerrit.asterisk.org/7241
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia19eaaeb0b139ff7ce7b971c7550e85c8b78ab76
Gerrit-Change-Number: 7241
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171116/515534cb/attachment.html>