switch_xml_set_attr: fix inconsistent state on error paths

Partially rewrite switch_xml_set_attr to fix memory leaks, uninitialized
argument values and use-after free warnings from Clang static analyzer.

Fixes these problems:

 - Add some comments and a new variable such that the code can more
   easily be audited / understood.
 - Always clear SWITCH_XML_DUP flag even if an error occurred to prevent
   free()'ing static strings on future invocations.
 - Keep the attribute list in a consistent state even if one of the
   memory allocation fails.
 - Keep allocation metadata in a consistent state when shrinking of the
   attribute lists fails. Previously the metadata was not updated,
   resulting in a wrong mapping from attributes to allocation flags.
 - Fix memory leaks when allocations fail.

Previous behavior: invalid memory accesses are possible after a memory
allocation failure, previous attributes may be lost.
New behavior: attributes list is always valid, a new attribute is either
set (or not), attributes can always be removed.

diff --git a/src/switch_xml.c b/src/switch_xml.c
index 43ffabfcc7e98ef4d3e4c6eaa1b487baf458ce4e..1173eb0155b21788d559153c892e80fa633279a1 100644 (file)
--- a/src/switch_xml.c
+++ b/src/switch_xml.c
@@ -2944,58 +2944,91 @@ SWITCH_DECLARE(switch_xml_t) switch_xml_set_txt(switch_xml_t xml, const char *tx
    of NULL will remove the specified attribute.  Returns the tag given */
 SWITCH_DECLARE(switch_xml_t) switch_xml_set_attr(switch_xml_t xml, const char *name, const char *value)
 {
-       int l = 0, c;
+       int l = 0, attr_number, c;
+       /* After adding the first attribute, xml->attr = { name1, val1, name2, val2,
+        * ..., nameN, valN, NULL, alloc_flags }. alloc_flags tracks how the memory
+        * for each attr and value is managed. */
+       char *alloc_flags;
 
        if (!xml)
                return NULL;
        while (xml->attr[l] && strcmp(xml->attr[l], name))
                l += 2;
+       attr_number = l / 2;
+
        if (!xml->attr[l]) {            /* not found, add as new attribute */
                if (!value)
-                       return xml;                     /* nothing to do */
+                       goto cleanup;           /* not found, no need to remove attribute */
                if (xml->attr == SWITCH_XML_NIL) {      /* first attribute */
-                       xml->attr = (char **) malloc(4 * sizeof(char *));
-                       if (!xml->attr)
-                               return NULL;
-                       xml->attr[1] = strdup("");      /* empty list of malloced names/vals */
+                       char ** tmp = (char **) malloc(4 * sizeof(char *));
+                       if (!tmp)
+                               goto cleanup;
+                       xml->attr = tmp;
+                       assert(l == 0);                 /* name of SWITCH_XML_NIL is assumed to be empty */
+                       xml->attr[0] = NULL;    /* terminator */
+                       xml->attr[1] = NULL;    /* empty list of malloced names/vals */
                } else {
                        char **tmp = (char **) realloc(xml->attr, (l + 4) * sizeof(char *));
                        if (!tmp)
-                               return xml;
+                               goto cleanup;
                        xml->attr = tmp;
                }
+               /* Extend list of allocation flags for the new attribute */
+               alloc_flags = (char *)realloc(xml->attr[l + 1], attr_number + 1);
+               if (!alloc_flags)
+                       goto cleanup;
+               alloc_flags[attr_number + 1] = '\0';    /* terminate list */
 
+               /* Add new attribute, the value will be set further below. */
                xml->attr[l] = (char *) name;   /* set attribute name */
-               xml->attr[l + 2] = NULL;        /* null terminate attribute list */
-               xml->attr[l + 3] = (char *) realloc(xml->attr[l + 1], (c = (int) strlen(xml->attr[l + 1])) + 2);
-               strcpy(xml->attr[l + 3] + c, " ");      /* set name/value as not malloced */
+               name = NULL;                                    /* control of memory is transferred */
+               l += 2;
+
+               xml->attr[l] = NULL;                    /* null terminate attribute list */
+               xml->attr[l + 1] = alloc_flags;
                if (xml->flags & SWITCH_XML_DUP)
-                       xml->attr[l + 3][c] = SWITCH_XML_NAMEM;
-       } else if (xml->flags & SWITCH_XML_DUP)
-               free((char *) name);    /* name was strduped */
+                       alloc_flags[attr_number] = SWITCH_XML_NAMEM;
+               else
+                       alloc_flags[attr_number] = ' ';         /* dummy value */
+       } else {
+               while (xml->attr[l]) l += 2;
+               alloc_flags = xml->attr[l + 1];
+       }
+
+       c = 2 * attr_number;            /* index of the current attribute name */
+       /* l points to the index of the terminator name */
 
-       for (c = l; xml->attr[c]; c += 2);      /* find end of attribute list */
-       if (xml->attr[c + 1][l / 2] & SWITCH_XML_TXTM)
-               free(xml->attr[l + 1]); /* old val */
+       if (alloc_flags[attr_number] & SWITCH_XML_TXTM)
+               free(xml->attr[c + 1]); /* old val */
        if (xml->flags & SWITCH_XML_DUP)
-               xml->attr[c + 1][l / 2] |= SWITCH_XML_TXTM;
+               alloc_flags[attr_number] |= SWITCH_XML_TXTM;
        else
-               xml->attr[c + 1][l / 2] &= ~SWITCH_XML_TXTM;
+               alloc_flags[attr_number] &= ~SWITCH_XML_TXTM;
 
-       if (value)
-               xml->attr[l + 1] = (char *) value;      /* set attribute value */
-       else {                                          /* remove attribute */
+       if (value) {
+               xml->attr[c + 1] = (char *) value;      /* set attribute value */
+               value = NULL;                           /* control of memory is transferred */
+       } else {                                                /* remove attribute */
                char **tmp;
-               if (xml->attr[c + 1][l / 2] & SWITCH_XML_NAMEM)
-                       free(xml->attr[l]);
-               memmove(xml->attr + l, xml->attr + l + 2, (c - l + 2) * sizeof(char *));
-               tmp = (char **) realloc(xml->attr, (c + 2) * sizeof(char *));
-               if (!tmp)
-                       return xml;
-               xml->attr = tmp;
-               memmove(xml->attr[c + 1] + (l / 2), xml->attr[c + 1] + (l / 2) + 1, (c / 2) - (l / 2)); /* fix list of which name/vals are malloced */
-       }
-       xml->flags &= ~SWITCH_XML_DUP;  /* clear strdup() flag */
+               /* free name if it was dynamically allocated (value is handled above) */
+               if (alloc_flags[attr_number] & SWITCH_XML_NAMEM)
+                       free(xml->attr[c]);
+
+               /* drop (name, value) pair from attribute list */
+               memmove(xml->attr + c, xml->attr + c + 2, (l - c) * sizeof(char *));
+               tmp = (char **) realloc(xml->attr, l * sizeof(char *));
+               if (tmp)                        /* try to shrink when possible */
+                       xml->attr = tmp;
+
+               memmove(alloc_flags + attr_number, alloc_flags + attr_number + 1, (l - c) / 2); /* shrink allocation info */
+       }
+
+cleanup:
+       if (xml->flags & SWITCH_XML_DUP) {
+               free((char *) name);    /* name was strduped */
+               free((char *) value);   /* name was strduped, but an error occurred */
+               xml->flags &= ~SWITCH_XML_DUP;  /* clear strdup() flag */
+       }
 
        return xml;
 }