[libvirt] [libvirt-php][PATCH 0/3] Couple of small fixes
While playing with our php bindings I've noticed couple of memory leaks mostly. And one bug in our examples. Michal Privoznik (3): examples: Check properly if connected Fix some leaks related to get_string_from_xpath Revisit some VIRT_RETURN_STRING examples/index.php | 4 +- src/libvirt-php.c | 121 ++++++++++++++++++++++++++++++----------------------- 2 files changed, 72 insertions(+), 53 deletions(-) -- 2.7.3
So far, we connect and issue an API immediately. And only after it fails we assume we're not connected. This is just not right as we should have checked before issuing the API whether we are connected at all. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/index.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/examples/index.php b/examples/index.php index 381baef..b2f5b69 100644 --- a/examples/index.php +++ b/examples/index.php @@ -1,9 +1,11 @@ <?php require('libvirt.php'); $lv = new Libvirt('qemu:///system'); + if ($lv == false) + die('<html><body>Cannot open connection to hypervisor</body></html>'); $hn = $lv->get_hostname(); if ($hn == false) - die('Cannot open connection to hypervisor</body></html>'); + die('<html><body>Cannot get hostname</body></html>'); $action = array_key_exists('action', $_GET) ? $_GET['action'] : false; $subaction = array_key_exists('subaction', $_GET) ? $_GET['subaction'] : false; -- 2.7.3
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@redhat.com> --- 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); + return ret; } /* @@ -3547,20 +3550,23 @@ char *connection_get_domain_type(virConnectPtr conn, char *arch TSRMLS_DC) */ char *connection_get_emulator(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 emulator for arch '%s'\n", __FUNCTION__, arch); @@ -3568,26 +3574,30 @@ char *connection_get_emulator(virConnectPtr conn, char *arch TSRMLS_DC) snprintf(xpath, sizeof(xpath), "//capabilities/guest/arch[@name='%s']/domain/emulator", arch); DPRINTF("%s: Applying xPath '%s' to capabilities XML output\n", __FUNCTION__, xpath); tmp = get_string_from_xpath(caps, xpath, NULL, &retval); - if ((tmp == NULL) || (retval < 0)) { - DPRINTF("%s: No emulator found. Trying next location ...\n", __FUNCTION__); - snprintf(xpath, sizeof(xpath), "//capabilities/guest/arch[@name='%s']/emulator", arch); - } - else { + if (tmp && retval >= 0) { DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, tmp); - return tmp; + goto done; } + DPRINTF("%s: No emulator found. Trying next location ...\n", __FUNCTION__); + snprintf(xpath, sizeof(xpath), "//capabilities/guest/arch[@name='%s']/emulator", arch); DPRINTF("%s: Applying xPath '%s' to capabilities XML output\n", __FUNCTION__, xpath); - + free(tmp); tmp = get_string_from_xpath(caps, xpath, NULL, &retval); - if ((tmp == NULL) || (retval < 0)) { - DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, tmp); - return NULL; + if (!tmp || retval < 0) { + DPRINTF("%s: None emulator found\n", __FUNCTION__); + goto cleanup; } - DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, tmp); - - return tmp; + done: + ret = tmp; + tmp = NULL; + DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, ret); + cleanup: + free(tmpArch); + free(caps); + free(tmp); + return ret; } /* @@ -3599,26 +3609,29 @@ char *connection_get_emulator(virConnectPtr conn, char *arch TSRMLS_DC) */ char *connection_get_arch(virConnectPtr conn TSRMLS_DC) { - int retval = -1; + char *ret = NULL; char *tmp = NULL; char *caps = NULL; - // char xpath[1024] = { 0 }; + int retval = -1; caps = virConnectGetCapabilities(conn); if (caps == NULL) return NULL; tmp = get_string_from_xpath(caps, "//capabilities/host/cpu/arch", NULL, &retval); - free(caps); - if ((tmp == NULL) || (retval < 0)) { DPRINTF("%s: Cannot get host CPU architecture from capabilities XML\n", __FUNCTION__); - return NULL; + goto cleanup; } - DPRINTF("%s: Host CPU architecture is '%s'\n", __FUNCTION__, tmp); + ret = tmp; + tmp = NULL; + DPRINTF("%s: Host CPU architecture is '%s'\n", __FUNCTION__, ret); - return tmp; + cleanup: + free(caps); + free(tmp); + return ret; } /* @@ -7130,7 +7143,8 @@ PHP_FUNCTION(libvirt_domain_xml_xpath) array_init(return_value); - if ((tmp = get_string_from_xpath(xml, (char *)zpath, &return_value, &rc)) == NULL) { + free(get_string_from_xpath(xml, (char *)zpath, &return_value, &rc)); + if (return_value < 0) { free(xml); RETURN_FALSE; } -- 2.7.3
On Tue, Apr 19, 2016 at 03:46:41PM +0200, Michal Privoznik wrote:
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@redhat.com> --- 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;
'value' is an xmlChar so must be freed with xmlFree. I'd change the return value of get_string_from_xpath() to make it clear that xmlFree needs to be used to free it as well.
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);
tmp/tmpArch should be freed xmlFree. (and I assume more of the same below). Christophe
On 19.04.2016 16:09, Christophe Fergeau wrote:
On Tue, Apr 19, 2016 at 03:46:41PM +0200, Michal Privoznik wrote:
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@redhat.com> --- 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;
'value' is an xmlChar so must be freed with xmlFree. I'd change the return value of get_string_from_xpath() to make it clear that xmlFree needs to be used to free it as well.
Yeah, I could not agree more. But problem is that would be a great patch. Moreover, xmlFree() == free() unless some debug is turned on in libxml2 during its compilation. And since Vasiliy will make this function go away, I'd rather push this as is. Michal
2016-04-19 17:59 GMT+03:00 Michal Privoznik <mprivozn@redhat.com>:
Yeah, I could not agree more. But problem is that would be a great patch. Moreover, xmlFree() == free() unless some debug is turned on in libxml2 during its compilation. And since Vasiliy will make this function go away, I'd rather push this as is.
Ok -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
On Tue, Apr 19, 2016 at 04:59:58PM +0200, Michal Privoznik wrote:
On 19.04.2016 16:09, Christophe Fergeau wrote:
On Tue, Apr 19, 2016 at 03:46:41PM +0200, Michal Privoznik wrote:
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@redhat.com> --- 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;
'value' is an xmlChar so must be freed with xmlFree. I'd change the return value of get_string_from_xpath() to make it clear that xmlFree needs to be used to free it as well.
Yeah, I could not agree more. But problem is that would be a great patch. Moreover, xmlFree() == free() unless some debug is turned on in libxml2 during its compilation. And since Vasiliy will make this function go away, I'd rather push this as is.
If this goes away soon, and if the patch would be big, fine with me. Christophe
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
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@redhat.com> --- 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); + return ret; }
/* @@ -3547,20 +3550,23 @@ char *connection_get_domain_type(virConnectPtr conn, char *arch TSRMLS_DC) */ char *connection_get_emulator(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 emulator for arch '%s'\n", __FUNCTION__, arch); @@ -3568,26 +3574,30 @@ char *connection_get_emulator(virConnectPtr conn, char *arch TSRMLS_DC) snprintf(xpath, sizeof(xpath), "//capabilities/guest/arch[@name='%s']/domain/emulator", arch); DPRINTF("%s: Applying xPath '%s' to capabilities XML output\n", __FUNCTION__, xpath); tmp = get_string_from_xpath(caps, xpath, NULL, &retval); - if ((tmp == NULL) || (retval < 0)) { - DPRINTF("%s: No emulator found. Trying next location ...\n", __FUNCTION__); - snprintf(xpath, sizeof(xpath), "//capabilities/guest/arch[@name='%s']/emulator", arch); - } - else { + if (tmp && retval >= 0) { DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, tmp); - return tmp; + goto done; }
+ DPRINTF("%s: No emulator found. Trying next location ...\n", __FUNCTION__); + snprintf(xpath, sizeof(xpath), "//capabilities/guest/arch[@name='%s']/emulator", arch); DPRINTF("%s: Applying xPath '%s' to capabilities XML output\n", __FUNCTION__, xpath); - + free(tmp); tmp = get_string_from_xpath(caps, xpath, NULL, &retval); - if ((tmp == NULL) || (retval < 0)) { - DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, tmp); - return NULL; + if (!tmp || retval < 0) { + DPRINTF("%s: None emulator found\n", __FUNCTION__); + goto cleanup; }
- DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, tmp); - - return tmp; + done: + ret = tmp; + tmp = NULL; + DPRINTF("%s: Emulator is '%s'\n", __FUNCTION__, ret); + cleanup: + free(tmpArch); + free(caps); + free(tmp); + return ret; }
/* @@ -3599,26 +3609,29 @@ char *connection_get_emulator(virConnectPtr conn, char *arch TSRMLS_DC) */ char *connection_get_arch(virConnectPtr conn TSRMLS_DC) { - int retval = -1; + char *ret = NULL; char *tmp = NULL; char *caps = NULL; - // char xpath[1024] = { 0 }; + int retval = -1;
caps = virConnectGetCapabilities(conn); if (caps == NULL) return NULL;
tmp = get_string_from_xpath(caps, "//capabilities/host/cpu/arch", NULL, &retval); - free(caps); - if ((tmp == NULL) || (retval < 0)) { DPRINTF("%s: Cannot get host CPU architecture from capabilities XML\n", __FUNCTION__); - return NULL; + goto cleanup; }
- DPRINTF("%s: Host CPU architecture is '%s'\n", __FUNCTION__, tmp); + ret = tmp; + tmp = NULL; + DPRINTF("%s: Host CPU architecture is '%s'\n", __FUNCTION__, ret);
- return tmp; + cleanup: + free(caps); + free(tmp); + return ret; }
/* @@ -7130,7 +7143,8 @@ PHP_FUNCTION(libvirt_domain_xml_xpath)
array_init(return_value);
- if ((tmp = get_string_from_xpath(xml, (char *)zpath, &return_value, &rc)) == NULL) { + free(get_string_from_xpath(xml, (char *)zpath, &return_value, &rc)); + if (return_value < 0) { free(xml); RETURN_FALSE; } -- 2.7.3
Applied locally and tested. Works okay for me. :) -- 真実はいつも一つ！/ Always, there's only one truth!
2016-04-19 16:46 GMT+03:00 Michal Privoznik <mprivozn@redhat.com>:
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.
I don't think that this function need at all. I'm switching to get full xml of domain and doing xpath via native php code. -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
On 19.04.2016 16:48, Vasiliy Tolstov wrote:
2016-04-19 16:46 GMT+03:00 Michal Privoznik <mprivozn@redhat.com>:
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.
I don't think that this function need at all. I'm switching to get full xml of domain and doing xpath via native php code.
Yeah, but I was just too lazy to do that :) If you can post a patch that would be great! Any time estimate, e.g. should I postpone the release for tomorrow? Michal
2016-04-19 17:51 GMT+03:00 Michal Privoznik <mprivozn@redhat.com>:
Yeah, but I was just too lazy to do that :) If you can post a patch that would be great! Any time estimate, e.g. should I postpone the release for tomorrow?
I think you can release not waiting for me, after release i prepare patches for cleanup old xml stuff. -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
This macro sets return value and returns immediately. This can lead to a possible leak because any free() that would come afterwards is in fact never called. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-php.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libvirt-php.c b/src/libvirt-php.c index fb0679b..6f6e137 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -4307,7 +4307,8 @@ PHP_FUNCTION(libvirt_domain_qemu_agent_command) ret = virDomainQemuAgentCommand(domain->domain, cmd, timeout, flags); if (ret == NULL) RETURN_FALSE; - VIRT_RETURN_STRING(ret); + VIRT_RETVAL_STRING(ret); + free(ret); } /* @@ -6691,7 +6692,7 @@ PHP_FUNCTION(libvirt_domain_memory_peek) buff=(char *)emalloc(size); retval=virDomainMemoryPeek(domain->domain,start,size,buff,flags); if (retval != 0) RETURN_FALSE; - VIRT_RETURN_STRINGL(buff, size); + VIRT_RETVAL_STRINGL(buff, size); efree(buff); } @@ -7872,7 +7873,8 @@ PHP_FUNCTION(libvirt_storagevolume_get_path) DPRINTF("%s: virStorageVolGetPath(%p) returned %s\n", PHPFUNC, volume->volume, retval); if (retval == NULL) RETURN_FALSE; - VIRT_RETURN_STRING(retval); + VIRT_RETVAL_STRING(retval); + free(retval); } /* @@ -9506,7 +9508,8 @@ PHP_FUNCTION(libvirt_network_get_bridge) RETURN_FALSE; } - VIRT_RETURN_STRING(name); + VIRT_RETVAL_STRING(name); + free(name); } /* -- 2.7.3
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
This macro sets return value and returns immediately. This can lead to a possible leak because any free() that would come afterwards is in fact never called.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-php.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/libvirt-php.c b/src/libvirt-php.c index fb0679b..6f6e137 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -4307,7 +4307,8 @@ PHP_FUNCTION(libvirt_domain_qemu_agent_command) ret = virDomainQemuAgentCommand(domain->domain, cmd, timeout, flags); if (ret == NULL) RETURN_FALSE;
- VIRT_RETURN_STRING(ret); + VIRT_RETVAL_STRING(ret); + free(ret); }
/* @@ -6691,7 +6692,7 @@ PHP_FUNCTION(libvirt_domain_memory_peek) buff=(char *)emalloc(size); retval=virDomainMemoryPeek(domain->domain,start,size,buff,flags); if (retval != 0) RETURN_FALSE; - VIRT_RETURN_STRINGL(buff, size); + VIRT_RETVAL_STRINGL(buff, size); efree(buff); }
@@ -7872,7 +7873,8 @@ PHP_FUNCTION(libvirt_storagevolume_get_path) DPRINTF("%s: virStorageVolGetPath(%p) returned %s\n", PHPFUNC, volume->volume, retval); if (retval == NULL) RETURN_FALSE;
- VIRT_RETURN_STRING(retval); + VIRT_RETVAL_STRING(retval); + free(retval); }
/* @@ -9506,7 +9508,8 @@ PHP_FUNCTION(libvirt_network_get_bridge) RETURN_FALSE; }
- VIRT_RETURN_STRING(name); + VIRT_RETVAL_STRING(name); + free(name); }
/* -- 2.7.3
Applied locally and tested. Works okay for me. :) -- 真実はいつも一つ！/ Always, there's only one truth!
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
While playing with our php bindings I've noticed couple of memory leaks mostly. And one bug in our examples.
Michal Privoznik (3): examples: Check properly if connected Fix some leaks related to get_string_from_xpath Revisit some VIRT_RETURN_STRING
examples/index.php | 4 +- src/libvirt-php.c | 121 ++++++++++++++++++++++++++++++----------------------- 2 files changed, 72 insertions(+), 53 deletions(-)
-- 2.7.3
For the patch set, I couldn't get the first one through the ML (for some reason, the ML never sent it to my inbox). The other two, however, made it and I've tested them. They applied cleanly, and libvirt-php built and ran cleanly on PHP5 and PHP7 on CentOS 7 and PHP7 on Ubuntu 16.04. Reading the first patch through the list archive, I manually applied and tested. Everything seems fine with it. For the patch series as a whole, I give it a thumbs up! :) -- 真実はいつも一つ！/ Always, there's only one truth!
participants (4)
-
Christophe Fergeau -
Michal Privoznik -
Neal Gompa -
Vasiliy Tolstov