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

[Markus Armbruster]

"Gonglei (Arei)" <address@hidden> writes:

>> 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. :)

Short answer: doesn't help, and you shouldn't be afraid anyway.

Now the long answer :)

Basic rules about returning errors through Error **errp parameters:

1. The errp argument may be null.

2. If it isn't, then it points to null.

3. The function either succeeds or fails.

4. If the function succeeds, it doesn't touch *errp.

5. If the function fails, and

   a. if errp isn't null, it creates an Error object and returns it in
   *errp.  The caller is responsible for freeing it.

   b. if errp is null, no Error object is created.

Corollary: when you pass a non-null errp argument, *errp is null exactly
on success.

Rule 3 connects the Error contract with what else the function's
supposed to do on success and on failure.  In particular with the return
value.

Common case: a function returns a pointer to something on success, null
pointer on failure.  When you pass a non-null errp, argument, *errp is
null exactly when the function value is non-null.  In other words,
either the value or *errp is null.

To detect errors, you can either check the function value, or your errp
argument.

If you detect an error, you can either handle it locally, or propagate
it to your caller.

Example: consider a function new_foo() returning a newly allocated Foo
on success, null on error.  Any error we want to propagate.

Checking the errp argument:

    Error *local_err = NULL;

    foop = new_foo(arg, &local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        // rely on postcondition !foop (else we'd leak it here)
        return;
    }

Checking the function value:

    Error *local_err = NULL;

    foop = new_foo(arg, &local_err);
    if (!foop) {
        error_propagate(errp, local_err);
        return;
    }
    // rely on postcondition !local_err (else, we'd leak it here)

No matter which of the two you check, you rely on new_foo()'s
postcondition "either value or *errp is null".

Therefore, switching from one to the other does not make the code more
robust.

Checking the function value can be simplified to just

    foop = new_foo(arg, errp);
    if (!foop) {
        return;
    }

I use this whenever possible, because it's simpler, and less clutter.

What you fixed is a different, less common case: a function returning a
list on success, null on failure, where the empty list is represented as
null.  Because a null value can be legitimately returned both on success
and on failure, you need to examine *errp to tell the to apart.  The
code you fixed didn't.

>> 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!

Appreciated!