Re: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath

This function not only did not free return value of xmlNodeListGetString() it strdup()-ed its return value. Therefore plenty of memory has been lost definitely upon return from this function. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt-php.c | 110 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 62 insertions(+), 48 deletions(-) diff --git a/src/libvirt-php.c b/src/libvirt-php.c index 308e764..fb0679b 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -3232,19 +3232,17 @@ char *get_string_from_xpath(char *xml, char *xpath, zval **val, int *retVal) if (val != NULL) { ret = 0; for (i = 0; i < nodeset->nodeNr; i++) { - if (xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode, 1) != NULL) { - value = (char *)xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode, 1); - + if ((value = xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode, 1))) { snprintf(key, sizeof(key), "%d", i); VIRT_ADD_ASSOC_STRING(*val, key, value); + free(value); + value = NULL;

ret++; } } add_assoc_long(*val, "num", (long)ret); - } - else { - if (xmlNodeListGetString(doc, nodeset->nodeTab[0]->xmlChildrenNode, 1) != NULL) - value = (char *)xmlNodeListGetString(doc, nodeset->nodeTab[0]->xmlChildrenNode, 1); + } else { + value = xmlNodeListGetString(doc, nodeset->nodeTab[0]->xmlChildrenNode, 1); } cleanup: @@ -3255,7 +3253,7 @@ char *get_string_from_xpath(char *xml, char *xpath, zval **val, int *retVal) xmlFreeParserCtxt(xp); xmlFreeDoc(doc); xmlCleanupParser(); - return (value != NULL) ? strdup(value) : NULL; + return value; } /* @@ -3327,11 +3325,8 @@ char **get_array_from_xpath(char *xml, char *xpath, int *num) ret = 0; val = (char **)malloc( nodeset->nodeNr * sizeof(char *) ); for (i = 0; i < nodeset->nodeNr; i++) { - if (xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode, 1) != NULL) { - value = (char *)xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode, 1); - - val[ret++] = strdup(value); - } + if ((value = xmlNodeListGetString(doc, nodeset->nodeTab[i]->xmlChildrenNode, 1))) + val[ret++] = value; } xmlXPathFreeContext(context); @@ -3506,20 +3501,23 @@ long get_next_free_numeric_value(virDomainPtr domain, char *xpath) */ char *connection_get_domain_type(virConnectPtr conn, char *arch TSRMLS_DC) { - int retval = -1; + char *ret = NULL; char *tmp = NULL; char *caps = NULL; + char *tmpArch = NULL; char xpath[1024] = { 0 }; + int retval = -1; caps = virConnectGetCapabilities(conn); if (caps == NULL) return NULL; if (arch == NULL) { - arch = get_string_from_xpath(caps, "//capabilities/host/cpu/arch", NULL, &retval); - DPRINTF("%s: No architecture defined, got '%s' from capabilities XML\n", __FUNCTION__, arch); - if ((arch == NULL) || (retval < 0)) - return NULL; + tmpArch = get_string_from_xpath(caps, "//capabilities/host/cpu/arch", NULL, &retval); + DPRINTF("%s: No architecture defined, got '%s' from capabilities XML\n", __FUNCTION__, tmpArch); + if (!tmpArch || retval < 0) + goto cleanup; + arch = tmpArch; } DPRINTF("%s: Requested domain type for arch '%s'\n", __FUNCTION__, arch); @@ -3529,12 +3527,17 @@ char *connection_get_domain_type(virConnectPtr conn, char *arch TSRMLS_DC) tmp = get_string_from_xpath(caps, xpath, NULL, &retval); if ((tmp == NULL) || (retval < 0)) { DPRINTF("%s: No domain type found in XML...\n", __FUNCTION__); - return NULL; + goto cleanup; } - DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, tmp); - - return tmp; + ret = tmp; + tmp = NULL; + DPRINTF("%s: Domain type is '%s'\n", __FUNCTION__, ret); + cleanup: + free(tmpArch); + free(caps); + free(tmp);