2008-01-28 22:19:28

by Adrian Bunk

Subject: media/radio/radio-si470x.c: check-after-use

The Coverity checker spotted the following check-after-use in
drivers/media/radio/radio-si470x.c:

<-- snip -->

...
static void si470x_usb_driver_disconnect(struct usb_interface *intf)
{
struct si470x_device *radio = usb_get_intfdata(intf);

del_timer_sync(&radio->timer); <------------------
flush_scheduled_work();

usb_set_intfdata(intf, NULL);
if (radio) { <------------------
video_unregister_device(radio->videodev);
kfree(radio->buffer);
kfree(radio);
}
}
...

<-- snip -->

Either "radio" can be NULL and this case has to be properly handled or
the NULL check is not required.

cu
Adrian

--

"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed

2008-01-28 22:43:31

by Tobias Lorenz

Subject: [PATCH] radio-si470x.c: check-after-use

Hi Mauro,
Hi Adrian,

Adrian used the coverity checker against radio-si470x and found this:

> The Coverity checker spotted the following check-after-use in
> drivers/media/radio/radio-si470x.c:
>
> <-- snip -->
> static void si470x_usb_driver_disconnect(struct usb_interface *intf)
> {
> struct si470x_device *radio = usb_get_intfdata(intf);
>
> del_timer_sync(&radio->timer); <------------------
> flush_scheduled_work();
>
> usb_set_intfdata(intf, NULL);
> if (radio) { <------------------
> video_unregister_device(radio->videodev);
> kfree(radio->buffer);
> kfree(radio);
> }
> }
> <-- snip -->
>
> Either "radio" can be NULL and this case has to be properly handled or
> the NULL check is not required.

These two lines should indeed better be inside the if statement. The patch for this is below.

Thanks,
Toby

Signed-off-by: Tobias Lorenz <[email protected]>
--- linux-2.6.23/drivers/media/radio/radio-si470x.c 2008-01-23 00:01:07.000000000 +0100
+++ linux-2.6.23.new/drivers/media/radio/radio-si470x.c 2008-01-27 15:31:42.000000000 +0100
@@ -1440,11 +1440,10 @@ static void si470x_usb_driver_disconnect
{
struct si470x_device *radio = usb_get_intfdata(intf);

- del_timer_sync(&radio->timer);
- flush_scheduled_work();
-
usb_set_intfdata(intf, NULL);
if (radio) {
+ del_timer_sync(&radio->timer);
+ flush_scheduled_work();
video_unregister_device(radio->videodev);
kfree(radio->buffer);
kfree(radio);

2008-01-29 19:38:17

by Oliver Neukum

Subject: Re: [PATCH] radio-si470x.c: check-after-use

Am Montag 28 Januar 2008 schrieb Tobias Lorenz:
> > Either "radio" can be NULL and this case has to be properly handled or
> > the NULL check is not required.
>
> These two lines should indeed better be inside the if statement. The patch for this is below.

No, in disconnect intfdata must be valid. Any check for NULL is wrong
there.

Regards
Oliver

2008-01-29 20:37:31

by Tobias Lorenz

Subject: Re: [PATCH] radio-si470x.c: check-after-use

Hi,

> > > Either "radio" can be NULL and this case has to be properly handled or
> > > the NULL check is not required.
> >
> > These two lines should indeed better be inside the if statement. The patch for this is below.
>
> No, in disconnect intfdata must be valid. Any check for NULL is wrong
> there.

Why hadn't anybody told me that earlier? :-)
I removed the complete check for radio==NULL.
See the patch below.

Thanks,
Toby


Signed-off-by: Tobias Lorenz <[email protected]>
--- linux-2.6.23/drivers/media/radio/radio-si470x.c 2008-01-23 00:01:07.000000000 +0100
+++ linux-2.6.23.new/drivers/media/radio/radio-si470x.c 2008-01-27 15:31:42.000000000 +0100
@@ -1441,13 +1441,11 @@ static void si470x_usb_driver_disconnect
struct si470x_device *radio = usb_get_intfdata(intf);

usb_set_intfdata(intf, NULL);
- if (radio) {
- del_timer_sync(&radio->timer);
- flush_scheduled_work();
- video_unregister_device(radio->videodev);
- kfree(radio->buffer);
- kfree(radio);
- }
+ del_timer_sync(&radio->timer);
+ flush_scheduled_work();
+ video_unregister_device(radio->videodev);
+ kfree(radio->buffer);
+ kfree(radio);
}