[Qemu-devel] [PATCH v2] xen_disk: cope with missing xenstore "params" node

[Qemu-devel] [PATCH v2] xen_disk: cope with missing xenstore "params" node

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

When disk is a cdrom and the drive is empty the "params" node in
xenstore might be missing completely: cope with it instead of
segfaulting.


Updated in v2:

- actually removed the strchr(blkdev->params, ':') that caused the
segfault;

- free all the allocated strings from xenstore before returning;

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen_disk.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 096d1c9..eec05dd 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -616,12 +616,14 @@ static int blk_init(struct XenDevice *xendev)
 {
     struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
     int index, qflags, have_barriers, info = 0;
-    char *h;
 
     /* read xenstore entries */
     if (blkdev->params == NULL) {
+        char *h = NULL;
         blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
-        h = strchr(blkdev->params, ':');
+        if (blkdev->params != NULL) {
+            h = strchr(blkdev->params, ':');
+        }
         if (h != NULL) {
             blkdev->fileproto = blkdev->params;
             blkdev->filename  = h+1;
@@ -649,7 +651,7 @@ static int blk_init(struct XenDevice *xendev)
         blkdev->mode == NULL   ||
         blkdev->type == NULL   ||
         blkdev->dev == NULL) {
-        return -1;
+        goto out_error;
     }
 
     /* read-only ? */
@@ -672,10 +674,15 @@ static int blk_init(struct XenDevice *xendev)
         /* setup via xenbus -> create new block driver instance */
         xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
         blkdev->bs = bdrv_new(blkdev->dev);
-        if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
-                      bdrv_find_whitelisted_format(blkdev->fileproto)) != 0) {
-            bdrv_delete(blkdev->bs);
-            return -1;
+        if (blkdev->bs) {
+            if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
+                        bdrv_find_whitelisted_format(blkdev->fileproto)) != 0) {
+                bdrv_delete(blkdev->bs);
+                blkdev->bs = NULL;
+            }
+        }
+        if (!blkdev->bs) {
+            goto out_error;
         }
     } else {
         /* setup via qemu cmdline -> already setup for us */
@@ -704,6 +711,14 @@ static int blk_init(struct XenDevice *xendev)
     xenstore_write_be_int(&blkdev->xendev, "sectors",
                           blkdev->file_size / blkdev->file_blk);
     return 0;
+
+out_error:
+    qemu_free(blkdev->params);
+    qemu_free(blkdev->mode);
+    qemu_free(blkdev->type);
+    qemu_free(blkdev->dev);
+    qemu_free(blkdev->devtype);
+    return -1;
 }
 
 static int blk_connect(struct XenDevice *xendev)
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH v2] xen_disk: cope with missing xenstore "params" node

[Peter Maydell]

On 24 June 2011 17:35,  <stefano.stabellini@eu.citrix.com> wrote:
> +out_error:
> +    qemu_free(blkdev->params);
> +    qemu_free(blkdev->mode);
> +    qemu_free(blkdev->type);
> +    qemu_free(blkdev->dev);
> +    qemu_free(blkdev->devtype);
> +    return -1;

It occured to me that could result in a double-free if it's
possible to call init again (or to call free) after the init
routine has returned failure. I don't know enough about the
Xen device lifecycle to know if that's possible, though -- is it?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] xen_disk: cope with missing xenstore "params" node

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On Sat, 25 Jun 2011, Peter Maydell wrote:
> On 24 June 2011 17:35,  <stefano.stabellini@eu.citrix.com> wrote:
> > +out_error:
> > +    qemu_free(blkdev->params);
> > +    qemu_free(blkdev->mode);
> > +    qemu_free(blkdev->type);
> > +    qemu_free(blkdev->dev);
> > +    qemu_free(blkdev->devtype);
> > +    return -1;
> 
> It occured to me that could result in a double-free if it's
> possible to call init again (or to call free) after the init
> routine has returned failure. I don't know enough about the
> Xen device lifecycle to know if that's possible, though -- is it?

It shouldn't happen, but xen_disk should be able to cope with it
nonetheless.
I am going to resend the patch again setting to NULL all the blkdev
fields after freeing them.