[PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free

[PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The function snd_mixart_create creates a link between mgr and card that
allows snd_mixart_free to free card as well.  But if snd_mixart_create
fails, then the link has not been created and card has to be freed explicitly.


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

// <smpl>
@r exists@
local idexpression x;
statement S,S1;
position p1,p2,p3;
expression E,E1;
type T,T1;
expression *ptr != NULL;
@@

(
 if ((x@p1 = snd_card_new(...)) == NULL) S
|
 x@p1 = snd_card_new(...);
)
 ... when != snd_card_free(...,(T)x,...)
     when != if (...) { <+... snd_card_free(...,(T)x,...) ...+> }
     when != true x == NULL || ...
     when != x = E
     when != E = (T)x
     when any
(
 if (x == NULL || ...) S1
|
 if@p2 (...) {
  ... when != snd_card_free(...,(T1)x,...)
      when != if (...) { <+... snd_card_free(...,(T1)x,...) ...+> }
      when != x = E1
      when != E1 = (T1)x
(
  return \(0\|<+...x...+>\|ptr\);
|
  return@p3 ...;
)
}
)

@ script:python @
p1 << r.p1;
p3 << r.p3;
@@

print "* file: %s snd_card_new: %s return: %s" % (p1[0].file,p1[0].line,p3[0].line)

// </smpl>

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

---
 sound/pci/mixart/mixart.c           |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/pci/mixart/mixart.c b/sound/pci/mixart/mixart.c
index ae7601f..1dd90f0 100644
--- a/sound/pci/mixart/mixart.c
+++ b/sound/pci/mixart/mixart.c
@@ -1377,6 +1377,7 @@ static int __devinit snd_mixart_probe(struct pci_dev *pci,
 		sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
 
 		if ((err = snd_mixart_create(mgr, card, i)) < 0) {
+			snd_card_free(card);
 			snd_mixart_free(mgr);
 			return err;
 		}

Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free

[Julia Lawall]

Oops, please ignore this patch.  There is a problem, but the modification 
is not correct.

julia


On Thu, 27 Nov 2008, Julia Lawall wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> The function snd_mixart_create creates a link between mgr and card that
> allows snd_mixart_free to free card as well.  But if snd_mixart_create
> fails, then the link has not been created and card has to be freed explicitly.
> 
> 
> The semantic match that finds the problem is as follows:
> (http://www.emn.fr/x-info/coccinelle/)
> 
> // <smpl>
> @r exists@
> local idexpression x;
> statement S,S1;
> position p1,p2,p3;
> expression E,E1;
> type T,T1;
> expression *ptr != NULL;
> @@
> 
> (
>  if ((x@p1 = snd_card_new(...)) == NULL) S
> |
>  x@p1 = snd_card_new(...);
> )
>  ... when != snd_card_free(...,(T)x,...)
>      when != if (...) { <+... snd_card_free(...,(T)x,...) ...+> }
>      when != true x == NULL || ...
>      when != x = E
>      when != E = (T)x
>      when any
> (
>  if (x == NULL || ...) S1
> |
>  if@p2 (...) {
>   ... when != snd_card_free(...,(T1)x,...)
>       when != if (...) { <+... snd_card_free(...,(T1)x,...) ...+> }
>       when != x = E1
>       when != E1 = (T1)x
> (
>   return \(0\|<+...x...+>\|ptr\);
> |
>   return@p3 ...;
> )
> }
> )
> 
> @ script:python @
> p1 << r.p1;
> p3 << r.p3;
> @@
> 
> print "* file: %s snd_card_new: %s return: %s" % (p1[0].file,p1[0].line,p3[0].line)
> 
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  sound/pci/mixart/mixart.c           |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/pci/mixart/mixart.c b/sound/pci/mixart/mixart.c
> index ae7601f..1dd90f0 100644
> --- a/sound/pci/mixart/mixart.c
> +++ b/sound/pci/mixart/mixart.c
> @@ -1377,6 +1377,7 @@ static int __devinit snd_mixart_probe(struct pci_dev *pci,
>  		sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
>  
>  		if ((err = snd_mixart_create(mgr, card, i)) < 0) {
> +			snd_card_free(card);
>  			snd_mixart_free(mgr);
>  			return err;
>  		}
> 

Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free

[Takashi Iwai]

At Thu, 27 Nov 2008 14:33:30 +0100 (CET),
Julia Lawall wrote:
> 
> Oops, please ignore this patch.  There is a problem, but the modification 
> is not correct.

How about the patch below?


Takashi

---
diff --git a/sound/pci/mixart/mixart.c b/sound/pci/mixart/mixart.c
index ae7601f..3cccfed 100644
--- a/sound/pci/mixart/mixart.c
+++ b/sound/pci/mixart/mixart.c
@@ -989,6 +989,7 @@ static int snd_mixart_pcm_digital(struct snd_mixart *chip)
 
 static int snd_mixart_chip_free(struct snd_mixart *chip)
 {
+	chip->mgr->chip[chip->chip_idx] = NULL;
 	kfree(chip);
 	return 0;
 }

Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free

[Julia Lawall]

On Thu, 27 Nov 2008, Takashi Iwai wrote:

> At Thu, 27 Nov 2008 14:33:30 +0100 (CET),
> Julia Lawall wrote:
> > 
> > Oops, please ignore this patch.  There is a problem, but the modification 
> > is not correct.
> 
> How about the patch below?

My solution was to move the initialization of mgr->chip[idx] down below 
the last error return in snd_mixart_create.  I think it depends on what is 
going to happen to the structure ops.  In your solution, if ops.dev_free 
is used later on, then the link between mgr and card will be broken, and 
no one will know to free card.

There is the same code and the same problem in sound/pci/pcxhr/pcxhr.c

I attach a proposed patch for both below.

julia

> Takashi
> 
> ---
> diff --git a/sound/pci/mixart/mixart.c b/sound/pci/mixart/mixart.c
> index ae7601f..3cccfed 100644
> --- a/sound/pci/mixart/mixart.c
> +++ b/sound/pci/mixart/mixart.c
> @@ -989,6 +989,7 @@ static int snd_mixart_pcm_digital(struct snd_mixart *chip)
>  
>  static int snd_mixart_chip_free(struct snd_mixart *chip)
>  {
> +	chip->mgr->chip[chip->chip_idx] = NULL;
>  	kfree(chip);
>  	return 0;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

---
 sound/pci/mixart/mixart.c |    4 +++-
 sound/pci/pcxhr/pcxhr.c   |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/pci/mixart/mixart.c b/sound/pci/mixart/mixart.c
index ae7601f..f23a735 100644
--- a/sound/pci/mixart/mixart.c
+++ b/sound/pci/mixart/mixart.c
@@ -1010,7 +1010,7 @@ static int __devinit snd_mixart_create(struct mixart_mgr *mgr, struct snd_card *
 		.dev_free = snd_mixart_chip_dev_free,
 	};
 
-	mgr->chip[idx] = chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (! chip) {
 		snd_printk(KERN_ERR "cannot allocate chip\n");
 		return -ENOMEM;
@@ -1025,6 +1025,7 @@ static int __devinit snd_mixart_create(struct mixart_mgr *mgr, struct snd_card *
 		return err;
 	}
 
+	mgr->chip[idx] = chip;
 	snd_card_set_dev(card, &mgr->pci->dev);
 
 	return 0;
@@ -1377,6 +1378,7 @@ static int __devinit snd_mixart_probe(struct pci_dev *pci,
 		sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
 
 		if ((err = snd_mixart_create(mgr, card, i)) < 0) {
+			snd_card_free(card);
 			snd_mixart_free(mgr);
 			return err;
 		}
diff --git a/sound/pci/pcxhr/pcxhr.c b/sound/pci/pcxhr/pcxhr.c
index 73de6e9..7d2b136 100644
--- a/sound/pci/pcxhr/pcxhr.c
+++ b/sound/pci/pcxhr/pcxhr.c
@@ -1024,7 +1024,7 @@ static int __devinit pcxhr_create(struct pcxhr_mgr *mgr, struct snd_card *card,
 		.dev_free = pcxhr_chip_dev_free,
 	};
 
-	mgr->chip[idx] = chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (! chip) {
 		snd_printk(KERN_ERR "cannot allocate chip\n");
 		return -ENOMEM;
@@ -1050,6 +1050,7 @@ static int __devinit pcxhr_create(struct pcxhr_mgr *mgr, struct snd_card *card,
 		return err;
 	}
 
+	mgr->chip[idx] = chip;
 	snd_card_set_dev(card, &mgr->pci->dev);
 
 	return 0;
@@ -1310,6 +1311,7 @@ static int __devinit pcxhr_probe(struct pci_dev *pci, const struct pci_device_id
 		sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
 
 		if ((err = pcxhr_create(mgr, card, i)) < 0) {
+			snd_card_free(card);
 			pcxhr_free(mgr);
 			return err;
 		}

Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free

[Takashi Iwai]

At Thu, 27 Nov 2008 15:01:51 +0100 (CET),
Julia Lawall wrote:
> 
> On Thu, 27 Nov 2008, Takashi Iwai wrote:
> 
> > At Thu, 27 Nov 2008 14:33:30 +0100 (CET),
> > Julia Lawall wrote:
> > > 
> > > Oops, please ignore this patch.  There is a problem, but the modification 
> > > is not correct.
> > 
> > How about the patch below?
> 
> My solution was to move the initialization of mgr->chip[idx] down below 
> the last error return in snd_mixart_create.  I think it depends on what is 
> going to happen to the structure ops.  In your solution, if ops.dev_free 
> is used later on, then the link between mgr and card will be broken, and 
> no one will know to free card.
> 
> There is the same code and the same problem in sound/pci/pcxhr/pcxhr.c
> 
> I attach a proposed patch for both below.

Looks good.  Could you repost with a readable comment?


thanks,

Takashi

> 
> julia
> 
> > Takashi
> > 
> > ---
> > diff --git a/sound/pci/mixart/mixart.c b/sound/pci/mixart/mixart.c
> > index ae7601f..3cccfed 100644
> > --- a/sound/pci/mixart/mixart.c
> > +++ b/sound/pci/mixart/mixart.c
> > @@ -989,6 +989,7 @@ static int snd_mixart_pcm_digital(struct snd_mixart *chip)
> >  
> >  static int snd_mixart_chip_free(struct snd_mixart *chip)
> >  {
> > +	chip->mgr->chip[chip->chip_idx] = NULL;
> >  	kfree(chip);
> >  	return 0;
> >  }
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> ---
>  sound/pci/mixart/mixart.c |    4 +++-
>  sound/pci/pcxhr/pcxhr.c   |    4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/mixart/mixart.c b/sound/pci/mixart/mixart.c
> index ae7601f..f23a735 100644
> --- a/sound/pci/mixart/mixart.c
> +++ b/sound/pci/mixart/mixart.c
> @@ -1010,7 +1010,7 @@ static int __devinit snd_mixart_create(struct mixart_mgr *mgr, struct snd_card *
>  		.dev_free = snd_mixart_chip_dev_free,
>  	};
>  
> -	mgr->chip[idx] = chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>  	if (! chip) {
>  		snd_printk(KERN_ERR "cannot allocate chip\n");
>  		return -ENOMEM;
> @@ -1025,6 +1025,7 @@ static int __devinit snd_mixart_create(struct mixart_mgr *mgr, struct snd_card *
>  		return err;
>  	}
>  
> +	mgr->chip[idx] = chip;
>  	snd_card_set_dev(card, &mgr->pci->dev);
>  
>  	return 0;
> @@ -1377,6 +1378,7 @@ static int __devinit snd_mixart_probe(struct pci_dev *pci,
>  		sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
>  
>  		if ((err = snd_mixart_create(mgr, card, i)) < 0) {
> +			snd_card_free(card);
>  			snd_mixart_free(mgr);
>  			return err;
>  		}
> diff --git a/sound/pci/pcxhr/pcxhr.c b/sound/pci/pcxhr/pcxhr.c
> index 73de6e9..7d2b136 100644
> --- a/sound/pci/pcxhr/pcxhr.c
> +++ b/sound/pci/pcxhr/pcxhr.c
> @@ -1024,7 +1024,7 @@ static int __devinit pcxhr_create(struct pcxhr_mgr *mgr, struct snd_card *card,
>  		.dev_free = pcxhr_chip_dev_free,
>  	};
>  
> -	mgr->chip[idx] = chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>  	if (! chip) {
>  		snd_printk(KERN_ERR "cannot allocate chip\n");
>  		return -ENOMEM;
> @@ -1050,6 +1050,7 @@ static int __devinit pcxhr_create(struct pcxhr_mgr *mgr, struct snd_card *card,
>  		return err;
>  	}
>  
> +	mgr->chip[idx] = chip;
>  	snd_card_set_dev(card, &mgr->pci->dev);
>  
>  	return 0;
> @@ -1310,6 +1311,7 @@ static int __devinit pcxhr_probe(struct pci_dev *pci, const struct pci_device_id
>  		sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
>  
>  		if ((err = pcxhr_create(mgr, card, i)) < 0) {
> +			snd_card_free(card);
>  			pcxhr_free(mgr);
>  			return err;
>  		}
> 

Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The function snd_mixart_create creates a link between mgr and card that
allows snd_mixart_free to free card as well.  But if snd_mixart_create
fails, then the link has not been created and card has to be freed explicitly.


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

// <smpl>
@r exists@
local idexpression x;
statement S,S1;
position p1,p2,p3;
expression E,E1;
type T,T1;
expression *ptr != NULL;
@@

(
 if ((x@p1 = snd_card_new(...)) == NULL) S
|
 x@p1 = snd_card_new(...);
)
 ... when != snd_card_free(...,(T)x,...)
     when != if (...) { <+... snd_card_free(...,(T)x,...) ...+> }
     when != true x == NULL || ...
     when != x = E
     when != E = (T)x
     when any
(
 if (x == NULL || ...) S1
|
 if@p2 (...) {
  ... when != snd_card_free(...,(T1)x,...)
      when != if (...) { <+... snd_card_free(...,(T1)x,...) ...+> }
      when != x = E1
      when != E1 = (T1)x
(
  return \(0\|<+...x...+>\|ptr\);
|
  return@p3 ...;
)
}
)

@ script:python @
p1 << r.p1;
p3 << r.p3;
@@

print "* file: %s snd_card_new: %s return: %s" % (p1[0].file,p1[0].line,p3[0].line)

// </smpl>

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

---
 sound/pci/mixart/mixart.c |    4 +++-
 sound/pci/pcxhr/pcxhr.c   |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/pci/mixart/mixart.c b/sound/pci/mixart/mixart.c
index ae7601f..f23a735 100644
--- a/sound/pci/mixart/mixart.c
+++ b/sound/pci/mixart/mixart.c
@@ -1010,7 +1010,7 @@ static int __devinit snd_mixart_create(struct mixart_mgr *mgr, struct snd_card *
 		.dev_free = snd_mixart_chip_dev_free,
 	};
 
-	mgr->chip[idx] = chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (! chip) {
 		snd_printk(KERN_ERR "cannot allocate chip\n");
 		return -ENOMEM;
@@ -1025,6 +1025,7 @@ static int __devinit snd_mixart_create(struct mixart_mgr *mgr, struct snd_card *
 		return err;
 	}
 
+	mgr->chip[idx] = chip;
 	snd_card_set_dev(card, &mgr->pci->dev);
 
 	return 0;
@@ -1377,6 +1378,7 @@ static int __devinit snd_mixart_probe(struct pci_dev *pci,
 		sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
 
 		if ((err = snd_mixart_create(mgr, card, i)) < 0) {
+			snd_card_free(card);
 			snd_mixart_free(mgr);
 			return err;
 		}
diff --git a/sound/pci/pcxhr/pcxhr.c b/sound/pci/pcxhr/pcxhr.c
index 73de6e9..7d2b136 100644
--- a/sound/pci/pcxhr/pcxhr.c
+++ b/sound/pci/pcxhr/pcxhr.c
@@ -1024,7 +1024,7 @@ static int __devinit pcxhr_create(struct pcxhr_mgr *mgr, struct snd_card *card,
 		.dev_free = pcxhr_chip_dev_free,
 	};
 
-	mgr->chip[idx] = chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (! chip) {
 		snd_printk(KERN_ERR "cannot allocate chip\n");
 		return -ENOMEM;
@@ -1050,6 +1050,7 @@ static int __devinit pcxhr_create(struct pcxhr_mgr *mgr, struct snd_card *card,
 		return err;
 	}
 
+	mgr->chip[idx] = chip;
 	snd_card_set_dev(card, &mgr->pci->dev);
 
 	return 0;
@@ -1310,6 +1311,7 @@ static int __devinit pcxhr_probe(struct pci_dev *pci, const struct pci_device_id
 		sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
 
 		if ((err = pcxhr_create(mgr, card, i)) < 0) {
+			snd_card_free(card);
 			pcxhr_free(mgr);
 			return err;
 		}

Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free

[Takashi Iwai]

At Thu, 27 Nov 2008 15:40:40 +0100 (CET),
Julia Lawall wrote:
> 
> From: Julia Lawall <julia@diku.dk>
> 
> The function snd_mixart_create creates a link between mgr and card that
> allows snd_mixart_free to free card as well.  But if snd_mixart_create
> fails, then the link has not been created and card has to be freed explicitly.
> 
> 
> The semantic match that finds the problem is as follows:
> (http://www.emn.fr/x-info/coccinelle/)
(snip)
> Signed-off-by: Julia Lawall <julia@diku.dk>

Thanks, applied now.


Takashi