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@xxxxxxxxx>
> ---
>  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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/