Re: [PATCH] Fix serious memory leak

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3272 bytes --]

On Fri, 16 Sep 2011 13:19:14 +0200 Lukasz Dorau <lukasz.dorau@intel.com>
wrote:

> During reshape function restore_stripes is called periodically
> and every time the buffer stripe_buf (of size raid_disks*chunk_size)
> is allocated but is not freed. It happens also upon successful completion.
> In case of huge arrays it can lead to the seizure of the entire
> system memory (even of the order of gigabytes).
> 
> Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>

Thanks.

I changed that patch a bit:
 - There were two places that freed everything and returned.  I changed the
   first one to just set 'rv' and 'goto' the last one.
 - You set 'rv = 0' twice.  Once is enough.  I removed the first one.

Thanks,

NeilBrown


> ---
>  restripe.c |   43 +++++++++++++++++++++++++++++++------------
>  1 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/restripe.c b/restripe.c
> index 9c83e2e..7cabbd0 100644
> --- a/restripe.c
> +++ b/restripe.c
> @@ -687,6 +687,7 @@ int restore_stripes(int *dest, unsigned long long *offsets,
>  	char **stripes = malloc(raid_disks * sizeof(char*));
>  	char **blocks = malloc(raid_disks * sizeof(char*));
>  	int i;
> +	int rv = 0;
>  
>  	int data_disks = raid_disks - (level == 0 ? 0 : level <= 5 ? 1 : 2);
>  
> @@ -717,20 +718,26 @@ int restore_stripes(int *dest, unsigned long long *offsets,
>  		unsigned long long offset;
>  		int disk, qdisk;
>  		int syndrome_disks;
> -		if (length < len)
> -			return -3;
> +		if (length < len) {
> +			rv = -3;
> +			goto abort;
> +		}
>  		for (i = 0; i < data_disks; i++) {
>  			int disk = geo_map(i, start/chunk_size/data_disks,
>  					   raid_disks, level, layout);
>  			if (src_buf == NULL) {
>  				/* read from file */
> -				if (lseek64(source,
> -					read_offset, 0) != (off64_t)read_offset)
> -					return -1;
> +				if (lseek64(source, read_offset, 0) !=
> +					 (off64_t)read_offset) {
> +					rv = -1;
> +					goto abort;
> +				}
>  				if (read(source,
>  					 stripes[disk],
> -					 chunk_size) != chunk_size)
> -					return -1;
> +					 chunk_size) != chunk_size) {
> +					rv = -1;
> +					goto abort;
> +				}
>  			} else {
>  				/* read from input buffer */
>  				memcpy(stripes[disk],
> @@ -782,15 +789,27 @@ int restore_stripes(int *dest, unsigned long long *offsets,
>  		}
>  		for (i=0; i < raid_disks ; i++)
>  			if (dest[i] >= 0) {
> -				if (lseek64(dest[i], offsets[i]+offset, 0) < 0)
> -					return -1;
> -				if (write(dest[i], stripes[i], chunk_size) != chunk_size)
> -					return -1;
> +				if (lseek64(dest[i],
> +					 offsets[i]+offset, 0) < 0) {
> +					rv = -1;
> +					goto abort;
> +				}
> +				if (write(dest[i], stripes[i],
> +					 chunk_size) != chunk_size) {
> +					rv = -1;
> +					goto abort;
> +				}
>  			}
>  		length -= len;
>  		start += len;
>  	}
> -	return 0;
> +	rv = 0;
> +
> +abort:
> +	free(stripe_buf);
> +	free(stripes);
> +	free(blocks);
> +	return rv;
>  }
>  
>  #ifdef MAIN
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 190 bytes --]