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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 19, 2016 at 9:46 AM, Michal Privoznik <mprivozn@xxxxxxxxxx> 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;
>                  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!

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]