| Message ID | 87k351idjy.fsf@blackfin.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
> From: Markus Armbruster [mailto:armbru@redhat.com] > Sent: Thursday, September 18, 2014 3:38 PM > Subject: Re: [Qemu-devel] [PATCH] usb-storage: fix possible memory leak and > missing error message > > <arei.gonglei@huawei.com> writes: > > > From: Gonglei <arei.gonglei@huawei.com> > > > > 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 <arei.gonglei@huawei.com> > > --- > > 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
"Gonglei (Arei)" <arei.gonglei@huawei.com> writes: >> From: Markus Armbruster [mailto:armbru@redhat.com] >> Sent: Thursday, September 18, 2014 3:38 PM >> Subject: Re: [Qemu-devel] [PATCH] usb-storage: fix possible memory leak and >> missing error message >> >> <arei.gonglei@huawei.com> writes: >> >> > From: Gonglei <arei.gonglei@huawei.com> >> > >> > 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 <arei.gonglei@huawei.com> >> > --- >> > 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!
> >> > --- > >> > 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. > Great! Thanks a lot! V2 will be posted shortly! BTW, I will include this patch in my usb patch series (usb-bus: convert init to realize). Best regards, -Gonglei > >> 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!
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;