[PATCH] GFS2: Fix mem leak in gfs2_get_acl()

[PATCH] GFS2: Fix mem leak in gfs2_get_acl()

[Jesper Juhl]

If gfs2_xattr_acl_get() returns 0 - which, as far as I can tell, it
may do independently of having allocated memory for its third argument
('data' in this case) - then we may leak the memory allocated to data.

This patch initializes 'data' to NULL so that it will be safe to call
kfree() on it even if we do not allocate anything and also makes sure
that we kfree(data) in the 'len == 0' case.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 fs/gfs2/acl.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

 Note: I have tested that this change compiles. It has seen no other 
       testing than that.

diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index 230eb0f..d254d98 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -43,7 +43,7 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct posix_acl *acl;
 	const char *name;
-	char *data;
+	char *data = NULL;
 	int len;
 
 	if (!ip->i_eattr)
@@ -60,8 +60,10 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
 	len = gfs2_xattr_acl_get(ip, name, &data);
 	if (len < 0)
 		return ERR_PTR(len);
-	if (len == 0)
+	if (len == 0) {
+		kfree(data);
 		return NULL;
+	}
 
 	acl = posix_acl_from_xattr(data, len);
 	kfree(data);
-- 
1.7.10


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

Re: [PATCH] GFS2: Fix mem leak in gfs2_get_acl()

[Steven Whitehouse]

Hi,

On Sat, 2012-04-21 at 23:00 +0200, Jesper Juhl wrote:
> If gfs2_xattr_acl_get() returns 0 - which, as far as I can tell, it
> may do independently of having allocated memory for its third argument
> ('data' in this case) - then we may leak the memory allocated to data.
> 
I'm not so sure... in gfs2_xattr_acl_get() we have:

        error = gfs2_ea_find(ip, GFS2_EATYPE_SYS, name, &el);
        if (error)
                return error;
        if (!el.el_ea)
                goto out;
        if (!GFS2_EA_DATA_LEN(el.el_ea))   <---- zero length means return without allocating
                goto out;

        len = GFS2_EA_DATA_LEN(el.el_ea);
        data = kmalloc(len, GFP_NOFS);
etc.

So it looks to me as if we will never have allocated any
data unless the length is greater than zero, unless I've
missed something?

Steve.

> This patch initializes 'data' to NULL so that it will be safe to call
> kfree() on it even if we do not allocate anything and also makes sure
> that we kfree(data) in the 'len == 0' case.
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  fs/gfs2/acl.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>  Note: I have tested that this change compiles. It has seen no other 
>        testing than that.
> 
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index 230eb0f..d254d98 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -43,7 +43,7 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	struct gfs2_inode *ip = GFS2_I(inode);
>  	struct posix_acl *acl;
>  	const char *name;
> -	char *data;
> +	char *data = NULL;
>  	int len;
>  
>  	if (!ip->i_eattr)
> @@ -60,8 +60,10 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	len = gfs2_xattr_acl_get(ip, name, &data);
>  	if (len < 0)
>  		return ERR_PTR(len);
> -	if (len == 0)
> +	if (len == 0) {
> +		kfree(data);
>  		return NULL;
> +	}
>  
>  	acl = posix_acl_from_xattr(data, len);
>  	kfree(data);
> -- 
> 1.7.10
> 
> 

[Cluster-devel] [PATCH] GFS2: Fix mem leak in gfs2_get_acl()

[Steven Whitehouse]

Hi,

On Sat, 2012-04-21 at 23:00 +0200, Jesper Juhl wrote:
> If gfs2_xattr_acl_get() returns 0 - which, as far as I can tell, it
> may do independently of having allocated memory for its third argument
> ('data' in this case) - then we may leak the memory allocated to data.
> 
I'm not so sure... in gfs2_xattr_acl_get() we have:

        error = gfs2_ea_find(ip, GFS2_EATYPE_SYS, name, &el);
        if (error)
                return error;
        if (!el.el_ea)
                goto out;
        if (!GFS2_EA_DATA_LEN(el.el_ea))   <---- zero length means return without allocating
                goto out;

        len = GFS2_EA_DATA_LEN(el.el_ea);
        data = kmalloc(len, GFP_NOFS);
etc.

So it looks to me as if we will never have allocated any
data unless the length is greater than zero, unless I've
missed something?

Steve.

> This patch initializes 'data' to NULL so that it will be safe to call
> kfree() on it even if we do not allocate anything and also makes sure
> that we kfree(data) in the 'len == 0' case.
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  fs/gfs2/acl.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>  Note: I have tested that this change compiles. It has seen no other 
>        testing than that.
> 
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index 230eb0f..d254d98 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -43,7 +43,7 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	struct gfs2_inode *ip = GFS2_I(inode);
>  	struct posix_acl *acl;
>  	const char *name;
> -	char *data;
> +	char *data = NULL;
>  	int len;
>  
>  	if (!ip->i_eattr)
> @@ -60,8 +60,10 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	len = gfs2_xattr_acl_get(ip, name, &data);
>  	if (len < 0)
>  		return ERR_PTR(len);
> -	if (len == 0)
> +	if (len == 0) {
> +		kfree(data);
>  		return NULL;
> +	}
>  
>  	acl = posix_acl_from_xattr(data, len);
>  	kfree(data);
> -- 
> 1.7.10
> 
>