Xen devel - Jun 2011 - [PATCH v2] xen_disk: cope with missing xenstore "params" node

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


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

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

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

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.
--8323329-2008323187-1309182040=:12963
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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

--8323329-2008323187-1309182040=:12963--