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; +} +