[PATCH 5/5] sound/aoa: Add kmalloc NULL tests

[PATCH 5/5] sound/aoa: Add kmalloc NULL tests

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Check that the result of kzalloc is not NULL before a dereference.

The semantic match that finds this problem is as follows:
(http://www.emn.fr/x-info/coccinelle/)

// <smpl>
@@
expression *x;
identifier f;
constant char *C;
@@

x = \(kmalloc\|kcalloc\|kzalloc\)(...);
... when != x == NULL
    when != x != NULL
    when != (x || ...)
(
kfree(x)
|
f(...,C,...,x,...)
|
*f(...,x,...)
|
*x->f
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 sound/aoa/core/gpio-pmf.c           |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/sound/aoa/core/gpio-pmf.c b/sound/aoa/core/gpio-pmf.c
index 5ca2220..b4439ce 100644
--- a/sound/aoa/core/gpio-pmf.c
+++ b/sound/aoa/core/gpio-pmf.c
@@ -182,6 +182,12 @@ static int pmf_set_notify(struct gpio_runtime *rt,
 	if (!old && notify) {
 		irq_client = kzalloc(sizeof(struct pmf_irq_client),
 				     GFP_KERNEL);
+		if (!irq_client) {
+			err = -ENOMEM;
+			printk(KERN_ERR "snd-aoa: gpio layer failed to"
+				" register %s irq (%d)\n", name, err);
+			goto out_unlock;
+		}
 		irq_client->data = notif;
 		irq_client->handler = pmf_handle_notify_irq;
 		irq_client->owner = THIS_MODULE;

Re: [PATCH 5/5] sound/aoa: Add kmalloc NULL tests

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On Thu, 2009-07-30 at 16:11 +0200, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> Check that the result of kzalloc is not NULL before a dereference.

>  		irq_client = kzalloc(sizeof(struct pmf_irq_client),
>  				     GFP_KERNEL);
> +		if (!irq_client) {
> +			err = -ENOMEM;
> +			printk(KERN_ERR "snd-aoa: gpio layer failed to"
> +				" register %s irq (%d)\n", name, err);
> +			goto out_unlock;
> +		}

Looks good, thanks, but I'd really drop the printk if only to not have
the string there, that doesn't really seem interesting.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 5/5] sound/aoa: Add kmalloc NULL tests

[Julia Lawall]

On Thu, 30 Jul 2009, Johannes Berg wrote:

> On Thu, 2009-07-30 at 16:11 +0200, Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Check that the result of kzalloc is not NULL before a dereference.
> 
> >  		irq_client = kzalloc(sizeof(struct pmf_irq_client),
> >  				     GFP_KERNEL);
> > +		if (!irq_client) {
> > +			err = -ENOMEM;
> > +			printk(KERN_ERR "snd-aoa: gpio layer failed to"
> > +				" register %s irq (%d)\n", name, err);
> > +			goto out_unlock;
> > +		}
> 
> Looks good, thanks, but I'd really drop the printk if only to not have
> the string there, that doesn't really seem interesting.

The printk is based on similar error handling code a few lines later:

               if (err) {
                        printk(KERN_ERR "snd-aoa: gpio layer failed to"
                                        " register %s irq (%d)\n", name, 
err);
                        kfree(irq_client);
                        goto out_unlock;
                }

Should the printk be removed in this case as well?  Or is it ok to fail 
silently in one case and not in the other?

julia

Re: [PATCH 5/5] sound/aoa: Add kmalloc NULL tests

[Takashi Iwai]

At Thu, 30 Jul 2009 16:29:54 +0200 (CEST),
Julia Lawall wrote:
> 
> On Thu, 30 Jul 2009, Johannes Berg wrote:
> 
> > On Thu, 2009-07-30 at 16:11 +0200, Julia Lawall wrote:
> > > From: Julia Lawall <julia@diku.dk>
> > > 
> > > Check that the result of kzalloc is not NULL before a dereference.
> > 
> > >  		irq_client = kzalloc(sizeof(struct pmf_irq_client),
> > >  				     GFP_KERNEL);
> > > +		if (!irq_client) {
> > > +			err = -ENOMEM;
> > > +			printk(KERN_ERR "snd-aoa: gpio layer failed to"
> > > +				" register %s irq (%d)\n", name, err);
> > > +			goto out_unlock;
> > > +		}
> > 
> > Looks good, thanks, but I'd really drop the printk if only to not have
> > the string there, that doesn't really seem interesting.
> 
> The printk is based on similar error handling code a few lines later:

But another problem is that the same error message is reused although
the error condition is totally different.  The kzalloc NULL isn't
about the registration error.  So, it's rather confusing.

However, for this particular error path, I agree with Johannes; we can
skip the error message since the error code ENOMEM is obvious.


thanks,

Takashi

> 
>                if (err) {
>                         printk(KERN_ERR "snd-aoa: gpio layer failed to"
>                                         " register %s irq (%d)\n", name, 
> err);
>                         kfree(irq_client);
>                         goto out_unlock;
>                 }
> 
> Should the printk be removed in this case as well?  Or is it ok to fail 
> silently in one case and not in the other?
> 
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 5/5] sound/aoa: Add kmalloc NULL tests

[Julia Lawall]

On Fri, 31 Jul 2009, Takashi Iwai wrote:

> At Thu, 30 Jul 2009 16:29:54 +0200 (CEST),
> Julia Lawall wrote:
> > 
> > On Thu, 30 Jul 2009, Johannes Berg wrote:
> > 
> > > On Thu, 2009-07-30 at 16:11 +0200, Julia Lawall wrote:
> > > > From: Julia Lawall <julia@diku.dk>
> > > > 
> > > > Check that the result of kzalloc is not NULL before a dereference.
> > > 
> > > >  		irq_client = kzalloc(sizeof(struct pmf_irq_client),
> > > >  				     GFP_KERNEL);
> > > > +		if (!irq_client) {
> > > > +			err = -ENOMEM;
> > > > +			printk(KERN_ERR "snd-aoa: gpio layer failed to"
> > > > +				" register %s irq (%d)\n", name, err);
> > > > +			goto out_unlock;
> > > > +		}
> > > 
> > > Looks good, thanks, but I'd really drop the printk if only to not have
> > > the string there, that doesn't really seem interesting.
> > 
> > The printk is based on similar error handling code a few lines later:
> 
> But another problem is that the same error message is reused although
> the error condition is totally different.  The kzalloc NULL isn't
> about the registration error.  So, it's rather confusing.
> 
> However, for this particular error path, I agree with Johannes; we can
> skip the error message since the error code ENOMEM is obvious.

OK, I will send a new patch.

julia

Re: [PATCH 5/5] sound/aoa: Add kmalloc NULL tests

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Check that the result of kzalloc is not NULL before a dereference.

The semantic match that finds this problem is as follows:
(http://www.emn.fr/x-info/coccinelle/)

// <smpl>
@@
expression *x;
identifier f;
constant char *C;
@@

x = \(kmalloc\|kcalloc\|kzalloc\)(...);
... when != x == NULL
    when != x != NULL
    when != (x || ...)
(
kfree(x)
|
f(...,C,...,x,...)
|
*f(...,x,...)
|
*x->f
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 sound/aoa/core/gpio-pmf.c           |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/var/linuxes/linux-next/sound/aoa/core/gpio-pmf.c b/var/julia/linuxcopy/sound/aoa/core/gpio-pmf.c
index 5ca2220..1dd0c28 100644
--- a/var/linuxes/linux-next/sound/aoa/core/gpio-pmf.c
+++ b/var/julia/linuxcopy/sound/aoa/core/gpio-pmf.c
@@ -182,6 +182,10 @@ static int pmf_set_notify(struct gpio_runtime *rt,
 	if (!old && notify) {
 		irq_client = kzalloc(sizeof(struct pmf_irq_client),
 				     GFP_KERNEL);
+		if (!irq_client) {
+			err = -ENOMEM;
+			goto out_unlock;
+		}
 		irq_client->data = notif;
 		irq_client->handler = pmf_handle_notify_irq;
 		irq_client->owner = THIS_MODULE;

Re: [PATCH 5/5] sound/aoa: Add kmalloc NULL tests

[Takashi Iwai]

At Fri, 31 Jul 2009 08:32:03 +0200 (CEST),
Julia Lawall wrote:
> 
> From: Julia Lawall <julia@diku.dk>
> 
> Check that the result of kzalloc is not NULL before a dereference.
> 
> The semantic match that finds this problem is as follows:
> (http://www.emn.fr/x-info/coccinelle/)
> 
> // <smpl>
> @@
> expression *x;
> identifier f;
> constant char *C;
> @@
> 
> x = \(kmalloc\|kcalloc\|kzalloc\)(...);
> ... when != x == NULL
>     when != x != NULL
>     when != (x || ...)
> (
> kfree(x)
> |
> f(...,C,...,x,...)
> |
> *f(...,x,...)
> |
> *x->f
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>

Applied now.
But, please fix the path of the file correctly applicable to linux
kernel tree at the next time.  It includes /var/x/y, and confuses git
am totally.

thanks,

Takashi

> 
> ---
>  sound/aoa/core/gpio-pmf.c           |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/var/linuxes/linux-next/sound/aoa/core/gpio-pmf.c b/var/julia/linuxcopy/sound/aoa/core/gpio-pmf.c
> index 5ca2220..1dd0c28 100644
> --- a/var/linuxes/linux-next/sound/aoa/core/gpio-pmf.c
> +++ b/var/julia/linuxcopy/sound/aoa/core/gpio-pmf.c
> @@ -182,6 +182,10 @@ static int pmf_set_notify(struct gpio_runtime *rt,
>  	if (!old && notify) {
>  		irq_client = kzalloc(sizeof(struct pmf_irq_client),
>  				     GFP_KERNEL);
> +		if (!irq_client) {
> +			err = -ENOMEM;
> +			goto out_unlock;
> +		}
>  		irq_client->data = notif;
>  		irq_client->handler = pmf_handle_notify_irq;
>  		irq_client->owner = THIS_MODULE;
>