Re: [Libvirt-cim] [PATCH 2/6] libxkutil:pool_parsing: Coverity cleanups

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(a)redhat.com&gt; --- 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;

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),