From: Julia Lawall <[email protected]>
device and acpi_driver_data(device) were tested just a few lines above.
A simplified version of the semantic match that finds this problem is as
follows: (http://www.emn.fr/x-info/coccinelle/)
// <smpl>
@r exists@
local idexpression x;
expression E;
@@
if (x == NULL || ...) { ... when forall
return ...; }
... when != \(x=E\|x--\|x++\|--x\|++x\|x-=E\|x+=E\|x|=E\|x&=E\|&x\)
(
*x == NULL
|
*x != NULL
)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/platform/x86/fujitsu-laptop.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 218b9a1..5306901 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -745,9 +745,6 @@ static int acpi_fujitsu_remove(struct acpi_device *device, int type)
fujitsu = acpi_driver_data(device);
- if (!device || !acpi_driver_data(device))
- return -EINVAL;
-
fujitsu->acpi_handle = NULL;
return 0;
Julia Lawall wrote:
> [...]
> ---
> drivers/platform/x86/fujitsu-laptop.c | 3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 218b9a1..5306901 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -745,9 +745,6 @@ static int acpi_fujitsu_remove(struct acpi_device *device, int type)
>
> fujitsu = acpi_driver_data(device);
>
> - if (!device || !acpi_driver_data(device))
> - return -EINVAL;
> -
Shouldn't this still do a:
if (!fujitsu)
return -EINVAL;
to avoid dereferencing a NULL pointer below?
> fujitsu->acpi_handle = NULL;
>
> return 0;
--
Paulo Marques - http://www.grupopie.com
"All I ask is a chance to prove that money can't make me happy."
On Tue, 28 Jul 2009, Paulo Marques wrote:
> Julia Lawall wrote:
> > [...]
> > ---
> > drivers/platform/x86/fujitsu-laptop.c | 3 ---
> > 1 files changed, 0 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index 218b9a1..5306901 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -745,9 +745,6 @@ static int acpi_fujitsu_remove(struct acpi_device *device, int type)
> >
> > fujitsu = acpi_driver_data(device);
> >
> > - if (!device || !acpi_driver_data(device))
> > - return -EINVAL;
> > -
>
> Shouldn't this still do a:
>
> if (!fujitsu)
> return -EINVAL;
acpi_driver_data just accesses a field of its argument. Is there a worry
that from one call to the next it could have a different value?
Perhaps it would be better to first test !device, then initialize fujitsu,
and then test the result of fujitsu? The acpi_driver_data, which might
someday do something more complicated, would only be called once.
julia
Hi guys
> Julia Lawall wrote:
> > [...]
> > ---
> > drivers/platform/x86/fujitsu-laptop.c | 3 ---
> > 1 files changed, 0 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index 218b9a1..5306901 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -745,9 +745,6 @@ static int acpi_fujitsu_remove(struct acpi_device *device, int type)
> >
> > fujitsu = acpi_driver_data(device);
> >
> > - if (!device || !acpi_driver_data(device))
> > - return -EINVAL;
> > -
>
> Shouldn't this still do a:
>
> if (!fujitsu)
> return -EINVAL;
>
> to avoid dereferencing a NULL pointer below?
Hmm, yes it should. Well spotted. And I'm not certain how the duplicate
test on "device" got in there in the first place. I suspect it came about
due to some structural changes made a few versions ago and I failed to
notice that the second check became redundant.
So, combining this with the above patch we should instead do
Signed-off-by: [email protected] <Jonathan Woithe>
--- a/drivers/platform/x86/fujitsu-laptop.c 2009-06-12 19:51:45.333234000 +0930
+++ b/drivers/platform/x86/fujitsu-laptop.c 2009-07-29 10:14:30.610249941 +0930
@@ -745,7 +745,7 @@ static int acpi_fujitsu_remove(struct ac
fujitsu = acpi_driver_data(device);
- if (!device || !acpi_driver_data(device))
+ if (!fujitsu)
return -EINVAL;
fujitsu->acpi_handle = NULL;
Regards
jonathan
On Wed, 29 Jul 2009, Jonathan Woithe wrote:
> Hi guys
>
> > Julia Lawall wrote:
> > > [...]
> > > ---
> > > drivers/platform/x86/fujitsu-laptop.c | 3 ---
> > > 1 files changed, 0 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > > index 218b9a1..5306901 100644
> > > --- a/drivers/platform/x86/fujitsu-laptop.c
> > > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > > @@ -745,9 +745,6 @@ static int acpi_fujitsu_remove(struct acpi_device *device, int type)
> > >
> > > fujitsu = acpi_driver_data(device);
> > >
> > > - if (!device || !acpi_driver_data(device))
> > > - return -EINVAL;
> > > -
> >
> > Shouldn't this still do a:
> >
> > if (!fujitsu)
> > return -EINVAL;
> >
> > to avoid dereferencing a NULL pointer below?
>
> Hmm, yes it should. Well spotted. And I'm not certain how the duplicate
> test on "device" got in there in the first place. I suspect it came about
> due to some structural changes made a few versions ago and I failed to
> notice that the second check became redundant.
If you are going to check fujitsu afterwards, then I think there is no
need to test the result of acpi_driver_data before.
julia
> So, combining this with the above patch we should instead do
>
> Signed-off-by: [email protected] <Jonathan Woithe>
>
> --- a/drivers/platform/x86/fujitsu-laptop.c 2009-06-12 19:51:45.333234000 +0930
> +++ b/drivers/platform/x86/fujitsu-laptop.c 2009-07-29 10:14:30.610249941 +0930
> @@ -745,7 +745,7 @@ static int acpi_fujitsu_remove(struct ac
>
> fujitsu = acpi_driver_data(device);
>
> - if (!device || !acpi_driver_data(device))
> + if (!fujitsu)
> return -EINVAL;
>
> fujitsu->acpi_handle = NULL;
>
> Regards
> jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi Julia
> > > Julia Lawall wrote:
> > > > [...]
> > > > ---
> > > > drivers/platform/x86/fujitsu-laptop.c | 3 ---
> > > > 1 files changed, 0 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > > > index 218b9a1..5306901 100644
> > > > --- a/drivers/platform/x86/fujitsu-laptop.c
> > > > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > > > @@ -745,9 +745,6 @@ static int acpi_fujitsu_remove(struct acpi_device *device, int type)
> > > >
> > > > fujitsu = acpi_driver_data(device);
> > > >
> > > > - if (!device || !acpi_driver_data(device))
> > > > - return -EINVAL;
> > > > -
> > >
> > > Shouldn't this still do a:
> > >
> > > if (!fujitsu)
> > > return -EINVAL;
> > >
> > > to avoid dereferencing a NULL pointer below?
> >
> > Hmm, yes it should. Well spotted. And I'm not certain how the duplicate
> > test on "device" got in there in the first place. I suspect it came about
> > due to some structural changes made a few versions ago and I failed to
> > notice that the second check became redundant.
>
> If you are going to check fujitsu afterwards, then I think there is no
> need to test the result of acpi_driver_data before.
Yes, of course. I'll wake up soon, promise!
So we're left with this.
Signed-off-by: [email protected] <Jonathan Woithe>
--- a/drivers/platform/x86/fujitsu-laptop.c 2009-06-12 19:51:45.333234000 +0930
+++ b/drivers/platform/x86/fujitsu-laptop.c 2009-07-29 12:10:11.504901871 +0930
@@ -740,12 +740,12 @@ static int acpi_fujitsu_remove(struct ac
{
struct fujitsu_t *fujitsu = NULL;
- if (!device || !acpi_driver_data(device))
+ if (!device)
return -EINVAL;
fujitsu = acpi_driver_data(device);
- if (!device || !acpi_driver_data(device))
+ if (!fujitsu)
return -EINVAL;
fujitsu->acpi_handle = NULL;
Regards
jonathan
On Wed, 29 Jul 2009, Jonathan Woithe wrote:
> Hi Julia
>
> > > > Julia Lawall wrote:
> > > > > [...]
> > > > > ---
> > > > > drivers/platform/x86/fujitsu-laptop.c | 3 ---
> > > > > 1 files changed, 0 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > > > > index 218b9a1..5306901 100644
> > > > > --- a/drivers/platform/x86/fujitsu-laptop.c
> > > > > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > > > > @@ -745,9 +745,6 @@ static int acpi_fujitsu_remove(struct acpi_device *device, int type)
> > > > >
> > > > > fujitsu = acpi_driver_data(device);
> > > > >
> > > > > - if (!device || !acpi_driver_data(device))
> > > > > - return -EINVAL;
> > > > > -
> > > >
> > > > Shouldn't this still do a:
> > > >
> > > > if (!fujitsu)
> > > > return -EINVAL;
> > > >
> > > > to avoid dereferencing a NULL pointer below?
> > >
> > > Hmm, yes it should. Well spotted. And I'm not certain how the duplicate
> > > test on "device" got in there in the first place. I suspect it came about
> > > due to some structural changes made a few versions ago and I failed to
> > > notice that the second check became redundant.
> >
> > If you are going to check fujitsu afterwards, then I think there is no
> > need to test the result of acpi_driver_data before.
>
> Yes, of course. I'll wake up soon, promise!
>
> So we're left with this.
Looks fine now, thanks.
julia
> Signed-off-by: [email protected] <Jonathan Woithe>
>
> --- a/drivers/platform/x86/fujitsu-laptop.c 2009-06-12 19:51:45.333234000 +0930
> +++ b/drivers/platform/x86/fujitsu-laptop.c 2009-07-29 12:10:11.504901871 +0930
> @@ -740,12 +740,12 @@ static int acpi_fujitsu_remove(struct ac
> {
> struct fujitsu_t *fujitsu = NULL;
>
> - if (!device || !acpi_driver_data(device))
> + if (!device)
> return -EINVAL;
>
> fujitsu = acpi_driver_data(device);
>
> - if (!device || !acpi_driver_data(device))
> + if (!fujitsu)
> return -EINVAL;
>
> fujitsu->acpi_handle = NULL;
>
>
> Regards
> jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>