Re: [PATCH] staging: altera-stapl: Fix memory leak ofaltera_init()

[Jesper Juhl]

On Tue, 31 May 2011, Peter Hüwe wrote:

> Am Montag 30 Mai 2011, 22:45:04 schrieb Andre Bartke:
> > In case kzalloc() fails the second or third time
> > we should free the previous allocated resources.
> 
> Good catch!
> Personally I prefer putting the cleanup logic to the bottom, maybe like this - but that's just personal preference.
> 

Another alternative would be something like this:

        key = kzalloc(33 * sizeof(char), GFP_KERNEL);
        value = kzalloc(257 * sizeof(char), GFP_KERNEL);
        astate = kzalloc(sizeof(struct altera_state), GFP_KERNEL);
	if (!astate || !value || ! key) {
		kfree(astate);
		kfree(value);
		kfree(key);
		return -ENOMEM;
	}

Ohh and btw, in your suggestion (below), why don't you initialize 'retval' 
to -ENOMEM and then set it to zero once after all the allocations have 
completed? That would remove the need to set it to -ENOMEM 3 times before 
the goto's... Sure, you are seting it 3 times in error paths and my 
suggestion would introduce a retval=0; in the normal path, but the code 
would be shorter and more readable IMHO.

/Jesper Juhl


> >From 1a13a1d7a2bad26f050ecc342741b6c07cac2b8a Mon Sep 17 00:00:00 2001
> From: Peter Huewe <peterhuewe@xxxxxx>
> Date: Tue, 31 May 2011 00:54:27 +0200
> Subject: [PATCH] staging: altera-stapl: Fix memory leak of altera_init()
> 
> In case kzalloc() fails the second or third time
> we should free the previous allocated resources.
> In order to keep one return point and to keep the cleanup code to one
> place, some reordering was necessary.
> 
> Also while at it, removed the *sizeof(char) - to quote Linus:
> "" Also removed the silly "* sizeof(u8)".  If that isn't 1, we have way
>    deeper problems than a simple multiplication can fix. """
> 
> Reported-by: Andre Bartke <andre.bartke@xxxxxxxxx>
> Signed-off-by: Peter Huewe <peterhuewe@xxxxxx>
> ---
>  drivers/staging/altera-stapl/altera.c |   33 +++++++++++++++++++++------------
>  1 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/altera-stapl/altera.c b/drivers/staging/altera-stapl/altera.c
> index 05aad35..09392ce 100644
> --- a/drivers/staging/altera-stapl/altera.c
> +++ b/drivers/staging/altera-stapl/altera.c
> @@ -2430,16 +2430,23 @@ int altera_init(struct altera_config *config, const struct firmware *fw)
>  	int index = 0;
>  	s32 offset = 0L;
>  	s32 error_address = 0L;
> +	int retval = 0;
>  
> -	key = kzalloc(33 * sizeof(char), GFP_KERNEL);
> -	if (!key)
> -		return -ENOMEM;
> -	value = kzalloc(257 * sizeof(char), GFP_KERNEL);
> -	if (!value)
> -		return -ENOMEM;
> +	key = kzalloc(33, GFP_KERNEL);
> +	if (!key) {
> +		retval = -ENOMEM;
> +		goto out;
> +	}
> +	value = kzalloc(257, GFP_KERNEL);
> +	if (!value) {
> +		retval = -ENOMEM;
> +		goto free_key;
> +	}
>  	astate = kzalloc(sizeof(struct altera_state), GFP_KERNEL);
> -	if (!astate)
> -		return -ENOMEM;
> +	if (!astate) {
> +		retval = -ENOMEM;
> +		goto free_value;
> +	}
>  
>  	astate->config = config;
>  	if (!astate->config->jtag_io) {
> @@ -2518,10 +2525,12 @@ int altera_init(struct altera_config *config, const struct firmware *fw)
>  	} else if (exec_result)
>  		printk(KERN_ERR "%s: error %d\n", __func__, exec_result);
>  
> -	kfree(key);
> -	kfree(value);
>  	kfree(astate);
> -
> -	return 0;
> +free_value:
> +	kfree(value);
> +free_key:
> +	kfree(key);
> +out:
> +	return retval;
>  }
>  EXPORT_SYMBOL(altera_init);
> 

-- 
Jesper Juhl <jj@xxxxxxxxxxxxx>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.