All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-4.5] tools/libxl: Use of init()/dispose() to avoid leaking libxl_dominfo.ssid_label
@ 2015-01-05 14:19 Andrew Cooper
  2015-01-05 14:40 ` Wei Liu
  2015-01-05 15:33 ` Konrad Rzeszutek Wilk
  0 siblings, 2 replies; 4+ messages in thread
From: Andrew Cooper @ 2015-01-05 14:19 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu

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.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@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 = libxl_domain_info(ctx, &info, domid);
         if (rc) {
             LOG(ERROR, "fail to get domain info for domain %d", domid);
+            libxl_dominfo_dispose(&info);
             goto out;
         }
         libxl_uuid_copy(ctx, &d_config->c_info.uuid, &info.uuid);
+        libxl_dominfo_dispose(&info);
     }
 
     /* Memory limits:
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 74ea84b..378b620 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -2043,19 +2043,25 @@ const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
                                  const char *wh)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    char *uuid_string;
+    char *uuid_string, *path;
     libxl_dominfo info;
     int rc;
 
+    libxl_dominfo_init(&info);
+
     rc = libxl_domain_info(ctx, &info, domid);
     if (rc) {
         LOGE(ERROR, "unable to find domain info for domain %"PRIu32, domid);
-        return NULL;
+        path = NULL;
+        goto out;
     }
     uuid_string = GCSPRINTF(LIBXL_UUID_FMT, LIBXL_UUID_BYTES(info.uuid));
-
-    return GCSPRINTF("/var/lib/xen/userdata-%s.%u.%s.%s",
+    path = GCSPRINTF("/var/lib/xen/userdata-%s.%u.%s.%s",
                      wh, domid, uuid_string, userdata_userid);
+
+ out:
+    libxl_dominfo_dispose(&info);
+    return path;
 }
 
 static int userdata_delete(libxl__gc *gc, const char *path)
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-01-06 11:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-05 14:19 [PATCH for-4.5] tools/libxl: Use of init()/dispose() to avoid leaking libxl_dominfo.ssid_label Andrew Cooper
2015-01-05 14:40 ` Wei Liu
2015-01-05 15:33 ` Konrad Rzeszutek Wilk
2015-01-06 11:43   ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.