[libvirt] [PATCH v8 2/4] Add vbox_snapshot_conf struct

[John Ferlan]

Ug... and trying to "locally" fix the RW/RO discovered two more issues...

On 06/11/2014 08:21 AM, John Ferlan wrote:
> 
> This patch has resulted in many new Coverity errors - mostly resource
> leaks as a result of the virVBoxSnapshotConfAllChildren() recursive
> function.  I would clean them up, but I'm a bit leary of missing some
> nuance in the original design.
> 
> There were also a couple of issues in
> virVBoxSnapshotConfGetRWDisksPathsFromLibvirtXML and
> virVBoxSnapshotConfGetRODisksPathsFromLibvirtXML
> 
> Details of the issues and some thoughts are inline below in the functions
> 
> These should be cleaned up before the next release...
> 
> John
> 

<...snip...>

>> +
>> +/*
>> + *getRWDisksPathsFromLibvirtXML: Parse a libvirt XML snapshot file, allocates and
>> + *fills a list of read-write disk paths.
>> + *return array length on success, -1 on failure.
>> + */
>> +int virVBoxSnapshotConfGetRWDisksPathsFromLibvirtXML(char *filePath, char ***rwDisksPath)
>> +{
>> +    int result = -1;
>> +    size_t i = 0;
>> +    char **ret;

Needs to be initialized to NULL (char **ret = NULL;)

>> +    xmlDocPtr xml = NULL;
>> +    xmlXPathContextPtr xPathContext = NULL;
>> +    xmlNodePtr *nodes = NULL;
>> +    int nodeSize = 0;
>> +    if (filePath == NULL) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("filePath is null"));
>> +        goto cleanup;
>> +    }
>> +    xml = virXMLParse(filePath, NULL, NULL);
>> +    if (xml == NULL) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("Unable to parse the xml"));
>> +        goto cleanup;
>> +    }
>> +    if (!(xPathContext = xmlXPathNewContext(xml))) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +    xPathContext->node = xmlDocGetRootElement(xml);
>> +    nodeSize = virXPathNodeSet("/domainsnapshot/disks/disk", xPathContext, &nodes);
> Coverity comlains that nodeSize can be a negative number.  Other code in
> libvirt will do something like:
> 
>     if ((nodeSize = virXPathNodeSet("/domainsnapshot/disks/disk",
>                                     xPathContext, &nodes)) < 0)
>         goto cleanup;
> 
> 
>> +
>> +    if (VIR_ALLOC_N(ret, nodeSize) < 0)
>> +        goto cleanup;
>> +
>> +    for (i = 0; i < nodeSize; i++) {
>> +        xmlNodePtr node = nodes[i];
>> +        xPathContext->node = node;
>> +        xmlNodePtr sourceNode = virXPathNode("./source", xPathContext);
>> +        if (sourceNode) {
>> +            ret[i] = virXMLPropString(sourceNode, "file");
>> +        }
>> +    }
>> +    result = 0;
>> +
>> + cleanup:
>> +    xmlFreeDoc(xml);
>> +    xmlXPathFreeContext(xPathContext);
>> +    if (result < 0) {
>> +        virStringFreeList(ret);
>> +        nodeSize = -1;
>> +    }
>> +    *rwDisksPath = ret;
>> +    return nodeSize;
>> +}
>> +
>> +/*
>> + *getRODisksPathsFromLibvirtXML: *Parse a libvirt XML snapshot file, allocates and fills
>> + *a list of read-only disk paths (the parents of the read-write disks).
>> + *return array length on success, -1 on failure.
>> + */
>> +int virVBoxSnapshotConfGetRODisksPathsFromLibvirtXML(char *filePath, char ***roDisksPath)
>> +{
>> +    int result = -1;
>> +    size_t i = 0;
>> +    char **ret;
>> +    xmlDocPtr xml = NULL;
>> +    xmlXPathContextPtr xPathContext = NULL;
>> +    xmlNodePtr *nodes = NULL;
>> +    int nodeSize = 0;
>> +    if (filePath == NULL) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("filePath is null"));
>> +        goto cleanup;
>> +    }
>> +    xml = virXMLParse(filePath, NULL, NULL);
>> +    if (xml == NULL) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("Unable to parse the xml"));
>> +        goto cleanup;
>> +    }
>> +    if (!(xPathContext = xmlXPathNewContext(xml))) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +    xPathContext->node = xmlDocGetRootElement(xml);
>> +    nodeSize = virXPathNodeSet("/domainsnapshot/domain/devices/disk",
>> +                               xPathContext,
>> +                               &nodes);
> 
> Same issue here as the RW code - you need to handle nodeSize return:
> 
>     if ((nodeSize = virXPathNodeSet("/domainsnapshot/domain/devices/disk",
>                                     xPathContext,
>                                     &nodes)) < 0)
>         goto cleanup;
> 
>> +    if (VIR_ALLOC_N(ret, nodeSize) < 0)
>> +        goto cleanup;
>> +
>> +    for (i = 0; i < nodeSize; i++) {
>> +        xmlNodePtr node = nodes[i];
>> +        xPathContext->node = node;
>> +        xmlNodePtr sourceNode = virXPathNode("./source", xPathContext);
>> +        if (sourceNode) {
>> +            ret[i] = virXMLPropString(sourceNode, "file");
>> +        }
>> +    }
>> +    result = 0;
>> +
>> + cleanup:
>> +    xmlFreeDoc(xml);
>> +    xmlXPathFreeContext(xPathContext);
>> +    if (result < 0) {
>> +        virStringFreeList(ret);
>> +        nodeSize = -1;
>> +    }
>> +    *roDisksPath = ret;

Use:

+    } else {
+        *roDisksPath = ret;
     }

>> +    return nodeSize;
>> +}
>> +