8:46 a.m.
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
8:46 a.m.
New subject: [libvirt] [libvirt-php][PATCH 1/3] examples: Check properly if connected
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(a)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
8:46 a.m.
New subject: [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>
---
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
9:09 a.m.
New subject: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
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(a)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
9:59 a.m.
New subject: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
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(a)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
10:06 a.m.
New subject: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
2016-04-19 17:59 GMT+03:00 Michal Privoznik <mprivozn(a)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(a)selfip.ru
10:10 a.m.
New subject: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
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(a)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
9:29 a.m.
New subject: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik <mprivozn(a)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(a)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!
9:48 a.m.
New subject: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
2016-04-19 16:46 GMT+03:00 Michal Privoznik <mprivozn(a)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(a)selfip.ru
9:51 a.m.
New subject: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
On 19.04.2016 16:48, Vasiliy Tolstov wrote:
2016-04-19 16:46 GMT+03:00 Michal Privoznik
<mprivozn(a)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
9:56 a.m.
New subject: [libvirt] [libvirt-php][PATCH 2/3] Fix some leaks related to get_string_from_xpath
2016-04-19 17:51 GMT+03:00 Michal Privoznik <mprivozn(a)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(a)selfip.ru
8:46 a.m.
New subject: [libvirt] [libvirt-php][PATCH 3/3] Revisit some VIRT_RETURN_STRING
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(a)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
9:29 a.m.
New subject: [libvirt] [libvirt-php][PATCH 3/3] Revisit some VIRT_RETURN_STRING
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik <mprivozn(a)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(a)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!
9:32 a.m.
On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik <mprivozn(a)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!
3141
days inactive
3141
days old
13 comments
4 participants
participants (4)
-
Christophe Fergeau -
Michal Privoznik -
Neal Gompa -
Vasiliy Tolstov