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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list