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

[Gonglei (Arei)]

> From: Markus Armbruster [mailto:address@hidden
> Sent: Thursday, September 18, 2014 3:38 PM
> Subject: Re: [Qemu-devel] [PATCH] usb-storage: fix possible memory leak and
> missing error message
> 
> <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.
> 
Hmm.. 

The main problem is I'm afraid the scenario when scsi_dev
is NULL the err is NULL too in someday. I have fixed a similar issue
and you have reviewed. :)
 
> scsi_hot_add() in pci-hotplug-old.c also loses the error message.  Would
> you care to fix that, too?

I have noticed that, but because scsi_hot_add() pass NULL to 
scsi_bus_legacy_add_drive(), so I let it pass. As you remainder,
I will post another patch to fix it. Thanks!


Best regards,
-Gonglei