A new version of Coverity found a number of issues:
get_pool_from_xml(): FORWARD_NULL parse_disk_pool(): RESOURCE_LEAK - If parse_disk_pool() returned name == NULL, then the strdup() of name into pool->id would dereference the NULL pointer leading to a segfault.
Furthermore parse_disk_pool() had a few issues. First the 'type_str' could be NULL, so that needs to be checked. Second, 'type_str' was never free()'d (the get_attr_value returns a strdup()'d value).
Realizing all that resulted in a few extra changes to not strdup() a value that we strdup()'d
Eventually get_pool_from_xml() will return to get_disk_pool() which returns to diskpool_set_capacity() or _new_volume_template() which both error out when return value != 0 (although, I did change the latter to be more explicit and match the former).
Finally, even though the parsing of the element will only ever have one "name" value - Coverity doesn't know that, so as a benign fix be sure to not overwrite 'name' if "name" isn't already set.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- libxkutil/pool_parsing.c | 39 ++++++++++++++++++----------------- src/Virt_SettingsDefineCapabilities.c | 2 +- 2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/libxkutil/pool_parsing.c b/libxkutil/pool_parsing.c index 922ff32..80bd4ca 100644 --- a/libxkutil/pool_parsing.c +++ b/libxkutil/pool_parsing.c @@ -194,15 +194,17 @@ char *get_disk_pool_type(uint16_t type)
}
-static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool) +static char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool) { xmlNode **nodes = nsv->nodeTab; xmlNode *child; - const char *type_str = NULL; - const char *name = NULL; + char *type_str = NULL; + char *name = NULL; int type = 0;
type_str = get_attr_value(nodes[0], "type"); + if (type_str == NULL) + return NULL;
if (STREQC(type_str, "dir")) type = DISK_POOL_DIR; @@ -220,12 +222,15 @@ static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool) type = DISK_POOL_SCSI; else type = DISK_POOL_UNKNOWN; + free(type_str);
pool->pool_type = type; - + for (child = nodes[0]->children; child != NULL; child = child->next) { - if (XSTREQ(child->name, "name")) { + if (XSTREQ(child->name, "name") && name == NULL) { name = get_node_content(child); + if (name == NULL) + return NULL; } else if (XSTREQ(child->name, "target")) parse_disk_target(child, pool); else if (XSTREQ(child->name, "source")) @@ -238,14 +243,18 @@ static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool) int get_pool_from_xml(const char *xml, struct virt_pool *pool, int type) { int len; - int ret = 0; + int ret = 1; Isn't this a bug fix... beside a cleanup? Before the return value was always 0! It also changes the method get_disk_pool in Virt_DevicePool.c in its return value behavior in the same way. The usage of get_disk_pool in the method diskpool_set_capacity checks for the return value and being !=0 it is considered an error (Virt_DevicePool.c line 324). Doesn't that need to get changed in this
On 01/22/2014 08:30 PM, John Ferlan wrote: patch? Even so you mention above in your description the two chained exploitation code pieces it seems that you fixed just one ... or did I miss something?
xmlDoc *xmldoc; xmlXPathContext *xpathctx; xmlXPathObject *xpathobj; const xmlChar *xpathstr = (xmlChar *)"/pool"; - const char *name;
CU_DEBUG("Pool XML : %s", xml); + + /* FIXME: Add support for parsing network pools */ + if (type == CIM_RES_TYPE_NET) + return 0; + len = strlen(xml) + 1;
if ((xmldoc = xmlParseMemory(xml, len)) == NULL) @@ -257,22 +266,14 @@ int get_pool_from_xml(const char *xml, struct virt_pool *pool, int type) if ((xpathobj = xmlXPathEvalExpression(xpathstr, xpathctx)) == NULL) goto err3;
- /* FIXME: Add support for parsing network pools */ - if (type == CIM_RES_TYPE_NET) { - ret = 0; - goto err1; - } - memset(pool, 0, sizeof(*pool));
- pool->type = CIM_RES_TYPE_DISK; - name = parse_disk_pool(xpathobj->nodesetval, - &(pool)->pool_info.disk); - if (name == NULL) + pool->type = CIM_RES_TYPE_DISK; + pool->id = parse_disk_pool(xpathobj->nodesetval, + &(pool)->pool_info.disk); + if (pool->id != NULL) ret = 0;
- pool->id = strdup(name); - xmlXPathFreeObject(xpathobj); err3: xmlXPathFreeContext(xpathctx); diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c index fe16e3f..756e46b 100644 --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -1376,7 +1376,7 @@ static CMPIStatus _new_volume_template(const CMPIObjectPath *ref, }
ret = get_disk_pool(poolptr, &pool); - if (ret == 1) { + if (ret != 0) { virt_set_status(_BROKER, &s, CMPI_RC_ERR_FAILED, virStoragePoolGetConnect(poolptr),
src/Virt_DevicePool.c line 324 =! change seems to be missing. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294