WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-changelog

[Xen-changelog] [xen-unstable] libxl: fix error handling (xenstore trans

To: xen-changelog@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-changelog] [xen-unstable] libxl: fix error handling (xenstore transaction leak) in libxl__domain_make
From: Xen patchbot-unstable <patchbot@xxxxxxx>
Date: Sat, 29 Jan 2011 15:05:58 -0800
Delivery-date: Sat, 29 Jan 2011 15:13:10 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-changelog-request@lists.xensource.com?subject=help>
List-id: BK change log <xen-changelog.lists.xensource.com>
List-post: <mailto:xen-changelog@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=unsubscribe>
Reply-to: xen-devel@xxxxxxxxxxxxxxxxxxx
Sender: xen-changelog-bounces@xxxxxxxxxxxxxxxxxxx
# HG changeset patch
# User Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
# Date 1296239692 0
# Node ID c5a7a40cc4f43302100219e8ad0d7100004e5d77
# Parent  3e928084336fba1f4a7e666d0576f5943e63b874
libxl: fix error handling (xenstore transaction leak) in libxl__domain_make

libxl__domain_make could under some circumstances leak the xenstore
transaction (stored in the variable t).  Also, failures to commit the
xenstore transaction for reasons other than EAGAIN would be ignored (!)

Fix this as follows:

  * Initialise t to 0 (not a valid transaction id), and when the
    transaction is successfully committed or rolled back, reset it.

  * Change all the instances of:       libxl__free_all(&gc); return error;
    to instead do:                     rc=error;  goto out;

  * Use the out stanza for exiting, setting rc=0 on success first.

  * Explicitly abort the transaction in the out stanza.

Also add a note by the calls manipulating the gc, to note that as this
is an internal function, the gc should really be set up and destroyed
by its caller.  But let's not do that at this stage of the 4.1 release
cycle.

Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Committed-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
---
 tools/libxl/libxl_create.c |   51 ++++++++++++++++++++++++++-------------------
 1 files changed, 30 insertions(+), 21 deletions(-)

diff -r 3e928084336f -r c5a7a40cc4f4 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c        Fri Jan 28 17:56:00 2011 +0000
+++ b/tools/libxl/libxl_create.c        Fri Jan 28 18:34:52 2011 +0000
@@ -284,7 +284,7 @@ int libxl__domain_make(libxl_ctx *ctx, l
 int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
                        uint32_t *domid)
 {
-    libxl__gc gc = LIBXL_INIT_GC(ctx);
+    libxl__gc gc = LIBXL_INIT_GC(ctx); /* fixme: should be done by caller */
     int flags, ret, i, rc;
     char *uuid_string;
     char *rw_paths[] = { "device", "device/suspend/event-channel" , "data"};
@@ -293,13 +293,13 @@ int libxl__domain_make(libxl_ctx *ctx, l
     char *dom_path, *vm_path;
     struct xs_permissions roperm[2];
     struct xs_permissions rwperm[1];
-    xs_transaction_t t;
+    xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
     uuid_string = libxl__uuid2string(&gc, info->uuid);
     if (!uuid_string) {
-        libxl__free_all(&gc);
-        return ERROR_NOMEM;
+        rc = ERROR_NOMEM;
+        goto out;
     }
 
     flags = info->hvm ? XEN_DOMCTL_CDF_hvm_guest : 0;
@@ -313,28 +313,28 @@ int libxl__domain_make(libxl_ctx *ctx, l
     ret = xc_domain_create(ctx->xch, info->ssidref, handle, flags, domid);
     if (ret < 0) {
         LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain creation 
fail");
-        libxl__free_all(&gc);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
     if (ret < 0) {
         LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain move fail");
-        libxl__free_all(&gc);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     dom_path = libxl__xs_get_dompath(&gc, *domid);
     if (!dom_path) {
-        libxl__free_all(&gc);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     vm_path = libxl__sprintf(&gc, "/vm/%s", uuid_string);
     if (!vm_path) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
-        libxl__free_all(&gc);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     roperm[0].id = 0;
@@ -356,10 +356,8 @@ retry_transaction:
 
     xs_write(ctx->xsh, t, libxl__sprintf(&gc, "%s/vm", dom_path), vm_path, 
strlen(vm_path));
     rc = libxl_domain_rename(ctx, *domid, 0, info->name, t);
-    if (rc) {
-        libxl__free_all(&gc);
-        return rc;
-    }
+    if (rc)
+        goto out;
 
     for (i = 0; i < ARRAY_SIZE(rw_paths); i++) {
         char *path = libxl__sprintf(&gc, "%s/%s", dom_path, rw_paths[i]);
@@ -381,12 +379,23 @@ retry_transaction:
     libxl__xs_writev(&gc, t, libxl__sprintf(&gc, "%s/platform", dom_path), 
info->platformdata);
 
     xs_write(ctx->xsh, t, libxl__sprintf(&gc, 
"%s/control/platform-feature-multiprocessor-suspend", dom_path), "1", 1);
-    if (!xs_transaction_end(ctx->xsh, t, 0))
-        if (errno == EAGAIN)
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
+        if (errno == EAGAIN) {
+            t = 0;
             goto retry_transaction;
-
-    libxl__free_all(&gc);
-    return 0;
+        }
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain creation "
+                         "xenstore transaction commit failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    t = 0;
+
+    rc = 0;
+ out:
+    if (t) xs_transaction_end(ctx->xsh, t, 1);
+    libxl__free_all(&gc); /* fixme: should be done by caller */
+    return rc;
 }
 
 static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-changelog] [xen-unstable] libxl: fix error handling (xenstore transaction leak) in libxl__domain_make, Xen patchbot-unstable <=