Re: [Qemu-devel] [PATCH] usb-storage: fix possible memory leak and missing error message

[Markus Armbruster]

<address@hidden> writes:

> From: Gonglei <address@hidden>
>
> When scsi_bus_legacy_add_drive() return NULL, meanwhile err will
> be not NULL, which will casue memory leak and missing error message.
>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  hw/usb/dev-storage.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 5c51ac0..cabd053 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -599,7 +599,6 @@ static int usb_msd_initfn_storage(USBDevice *dev)
>  {
>      MSDState *s = DO_UPCAST(MSDState, dev, dev);
>      BlockDriverState *bs = s->conf.bs;
> -    SCSIDevice *scsi_dev;
>      Error *err = NULL;
>  
>      if (!bs) {
> @@ -625,10 +624,12 @@ static int usb_msd_initfn_storage(USBDevice *dev)
>      usb_desc_init(dev);
>      scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
>                   &usb_msd_scsi_info_storage, NULL);
> -    scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable,
> -                                         s->conf.bootindex, dev->serial,
> -                                         &err);
> -    if (!scsi_dev) {
> +    scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable,
> +                              s->conf.bootindex, dev->serial,
> +                              &err);
> +    if (err) {
> +        error_report("%s", error_get_pretty(err));
> +        error_free(err);
>          return -1;
>      }
>      s->bus.qbus.allow_hotplug = 0;

I'd keep the original condition and just fix the error message loss /
error object leak:

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index ae4efcb..f731b0a 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -624,6 +624,8 @@ static int usb_msd_initfn_storage(USBDevice *dev)
                                          s->conf.bootindex, dev->serial,
                                          &err);
     if (!scsi_dev) {
+        error_report("%s", error_get_pretty(err));
+        error_free(err);
         return -1;
     }
     s->bus.qbus.allow_hotplug = 0;

Partly because it makes the fix more obvious, partly because I prefer
checking the function value when it is available.  Matter of taste.

scsi_hot_add() in pci-hotplug-old.c also loses the error message.  Would
you care to fix that, too?