Re: [Xen-devel] [PATCH for-4.5] tools/libxl: Use of init()/dispose() to avoid leaking libxl_dominfo.ssid_label
On Mon, 2015-01-05 at 10:33 -0500, Konrad Rzeszutek Wilk wrote:
On Mon, Jan 05, 2015 at 02:19:58PM +, Andrew Cooper wrote:
libxl_dominfo contains a ssid_label pointer which will have memory allocated
for it in libxl_domain_info() if the hypervisor has CONFIG_XSM compiled.
However, the lack of appropriate use of libxl_dominfo_{init,dispose}() will
cause the label string to be leaked, even in success cases.
This was discovered by XenServers Coverity scanning, and are issues not
identified by upstream Coverity Scan.
Signed-off-by: Andrew Cooper andrew.coop...@citrix.com
CC: Ian Campbell ian.campb...@citrix.com
CC: Ian Jackson ian.jack...@eu.citrix.com
CC: Wei Liu wei.l...@citrix.com
CC: Konrad Rzeszutek Wilk konrad.w...@oracle.com
---
Requesting a release-exception as suggested by IanJ. These are memory leaks
which accumulate via the successful completion of libxl library functions
(if
XSM is enabled), which will undoubtedly cause issues for the likes of
libvirt
and xenopsd-libxl which use libxl in daemon processes.
Release-Acked-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
Somehow I don't seem to have the original in any folder, I probably fat
fingered the D key at some point.
Anyway, this looks good to me for 4.5:
Acked-by: Ian Campbell ian.campb...@citrix.com
Applied.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH for-4.5] tools/libxl: Use of init()/dispose() to avoid leaking libxl_dominfo.ssid_label
libxl_dominfo contains a ssid_label pointer which will have memory allocated
for it in libxl_domain_info() if the hypervisor has CONFIG_XSM compiled.
However, the lack of appropriate use of libxl_dominfo_{init,dispose}() will
cause the label string to be leaked, even in success cases.
This was discovered by XenServers Coverity scanning, and are issues not
identified by upstream Coverity Scan.
Signed-off-by: Andrew Cooper andrew.coop...@citrix.com
CC: Ian Campbell ian.campb...@citrix.com
CC: Ian Jackson ian.jack...@eu.citrix.com
CC: Wei Liu wei.l...@citrix.com
CC: Konrad Rzeszutek Wilk konrad.w...@oracle.com
---
Requesting a release-exception as suggested by IanJ. These are memory leaks
which accumulate via the successful completion of libxl library functions (if
XSM is enabled), which will undoubtedly cause issues for the likes of libvirt
and xenopsd-libxl which use libxl in daemon processes.
As we are very close to the release, I have opted for the more
obviously-correct code rather than the pragmatically better code, particularly
in two locations where libxl_domain_info() is called in a loop, and the
dispose()/init() pair is required to prevent leaking the allocation on each
iteration.
All of the uses of libxl_domain_info() patched here could better be an
xc_dominfo_getlist() which reduces the quantity of hypercalls made, and the
amount of data transformation done behind the scenes.
---
tools/libxl/libxl.c | 26 --
tools/libxl/libxl_dom.c | 14 ++
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 50a8928..372dd3b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -364,6 +364,8 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
char *uuid;
const char *vm_name_path;
+libxl_dominfo_init(info);
+
dom_path = libxl__xs_get_dompath(gc, domid);
if (!dom_path) goto x_nomem;
@@ -481,6 +483,7 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
rc = 0;
x_rc:
if (our_trans) xs_transaction_end(ctx-xsh, our_trans, 1);
+libxl_dominfo_dispose(info);
return rc;
x_fail: rc = ERROR_FAIL; goto x_rc;
@@ -4594,6 +4597,8 @@ static int libxl__fill_dom0_memory_info(libxl__gc *gc,
uint32_t *target_memkb,
libxl_ctx *ctx = libxl__gc_owner(gc);
uint32_t free_mem_slack_kb = 0;
+libxl_dominfo_init(info);
+
retry_transaction:
t = xs_transaction_start(ctx-xsh);
@@ -4626,6 +4631,8 @@ retry_transaction:
}
}
+libxl_dominfo_dispose(info);
+libxl_dominfo_init(info);
rc = libxl_domain_info(ctx, info, 0);
if (rc 0)
goto out;
@@ -4664,7 +4671,7 @@ out:
rc = ERROR_FAIL;
}
-
+libxl_dominfo_dispose(info);
return rc;
}
@@ -4999,6 +5006,8 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t
domid, int wait_secs)
uint32_t target_memkb = 0;
libxl_dominfo info;
+libxl_dominfo_init(info);
+
do {
wait_secs--;
sleep(1);
@@ -5007,9 +5016,11 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx,
uint32_t domid, int wait_secs)
if (rc 0)
goto out;
+libxl_dominfo_dispose(info);
+libxl_dominfo_init(info);
rc = libxl_domain_info(ctx, info, domid);
if (rc 0)
-return rc;
+goto out;
} while (wait_secs 0 (info.current_memkb + info.outstanding_memkb)
target_memkb);
if ((info.current_memkb + info.outstanding_memkb) = target_memkb)
@@ -5018,6 +5029,7 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t
domid, int wait_secs)
rc = ERROR_FAIL;
out:
+libxl_dominfo_dispose(info);
return rc;
}
@@ -5435,6 +5447,8 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc *gc,
uint32_t domid,
xs_transaction_t t;
int i, rc = ERROR_FAIL;
+libxl_dominfo_init(info);
+
if (libxl_domain_info(CTX, info, domid) 0) {
LOGE(ERROR, getting domain info list);
goto out;
@@ -5454,6 +5468,7 @@ retry_transaction:
} else
rc = 0;
out:
+libxl_dominfo_dispose(info);
return rc;
}
@@ -5463,8 +5478,11 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc,
uint32_t domid,
libxl_dominfo info;
int i;
+libxl_dominfo_init(info);
+
if (libxl_domain_info(CTX, info, domid) 0) {
LOGE(ERROR, getting domain info list);
+libxl_dominfo_dispose(info);
return ERROR_FAIL;
}
for (i = 0; i = info.vcpu_max_id; i++) {
@@ -5477,6 +5495,7 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc,
uint32_t domid,
libxl__qmp_cpu_add(gc, domid, i);
}
}
+libxl_dominfo_dispose(info);
return 0;
}
@@ -6569,12 +6588,15 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx,
uint32_t domid,
/* Domain UUID */
{
libxl_dominfo info;
+libxl_dominfo_init(info);
rc =
Re: [Xen-devel] [PATCH for-4.5] tools/libxl: Use of init()/dispose() to avoid leaking libxl_dominfo.ssid_label
On Mon, Jan 05, 2015 at 02:19:58PM +, Andrew Cooper wrote:
libxl_dominfo contains a ssid_label pointer which will have memory allocated
for it in libxl_domain_info() if the hypervisor has CONFIG_XSM compiled.
However, the lack of appropriate use of libxl_dominfo_{init,dispose}() will
cause the label string to be leaked, even in success cases.
This was discovered by XenServers Coverity scanning, and are issues not
identified by upstream Coverity Scan.
Signed-off-by: Andrew Cooper andrew.coop...@citrix.com
CC: Ian Campbell ian.campb...@citrix.com
CC: Ian Jackson ian.jack...@eu.citrix.com
Reviewed-by : Wei Liu wei.l...@citrix.com
CC: Konrad Rzeszutek Wilk konrad.w...@oracle.com
---
Requesting a release-exception as suggested by IanJ. These are memory leaks
which accumulate via the successful completion of libxl library functions (if
XSM is enabled), which will undoubtedly cause issues for the likes of libvirt
and xenopsd-libxl which use libxl in daemon processes.
As we are very close to the release, I have opted for the more
obviously-correct code rather than the pragmatically better code, particularly
in two locations where libxl_domain_info() is called in a loop, and the
dispose()/init() pair is required to prevent leaking the allocation on each
iteration.
All of the uses of libxl_domain_info() patched here could better be an
xc_dominfo_getlist() which reduces the quantity of hypercalls made, and the
amount of data transformation done behind the scenes.
---
tools/libxl/libxl.c | 26 --
tools/libxl/libxl_dom.c | 14 ++
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 50a8928..372dd3b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -364,6 +364,8 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
(This function is really convoluted. :-/)
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.5] tools/libxl: Use of init()/dispose() to avoid leaking libxl_dominfo.ssid_label
On Mon, Jan 05, 2015 at 02:19:58PM +, Andrew Cooper wrote:
libxl_dominfo contains a ssid_label pointer which will have memory allocated
for it in libxl_domain_info() if the hypervisor has CONFIG_XSM compiled.
However, the lack of appropriate use of libxl_dominfo_{init,dispose}() will
cause the label string to be leaked, even in success cases.
This was discovered by XenServers Coverity scanning, and are issues not
identified by upstream Coverity Scan.
Signed-off-by: Andrew Cooper andrew.coop...@citrix.com
CC: Ian Campbell ian.campb...@citrix.com
CC: Ian Jackson ian.jack...@eu.citrix.com
CC: Wei Liu wei.l...@citrix.com
CC: Konrad Rzeszutek Wilk konrad.w...@oracle.com
---
Requesting a release-exception as suggested by IanJ. These are memory leaks
which accumulate via the successful completion of libxl library functions (if
XSM is enabled), which will undoubtedly cause issues for the likes of libvirt
and xenopsd-libxl which use libxl in daemon processes.
Release-Acked-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
As we are very close to the release, I have opted for the more
obviously-correct code rather than the pragmatically better code, particularly
in two locations where libxl_domain_info() is called in a loop, and the
dispose()/init() pair is required to prevent leaking the allocation on each
iteration.
All of the uses of libxl_domain_info() patched here could better be an
xc_dominfo_getlist() which reduces the quantity of hypercalls made, and the
amount of data transformation done behind the scenes.
---
tools/libxl/libxl.c | 26 --
tools/libxl/libxl_dom.c | 14 ++
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 50a8928..372dd3b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -364,6 +364,8 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
char *uuid;
const char *vm_name_path;
+libxl_dominfo_init(info);
+
dom_path = libxl__xs_get_dompath(gc, domid);
if (!dom_path) goto x_nomem;
@@ -481,6 +483,7 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
rc = 0;
x_rc:
if (our_trans) xs_transaction_end(ctx-xsh, our_trans, 1);
+libxl_dominfo_dispose(info);
return rc;
x_fail: rc = ERROR_FAIL; goto x_rc;
@@ -4594,6 +4597,8 @@ static int libxl__fill_dom0_memory_info(libxl__gc *gc,
uint32_t *target_memkb,
libxl_ctx *ctx = libxl__gc_owner(gc);
uint32_t free_mem_slack_kb = 0;
+libxl_dominfo_init(info);
+
retry_transaction:
t = xs_transaction_start(ctx-xsh);
@@ -4626,6 +4631,8 @@ retry_transaction:
}
}
+libxl_dominfo_dispose(info);
+libxl_dominfo_init(info);
rc = libxl_domain_info(ctx, info, 0);
if (rc 0)
goto out;
@@ -4664,7 +4671,7 @@ out:
rc = ERROR_FAIL;
}
-
+libxl_dominfo_dispose(info);
return rc;
}
@@ -4999,6 +5006,8 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx,
uint32_t domid, int wait_secs)
uint32_t target_memkb = 0;
libxl_dominfo info;
+libxl_dominfo_init(info);
+
do {
wait_secs--;
sleep(1);
@@ -5007,9 +5016,11 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx,
uint32_t domid, int wait_secs)
if (rc 0)
goto out;
+libxl_dominfo_dispose(info);
+libxl_dominfo_init(info);
rc = libxl_domain_info(ctx, info, domid);
if (rc 0)
-return rc;
+goto out;
} while (wait_secs 0 (info.current_memkb + info.outstanding_memkb)
target_memkb);
if ((info.current_memkb + info.outstanding_memkb) = target_memkb)
@@ -5018,6 +5029,7 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx,
uint32_t domid, int wait_secs)
rc = ERROR_FAIL;
out:
+libxl_dominfo_dispose(info);
return rc;
}
@@ -5435,6 +5447,8 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc
*gc, uint32_t domid,
xs_transaction_t t;
int i, rc = ERROR_FAIL;
+libxl_dominfo_init(info);
+
if (libxl_domain_info(CTX, info, domid) 0) {
LOGE(ERROR, getting domain info list);
goto out;
@@ -5454,6 +5468,7 @@ retry_transaction:
} else
rc = 0;
out:
+libxl_dominfo_dispose(info);
return rc;
}
@@ -5463,8 +5478,11 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc,
uint32_t domid,
libxl_dominfo info;
int i;
+libxl_dominfo_init(info);
+
if (libxl_domain_info(CTX, info, domid) 0) {
LOGE(ERROR, getting domain info list);
+libxl_dominfo_dispose(info);
return ERROR_FAIL;
}
for (i = 0; i = info.vcpu_max_id; i++) {
@@ -5477,6 +5495,7 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc,
uint32_t domid,
libxl__qmp_cpu_add(gc, domid, i);
