[Qemu-devel] [PATCH] usb: fix usb_qdev_init() error handling again

[Qemu-devel] [PATCH] usb: fix usb_qdev_init() error handling again

[Stefan Hajnoczi]

Commit f462141f18ffdd75847f6459ef83d90b831d12c0 introduced clean up code
when usb_qdev_init() fails.  Unfortunately it calls .handle_destroy()
when .init() was never invoked or failed.  This can lead to crashes when
.handle_destroy() tries to clean up things that were never initialized.

This patch is careful to undo only those steps that completed along the
usb_qdev_init() code path.  It's not as pretty as the unified error
handling in f462141f18ffdd75847f6459ef83d90b831d12c0 but it's necessary.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/usb-bus.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 8cafb76..8203390 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -77,23 +77,21 @@ static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
     QLIST_INIT(&dev->strings);
     rc = usb_claim_port(dev);
     if (rc != 0) {
-        goto err;
+        return rc;
     }
     rc = dev->info->init(dev);
     if (rc != 0) {
-        goto err;
+        usb_release_port(dev);
+        return rc;
     }
     if (dev->auto_attach) {
         rc = usb_device_attach(dev);
         if (rc != 0) {
-            goto err;
+            usb_qdev_exit(qdev);
+            return rc;
         }
     }
     return 0;
-
-err:
-    usb_qdev_exit(qdev);
-    return rc;
 }
 
 static int usb_qdev_exit(DeviceState *qdev)
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH] usb: fix usb_qdev_init() error handling again

[Anthony Liguori]

On 12/15/2011 04:05 AM, Stefan Hajnoczi wrote:
> Commit f462141f18ffdd75847f6459ef83d90b831d12c0 introduced clean up code
> when usb_qdev_init() fails.  Unfortunately it calls .handle_destroy()
> when .init() was never invoked or failed.  This can lead to crashes when
> .handle_destroy() tries to clean up things that were never initialized.
>
> This patch is careful to undo only those steps that completed along the
> usb_qdev_init() code path.  It's not as pretty as the unified error
> handling in f462141f18ffdd75847f6459ef83d90b831d12c0 but it's necessary.
>
> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   hw/usb-bus.c |   12 +++++-------
>   1 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
> index 8cafb76..8203390 100644
> --- a/hw/usb-bus.c
> +++ b/hw/usb-bus.c
> @@ -77,23 +77,21 @@ static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
>       QLIST_INIT(&dev->strings);
>       rc = usb_claim_port(dev);
>       if (rc != 0) {
> -        goto err;
> +        return rc;
>       }
>       rc = dev->info->init(dev);
>       if (rc != 0) {
> -        goto err;
> +        usb_release_port(dev);
> +        return rc;
>       }
>       if (dev->auto_attach) {
>           rc = usb_device_attach(dev);
>           if (rc != 0) {
> -            goto err;
> +            usb_qdev_exit(qdev);
> +            return rc;
>           }
>       }
>       return 0;
> -
> -err:
> -    usb_qdev_exit(qdev);
> -    return rc;
>   }
>
>   static int usb_qdev_exit(DeviceState *qdev)