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

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

[Adrian Bunk]

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

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

[Tobias Lorenz]

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 <tobias.lorenz@gmx.net>
--- 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);

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

[Oliver Neukum]

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

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

[Tobias Lorenz]

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 <tobias.lorenz@gmx.net>
--- 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);
 }