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

[Christophe Fergeau]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160419/6013f428/attachment-0001.sig>