* [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free
@ 2008-11-27 13:23 Julia Lawall
2008-11-27 13:33 ` Julia Lawall
0 siblings, 1 reply; 7+ messages in thread
From: Julia Lawall @ 2008-11-27 13:23 UTC (permalink / raw)
To: perex, tiwai, alsa-devel, linux-kernel, kernel-janitors
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;
}
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free 2008-11-27 13:23 [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free Julia Lawall @ 2008-11-27 13:33 ` Julia Lawall 2008-11-27 13:43 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Julia Lawall @ 2008-11-27 13:33 UTC (permalink / raw) To: perex, tiwai, alsa-devel, linux-kernel, kernel-janitors 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; > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free 2008-11-27 13:33 ` Julia Lawall @ 2008-11-27 13:43 ` Takashi Iwai 2008-11-27 14:01 ` Julia Lawall 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2008-11-27 13:43 UTC (permalink / raw) To: Julia Lawall; +Cc: perex, alsa-devel, linux-kernel, kernel-janitors 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free 2008-11-27 13:43 ` Takashi Iwai @ 2008-11-27 14:01 ` Julia Lawall 2008-11-27 14:36 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Julia Lawall @ 2008-11-27 14:01 UTC (permalink / raw) To: Takashi Iwai; +Cc: perex, alsa-devel, linux-kernel, kernel-janitors 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free 2008-11-27 14:01 ` Julia Lawall @ 2008-11-27 14:36 ` Takashi Iwai 2008-11-27 14:40 ` Julia Lawall 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2008-11-27 14:36 UTC (permalink / raw) To: Julia Lawall; +Cc: perex, alsa-devel, linux-kernel, kernel-janitors 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; > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free 2008-11-27 14:36 ` Takashi Iwai @ 2008-11-27 14:40 ` Julia Lawall 2008-11-27 15:04 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Julia Lawall @ 2008-11-27 14:40 UTC (permalink / raw) To: Takashi Iwai; +Cc: perex, alsa-devel, linux-kernel, kernel-janitors 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free 2008-11-27 14:40 ` Julia Lawall @ 2008-11-27 15:04 ` Takashi Iwai 0 siblings, 0 replies; 7+ messages in thread From: Takashi Iwai @ 2008-11-27 15:04 UTC (permalink / raw) To: Julia Lawall; +Cc: perex, alsa-devel, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-11-27 15:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-11-27 13:23 [PATCH] sound/pci/mixart/mixart.c: Add missing snd_card_free Julia Lawall 2008-11-27 13:33 ` Julia Lawall 2008-11-27 13:43 ` Takashi Iwai 2008-11-27 14:01 ` Julia Lawall 2008-11-27 14:36 ` Takashi Iwai 2008-11-27 14:40 ` Julia Lawall 2008-11-27 15:04 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®