<p>Corey Farrell has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/7241">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">acl: Fix allocation related issues.<br><br>Add checks for allocation errors, cleanup and report failure when they<br>occur.<br><br>* ast_duplicate_acl_list: Replace log warnings with errors, add missing<br>  line-feed.<br>* ast_append_acl: Add missing line-feed to logger message.<br>* ast_append_ha: Avoid ast_strdupa in loop by moving debug message to<br>  separate function.<br>* ast_ha_join: Use two separate calls to ast_str_append to avoid using<br>  ast_strdupa in a loop.<br><br>Change-Id: Ia19eaaeb0b139ff7ce7b971c7550e85c8b78ab76<br>---<br>M main/acl.c<br>1 file changed, 47 insertions(+), 12 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/41/7241/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/main/acl.c b/main/acl.c<br>index 6868ea1..6b1fbbb 100644<br>--- a/main/acl.c<br>+++ b/main/acl.c<br>@@ -281,6 +281,12 @@<br> <br>    while (start) {<br>               current = ast_duplicate_ha(start);  /* Create copy of this object */<br>+         if (!current) {<br>+                      ast_free_ha(ret);<br>+<br>+                 return NULL;<br>+         }<br>+<br>          if (prev) {<br>                   prev->next = current;           /* Link previous to this object */<br>                 }<br>@@ -318,7 +324,7 @@<br>        }<br> <br>  if (!(clone = ast_calloc(1, sizeof(*clone)))) {<br>-              ast_log(LOG_WARNING, "Failed to allocate ast_acl_list struct while cloning an ACL\n");<br>+             ast_log(LOG_ERROR, "Failed to allocate ast_acl_list struct while cloning an ACL\n");<br>                return NULL;<br>  }<br>     AST_LIST_HEAD_INIT(clone);<br>@@ -327,8 +333,10 @@<br> <br>   AST_LIST_TRAVERSE(original, current_cursor, list) {<br>           if ((acl_new(&current_clone, current_cursor->name))) {<br>-                        ast_log(LOG_WARNING, "Failed to allocate ast_acl struct while cloning an ACL.");<br>-                   continue;<br>+                    ast_log(LOG_ERROR, "Failed to allocate ast_acl struct while cloning an ACL.\n");<br>+                   ast_free_acl_list(clone);<br>+                    clone = NULL;<br>+                        break;<br>                }<br> <br>          /* Copy data from original ACL to clone ACL */<br>@@ -338,6 +346,15 @@<br>          current_clone->is_realtime = current_cursor->is_realtime;<br> <br>            AST_LIST_INSERT_TAIL(clone, current_clone, list);<br>+<br>+         if (current_cursor->acl && !current_clone->acl) {<br>+                      /* Deal with failure after adding to clone so we don't have to free<br>+                       * current_clone separately. */<br>+                      ast_log(LOG_ERROR, "Failed to duplicate HA list while cloning ACL.\n");<br>+                    ast_free_acl_list(clone);<br>+                    clone = NULL;<br>+                        break;<br>+               }<br>     }<br> <br>  AST_LIST_UNLOCK(original);<br>@@ -448,6 +465,8 @@<br>                               if (error) {<br>                                  *error = 1;<br>                           }<br>+                            AST_LIST_UNLOCK(working_list);<br>+                               return;<br>                       }<br>                     // Need to INSERT the ACL at the head here.<br>                   AST_LIST_INSERT_HEAD(working_list, acl, list);<br>@@ -457,6 +476,7 @@<br>           }<br> <br>          /* With the proper ACL set for modification, we can just pass this off to the ast_ha append function. */<br>+             /* If this fails we set error, is that good enough? */<br>                acl->acl = ast_append_ha(sense, stuff, acl->acl, error);<br> <br>             AST_LIST_UNLOCK(working_list);<br>@@ -477,7 +497,8 @@<br>           AST_LIST_TRAVERSE(working_list, current, list) {<br>                      if (!strcasecmp(current->name, tmp)) { /* ACL= */<br>                          /* Inclusion of the same ACL multiple times isn't a catastrophic error, but it will raise the error flag and skip the entry. */<br>-                          ast_log(LOG_ERROR, "Named ACL '%s' occurs multiple times in ACL definition. Please update your ACL configuration.", tmp);<br>+                          ast_log(LOG_ERROR, "Named ACL '%s' occurs multiple times in ACL definition. "<br>+                                                 "Please update your ACL configuration.\n", tmp);<br>                                 if (error) {<br>                                  *error = 1;<br>                           }<br>@@ -534,6 +555,20 @@<br>       }<br> <br>  return 1;<br>+}<br>+<br>+/*!<br>+ * \internal<br>+ * \brief Used by ast_append_ha to avoid ast_strdupa in a loop.<br>+ */<br>+static void debug_ha_sense_appended(struct ast_ha *ha)<br>+{<br>+   const char *parsed_mask = ast_strdupa(ast_sockaddr_stringify(&ha->netmask));<br>+<br>+       ast_debug(3, "%s/%s sense %u appended to ACL\n",<br>+           ast_sockaddr_stringify(&ha->addr),<br>+            parsed_mask,<br>+         ha->sense);<br> }<br> <br> struct ast_ha *ast_append_ha(const char *sense, const char *stuff, struct ast_ha *path, int *error)<br>@@ -653,10 +688,9 @@<br>             }<br>             prev = ha;<br> <br>-                parsed_addr = ast_strdupa(ast_sockaddr_stringify(&ha->addr));<br>-         parsed_mask = ast_strdupa(ast_sockaddr_stringify(&ha->netmask));<br>-<br>-           ast_debug(3, "%s/%s sense %u appended to ACL\n", parsed_addr, parsed_mask, ha->sense);<br>+          if (DEBUG_ATLEAST(3)) {<br>+                      debug_ha_sense_added();<br>+              }<br>     }<br> <br>  return ret;<br>@@ -665,10 +699,11 @@<br> void ast_ha_join(const struct ast_ha *ha, struct ast_str **buf)<br> {<br>      for (; ha; ha = ha->next) {<br>-               const char *addr = ast_strdupa(ast_sockaddr_stringify_addr(&ha->addr));<br>-               ast_str_append(buf, 0, "%s%s/%s",<br>-                         ha->sense == AST_SENSE_ALLOW ? "!" : "",<br>-                              addr, ast_sockaddr_stringify_addr(&ha->netmask));<br>+              ast_str_append(buf, 0, "%s%s/",<br>+                    ha->sense == AST_SENSE_ALLOW ? "!" : "",<br>+                      ast_sockaddr_stringify_addr(&ha->addr));<br>+              /* Separated to avoid duplicating stringified addresses. */<br>+          ast_str_append(buf, 0, "/%s", ast_sockaddr_stringify_addr(&ha->netmask));<br>            if (ha->next) {<br>                    ast_str_append(buf, 0, ",");<br>                }<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/7241">change 7241</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/7241"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: Ia19eaaeb0b139ff7ce7b971c7550e85c8b78ab76 </div>
<div style="display:none"> Gerrit-Change-Number: 7241 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </div>