kenmirrors
/
freeswitch
mirror of https://github.com/signalwire/freeswitch.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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.
This commit is contained in:
Peter Wu 2015-11-08 16:26:46 +01:00
parent 91e86ab352
commit b683199688
1 changed files with 68 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

103
src/switch_xml.c
View File

@ -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 */
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 (xml->flags & SWITCH_XML_DUP)
xml->attr[c + 1][l / 2] |= SWITCH_XML_TXTM;
else
xml->attr[c + 1][l / 2] &= ~SWITCH_XML_TXTM;
if (value)
xml->attr[l + 1] = (char *) value; /* set attribute value */
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 */
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 */
if (alloc_flags[attr_number] & SWITCH_XML_TXTM)
free(xml->attr[c + 1]); /* old val */
if (xml->flags & SWITCH_XML_DUP)
alloc_flags[attr_number] |= SWITCH_XML_TXTM;
else
alloc_flags[attr_number] &= ~SWITCH_XML_TXTM;
if (value) {
xml->attr[c + 1] = (char *) value; /* set attribute value */
value = NULL; /* control of memory is transferred */
} else { /* remove attribute */
char **tmp;
/* 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 */
}
xml->flags &= ~SWITCH_XML_DUP; /* clear strdup() flag */
return xml;
}