[ALSA] seq: resource leak fix and various code cleanups

[ALSA] seq: resource leak fix and various code cleanups

[Eugene Teo]

This patch fixes:
1) a resource leak (CID: 1817)
2) various code cleanups

Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 sound/core/seq/oss/seq_oss_init.c   |   29 ++++++++++++++++++-----------
 sound/core/seq/oss/seq_oss_writeq.c |    6 ++++--
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/sound/core/seq/oss/seq_oss_init.c b/sound/core/seq/oss/seq_oss_init.c
index ca5a2ed..c9b95c3 100644
--- a/sound/core/seq/oss/seq_oss_init.c
+++ b/sound/core/seq/oss/seq_oss_init.c
@@ -176,7 +176,8 @@ snd_seq_oss_open(struct file *file, int level)
 	int i, rc;
 	struct seq_oss_devinfo *dp;
 
-	if ((dp = kzalloc(sizeof(*dp), GFP_KERNEL)) == NULL) {
+	dp = kzalloc(sizeof(*dp), GFP_KERNEL);
+	if (dp == NULL) {
 		snd_printk(KERN_ERR "can't malloc device info\n");
 		return -ENOMEM;
 	}
@@ -188,8 +189,8 @@ snd_seq_oss_open(struct file *file, int level)
 	}
 	if (i >= SNDRV_SEQ_OSS_MAX_CLIENTS) {
 		snd_printk(KERN_ERR "too many applications\n");
-		kfree(dp);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto _error;
 	}
 
 	dp->index = i;
@@ -211,14 +212,16 @@ snd_seq_oss_open(struct file *file, int level)
 
 	/* create port */
 	debug_printk(("create new port\n"));
-	if ((rc = create_port(dp)) < 0) {
+	rc = create_port(dp);
+	if (rc < 0) {
 		snd_printk(KERN_ERR "can't create port\n");
 		goto _error;
 	}
 
 	/* allocate queue */
 	debug_printk(("allocate queue\n"));
-	if ((rc = alloc_seq_queue(dp)) < 0)
+	rc = alloc_seq_queue(dp);
+	if (rc < 0)
 		goto _error;
 
 	/* set address */
@@ -235,7 +238,8 @@ snd_seq_oss_open(struct file *file, int level)
 	/* initialize read queue */
 	debug_printk(("initialize read queue\n"));
 	if (is_read_mode(dp->file_mode)) {
-		if ((dp->readq = snd_seq_oss_readq_new(dp, maxqlen)) == NULL) {
+		dp->readq = snd_seq_oss_readq_new(dp, maxqlen);
+		if (dp->readq == NULL) {
 			rc = -ENOMEM;
 			goto _error;
 		}
@@ -253,7 +257,8 @@ snd_seq_oss_open(struct file *file, int level)
 
 	/* initialize timer */
 	debug_printk(("initialize timer\n"));
-	if ((dp->timer = snd_seq_oss_timer_new(dp)) == NULL) {
+	dp->timer = snd_seq_oss_timer_new(dp);
+	if (dp->timer == NULL) {
 		snd_printk(KERN_ERR "can't alloc timer\n");
 		rc = -ENOMEM;
 		goto _error;
@@ -276,11 +281,13 @@ snd_seq_oss_open(struct file *file, int level)
 	return 0;
 
  _error:
-	snd_seq_oss_synth_cleanup(dp);
-	snd_seq_oss_midi_cleanup(dp);
-	i = dp->queue;
+	snd_seq_oss_writeq_delete(dp->writeq);
+	snd_seq_oss_readq_delete(dp->readq);
+	delete_seq_queue(dp->queue);
 	delete_port(dp);
-	delete_seq_queue(i);
+	snd_seq_oss_midi_cleanup(dp);
+	snd_seq_oss_synth_cleanup(dp);
+	kfree(dp);
 
 	return rc;
 }
diff --git a/sound/core/seq/oss/seq_oss_writeq.c b/sound/core/seq/oss/seq_oss_writeq.c
index 5c84956..2174248 100644
--- a/sound/core/seq/oss/seq_oss_writeq.c
+++ b/sound/core/seq/oss/seq_oss_writeq.c
@@ -63,8 +63,10 @@ snd_seq_oss_writeq_new(struct seq_oss_devinfo *dp, int maxlen)
 void
 snd_seq_oss_writeq_delete(struct seq_oss_writeq *q)
 {
-	snd_seq_oss_writeq_clear(q);	/* to be sure */
-	kfree(q);
+	if (q) {
+		snd_seq_oss_writeq_clear(q);	/* to be sure */
+		kfree(q);
+	}
 }
 
 

Re: [ALSA] seq: resource leak fix and various code cleanups

[Takashi Iwai]

At Tue, 7 Aug 2007 16:40:48 +0800,
Eugene Teo wrote:
> 
> This patch fixes:
> 1) a resource leak (CID: 1817)
> 2) various code cleanups
> 
> Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
> ---
>  sound/core/seq/oss/seq_oss_init.c   |   29 ++++++++++++++++++-----------
>  sound/core/seq/oss/seq_oss_writeq.c |    6 ++++--
>  2 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/core/seq/oss/seq_oss_init.c b/sound/core/seq/oss/seq_oss_init.c
> index ca5a2ed..c9b95c3 100644
> --- a/sound/core/seq/oss/seq_oss_init.c
> +++ b/sound/core/seq/oss/seq_oss_init.c
> @@ -176,7 +176,8 @@ snd_seq_oss_open(struct file *file, int level)
>  	int i, rc;
>  	struct seq_oss_devinfo *dp;
>  
> -	if ((dp = kzalloc(sizeof(*dp), GFP_KERNEL)) == NULL) {
> +	dp = kzalloc(sizeof(*dp), GFP_KERNEL);
> +	if (dp == NULL) {

Better to use "if (!dp)"

>  		snd_printk(KERN_ERR "can't malloc device info\n");
>  		return -ENOMEM;
>  	}
> @@ -188,8 +189,8 @@ snd_seq_oss_open(struct file *file, int level)
>  	}
>  	if (i >= SNDRV_SEQ_OSS_MAX_CLIENTS) {
>  		snd_printk(KERN_ERR "too many applications\n");
> -		kfree(dp);
> -		return -ENOMEM;
> +		rc = -ENOMEM;
> +		goto _error;

This seems wrong.  It goes before initializing dp->queue = -1, so it
screws up delete_seq_queue().

> @@ -276,11 +281,13 @@ snd_seq_oss_open(struct file *file, int level)
>  	return 0;
>  
>   _error:
> -	snd_seq_oss_synth_cleanup(dp);
> -	snd_seq_oss_midi_cleanup(dp);
> -	i = dp->queue;
> +	snd_seq_oss_writeq_delete(dp->writeq);
> +	snd_seq_oss_readq_delete(dp->readq);
> +	delete_seq_queue(dp->queue);
>  	delete_port(dp);
> -	delete_seq_queue(i);
> +	snd_seq_oss_midi_cleanup(dp);
> +	snd_seq_oss_synth_cleanup(dp);
> +	kfree(dp);

The order of these calls is important.  Did you test it and confirm
that it has no side effects?


Takashi

Re: [ALSA] seq: resource leak fix and various code cleanups

[Eugene Teo]

Hi Takashi-san,

<quote sender="Takashi Iwai">
> At Tue, 7 Aug 2007 16:40:48 +0800,
> Eugene Teo wrote:
> > 
> > This patch fixes:
> > 1) a resource leak (CID: 1817)
> > 2) various code cleanups
[...]
> >  	if (i >= SNDRV_SEQ_OSS_MAX_CLIENTS) {
> >  		snd_printk(KERN_ERR "too many applications\n");
> > -		kfree(dp);
> > -		return -ENOMEM;
> > +		rc = -ENOMEM;
> > +		goto _error;
> 
> This seems wrong.  It goes before initializing dp->queue = -1, so it
> screws up delete_seq_queue().

You are right. The initialisations should come first.

> > @@ -276,11 +281,13 @@ snd_seq_oss_open(struct file *file, int level)
> >  	return 0;
> >  
> >   _error:
> > -	snd_seq_oss_synth_cleanup(dp);
> > -	snd_seq_oss_midi_cleanup(dp);
> > -	i = dp->queue;
> > +	snd_seq_oss_writeq_delete(dp->writeq);
> > +	snd_seq_oss_readq_delete(dp->readq);
> > +	delete_seq_queue(dp->queue);
> >  	delete_port(dp);
> > -	delete_seq_queue(i);
> > +	snd_seq_oss_midi_cleanup(dp);
> > +	snd_seq_oss_synth_cleanup(dp);
> > +	kfree(dp);
> 
> The order of these calls is important.  Did you test it and confirm
> that it has no side effects?

Only snd_seq_oss_synth_cleanup() and snd_seq_oss_midi_cleanup() should
be in order. The rest of the cleanup functions do not depend on one
another. Thanks for your advice.

Here's my second try:

This patch fixes:
1) a resource leak (CID: 1817)
2) various code cleanups

Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 sound/core/seq/oss/seq_oss_init.c   |   40 +++++++++++++++++++++--------------
 sound/core/seq/oss/seq_oss_writeq.c |    6 +++-
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/sound/core/seq/oss/seq_oss_init.c b/sound/core/seq/oss/seq_oss_init.c
index ca5a2ed..f26b751 100644
--- a/sound/core/seq/oss/seq_oss_init.c
+++ b/sound/core/seq/oss/seq_oss_init.c
@@ -176,29 +176,31 @@ snd_seq_oss_open(struct file *file, int level)
 	int i, rc;
 	struct seq_oss_devinfo *dp;
 
-	if ((dp = kzalloc(sizeof(*dp), GFP_KERNEL)) == NULL) {
+	dp = kzalloc(sizeof(*dp), GFP_KERNEL);
+	if (!dp) {
 		snd_printk(KERN_ERR "can't malloc device info\n");
 		return -ENOMEM;
 	}
 	debug_printk(("oss_open: dp = %p\n", dp));
 
+	dp->cseq = system_client;
+	dp->port = -1;
+	dp->queue = -1;
+	dp->readq = NULL;
+	dp->writeq = NULL;
+
 	for (i = 0; i < SNDRV_SEQ_OSS_MAX_CLIENTS; i++) {
 		if (client_table[i] == NULL)
 			break;
 	}
+
+	dp->index = i;
 	if (i >= SNDRV_SEQ_OSS_MAX_CLIENTS) {
 		snd_printk(KERN_ERR "too many applications\n");
-		kfree(dp);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto _error;
 	}
 
-	dp->index = i;
-	dp->cseq = system_client;
-	dp->port = -1;
-	dp->queue = -1;
-	dp->readq = NULL;
-	dp->writeq = NULL;
-
 	/* look up synth and midi devices */
 	snd_seq_oss_synth_setup(dp);
 	snd_seq_oss_midi_setup(dp);
@@ -211,14 +213,16 @@ snd_seq_oss_open(struct file *file, int level)
 
 	/* create port */
 	debug_printk(("create new port\n"));
-	if ((rc = create_port(dp)) < 0) {
+	rc = create_port(dp);
+	if (rc < 0) {
 		snd_printk(KERN_ERR "can't create port\n");
 		goto _error;
 	}
 
 	/* allocate queue */
 	debug_printk(("allocate queue\n"));
-	if ((rc = alloc_seq_queue(dp)) < 0)
+	rc = alloc_seq_queue(dp);
+	if (rc < 0)
 		goto _error;
 
 	/* set address */
@@ -235,7 +239,8 @@ snd_seq_oss_open(struct file *file, int level)
 	/* initialize read queue */
 	debug_printk(("initialize read queue\n"));
 	if (is_read_mode(dp->file_mode)) {
-		if ((dp->readq = snd_seq_oss_readq_new(dp, maxqlen)) == NULL) {
+		dp->readq = snd_seq_oss_readq_new(dp, maxqlen);
+		if (dp->readq == NULL) {
 			rc = -ENOMEM;
 			goto _error;
 		}
@@ -253,7 +258,8 @@ snd_seq_oss_open(struct file *file, int level)
 
 	/* initialize timer */
 	debug_printk(("initialize timer\n"));
-	if ((dp->timer = snd_seq_oss_timer_new(dp)) == NULL) {
+	dp->timer = snd_seq_oss_timer_new(dp);
+	if (dp->timer == NULL) {
 		snd_printk(KERN_ERR "can't alloc timer\n");
 		rc = -ENOMEM;
 		goto _error;
@@ -276,11 +282,13 @@ snd_seq_oss_open(struct file *file, int level)
 	return 0;
 
  _error:
+	snd_seq_oss_writeq_delete(dp->writeq);
+	snd_seq_oss_readq_delete(dp->readq);
 	snd_seq_oss_synth_cleanup(dp);
 	snd_seq_oss_midi_cleanup(dp);
-	i = dp->queue;
 	delete_port(dp);
-	delete_seq_queue(i);
+	delete_seq_queue(dp->queue);
+	kfree(dp);
 
 	return rc;
 }
diff --git a/sound/core/seq/oss/seq_oss_writeq.c b/sound/core/seq/oss/seq_oss_writeq.c
index 5c84956..2174248 100644
--- a/sound/core/seq/oss/seq_oss_writeq.c
+++ b/sound/core/seq/oss/seq_oss_writeq.c
@@ -63,8 +63,10 @@ snd_seq_oss_writeq_new(struct seq_oss_devinfo *dp, int maxlen)
 void
 snd_seq_oss_writeq_delete(struct seq_oss_writeq *q)
 {
-	snd_seq_oss_writeq_clear(q);	/* to be sure */
-	kfree(q);
+	if (q) {
+		snd_seq_oss_writeq_clear(q);	/* to be sure */
+		kfree(q);
+	}
 }
 
 

Re: [ALSA] seq: resource leak fix and various code cleanups

[Takashi Iwai]

At Tue, 7 Aug 2007 18:52:49 +0800,
Eugene Teo wrote:
> 
> diff --git a/sound/core/seq/oss/seq_oss_init.c b/sound/core/seq/oss/seq_oss_init.c
> index ca5a2ed..f26b751 100644
> --- a/sound/core/seq/oss/seq_oss_init.c
> +++ b/sound/core/seq/oss/seq_oss_init.c
> @@ -176,29 +176,31 @@ snd_seq_oss_open(struct file *file, int level)
>  	int i, rc;
>  	struct seq_oss_devinfo *dp;
>  
> -	if ((dp = kzalloc(sizeof(*dp), GFP_KERNEL)) == NULL) {
> +	dp = kzalloc(sizeof(*dp), GFP_KERNEL);
> +	if (!dp) {
>  		snd_printk(KERN_ERR "can't malloc device info\n");
>  		return -ENOMEM;
>  	}
>  	debug_printk(("oss_open: dp = %p\n", dp));
>  
> +	dp->cseq = system_client;
> +	dp->port = -1;
> +	dp->queue = -1;
> +	dp->readq = NULL;
> +	dp->writeq = NULL;

NULL initialization is redundant, as it's kzalloced.

> @@ -235,7 +239,8 @@ snd_seq_oss_open(struct file *file, int level)
>  	/* initialize read queue */
>  	debug_printk(("initialize read queue\n"));
>  	if (is_read_mode(dp->file_mode)) {
> -		if ((dp->readq = snd_seq_oss_readq_new(dp, maxqlen)) == NULL) {
> +		dp->readq = snd_seq_oss_readq_new(dp, maxqlen);
> +		if (dp->readq == NULL) {

"if (!dp->readq)" is preferred.

> @@ -253,7 +258,8 @@ snd_seq_oss_open(struct file *file, int level)
>  
>  	/* initialize timer */
>  	debug_printk(("initialize timer\n"));
> -	if ((dp->timer = snd_seq_oss_timer_new(dp)) == NULL) {
> +	dp->timer = snd_seq_oss_timer_new(dp);
> +	if (dp->timer == NULL) {

Ditto.


I fixed these and applied your patch to ALSA tree now.
Thanks!


Takashi

Re: [ALSA] seq: resource leak fix and various code cleanups

[Eugene Teo]

<quote sender="Takashi Iwai">
> At Tue, 7 Aug 2007 18:52:49 +0800,
> Eugene Teo wrote:
[...]
> I fixed these and applied your patch to ALSA tree now.
> Thanks!

Thanks! Will take note next time.

Eugene