[Xen-changelog] [xen-unstable] tools/libxl: fix memory management bugs in libxl_device_disk_list()

[Xen patchbot-unstable <patchbot-unstable@xxxxxxxxxxxxxxxxxxx>]

# HG changeset patch
# User Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
# Date 1281962278 -3600
# Node ID 1382296539fc73e259e96b1d8d59ab285a24712d
# Parent  be41e8ee8052a6d2a56161112375988a44f33ad2
tools/libxl: fix memory management bugs in libxl_device_disk_list()

fix invalid free segfault and use-after-free in libxl_device_disk_list()

Gah, libxl_device_disk_list() is returning a lot of pointers to free'd
data. Fix that by replacing libxl_xs_read() with xs_read() in line with
the policy.

Also fix a segfault caused by an erroneous free of the last disk-list
array element rather than the first one. This was causing xl create to
segfault when using the new qemu-dm code-base. Fix that and add a
comment about the fact that this libxl API requires a corresponding
libxl_device_disk_free() function.

Signed-off-by: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
---
 tools/libxl/libxl.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff -r be41e8ee8052 -r 1382296539fc tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Mon Aug 16 13:33:54 2010 +0100
+++ b/tools/libxl/libxl.c       Mon Aug 16 13:37:58 2010 +0100
@@ -1315,17 +1315,17 @@ static char ** libxl_build_device_model_
         flexarray_set(dm_args, num++, "xenfv");
 
     disks = libxl_device_disk_list(libxl_gc_owner(gc), info->domid, &nb);
-    for (; nb > 0; --nb, ++disks) {
-        if ( disks->is_cdrom ) {
+    for (i; i < nb; i++) {
+        if ( disks[i].is_cdrom ) {
             flexarray_set(dm_args, num++, "-cdrom");
-            flexarray_set(dm_args, num++, disks->physpath);
+            flexarray_set(dm_args, num++, disks[i].physpath);
         }else{
-            flexarray_set(dm_args, num++, libxl_sprintf(gc, "-%s", 
disks->virtpath));
-            flexarray_set(dm_args, num++, disks->physpath);
-        }
-    }
+            flexarray_set(dm_args, num++, libxl_sprintf(gc, "-%s", 
disks[i].virtpath));
+            flexarray_set(dm_args, num++, disks[i].physpath);
+        }
+    }
+    /* FIXME: leaks disk paths */
     free(disks);
-
     flexarray_set(dm_args, num++, NULL);
     return (char **) flexarray_contents(dm_args);
 }
@@ -2462,7 +2462,7 @@ libxl_device_disk *libxl_device_disk_lis
     char *be_path_tap, *be_path_vbd;
     libxl_device_disk *dend, *disks, *ret = NULL;
     char **b, **l = NULL;
-    unsigned int numl;
+    unsigned int numl, len;
     char *type;
 
     be_path_vbd = libxl_sprintf(&gc, "%s/backend/vbd/%d", 
libxl_xs_get_dompath(&gc, 0), domid);
@@ -2477,9 +2477,9 @@ libxl_device_disk *libxl_device_disk_lis
         for (; disks < dend; ++disks, ++l) {
             disks->backend_domid = 0;
             disks->domid = domid;
-            disks->physpath = libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/params", be_path_vbd, *l));
+            disks->physpath = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/params", be_path_vbd, *l), &len);
             libxl_string_to_phystype(ctx, libxl_xs_read(&gc, XBT_NULL, 
libxl_sprintf(&gc, "%s/%s/type", be_path_vbd, *l)), &(disks->phystype));
-            disks->virtpath = libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/dev", be_path_vbd, *l));
+            disks->virtpath = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/dev", be_path_vbd, *l), &len);
             disks->unpluggable = atoi(libxl_xs_read(&gc, XBT_NULL, 
libxl_sprintf(&gc, "%s/%s/removable", be_path_vbd, *l)));
             if (!strcmp(libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/mode", be_path_vbd, *l)), "w"))
                 disks->readwrite = 1;
@@ -2497,9 +2497,9 @@ libxl_device_disk *libxl_device_disk_lis
         for (dend = ret + *num; disks < dend; ++disks, ++l) {
             disks->backend_domid = 0;
             disks->domid = domid;
-            disks->physpath = libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/params", be_path_tap, *l));
+            disks->physpath = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/params", be_path_tap, *l), &len);
             libxl_string_to_phystype(ctx, libxl_xs_read(&gc, XBT_NULL, 
libxl_sprintf(&gc, "%s/%s/type", be_path_tap, *l)), &(disks->phystype));
-            disks->virtpath = libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/dev", be_path_tap, *l));
+            disks->virtpath = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/dev", be_path_tap, *l), &len);
             disks->unpluggable = atoi(libxl_xs_read(&gc, XBT_NULL, 
libxl_sprintf(&gc, "%s/%s/removable", be_path_tap, *l)));
             if (!strcmp(libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, 
"%s/%s/mode", be_path_tap, *l)), "w"))
                 disks->readwrite = 1;
@@ -2579,6 +2579,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
         libxl_device_disk_add(ctx, stubdomid, disk);
         disk->domid = domid;
     }
+    /* FIXME: leaks disk paths */
     free(disks);
     return 0;
 }

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