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