[PATCH v2] posix_acl: fix memleak when set posix acl.

[PATCH v2] posix_acl: fix memleak when set posix acl.

[Zhang Xiaoxu]

When set posix acl, it maybe call posix_acl_update_mode in some
filesystem, eg. ext4. It may set acl to NULL, so, we can't free
the acl which allocated in posix_acl_xattr_set.

Use an temp value to store the acl address for posix_acl_release.

Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
 fs/posix_acl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 84ad1c90d535..0a359d06274c 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -882,7 +882,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
 		    const char *name, const void *value,
 		    size_t size, int flags)
 {
-	struct posix_acl *acl = NULL;
+	struct posix_acl *acl = NULL, *p = NULL;
 	int ret;
 
 	if (value) {
@@ -890,8 +890,15 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
 		if (IS_ERR(acl))
 			return PTR_ERR(acl);
 	}
+
+	/*
+	 * when call set_posix_acl, posix_acl_update_mode may set acl
+	 * to NULL,use temporary variables p for posix_acl_release.
+	 */
+	p = acl;
 	ret = set_posix_acl(inode, handler->flags, acl);
-	posix_acl_release(acl);
+
+	posix_acl_release(p);
 	return ret;
 }
 
-- 
2.17.2

Re: [PATCH v2] posix_acl: fix memleak when set posix acl.

[Gao Xiang]

Hi Xiaoxu,

On Tue, Nov 26, 2019 at 09:38:09PM +0800, Zhang Xiaoxu wrote:
> When set posix acl, it maybe call posix_acl_update_mode in some
> filesystem, eg. ext4. It may set acl to NULL, so, we can't free
> the acl which allocated in posix_acl_xattr_set.
> 
> Use an temp value to store the acl address for posix_acl_release.
> 
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> ---
>  fs/posix_acl.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 84ad1c90d535..0a359d06274c 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -882,7 +882,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
>  		    const char *name, const void *value,
>  		    size_t size, int flags)
>  {
> -	struct posix_acl *acl = NULL;
> +	struct posix_acl *acl = NULL, *p = NULL;
>  	int ret;
>  
>  	if (value) {
> @@ -890,8 +890,15 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
>  		if (IS_ERR(acl))
>  			return PTR_ERR(acl);
>  	}
> +
> +	/*
> +	 * when call set_posix_acl, posix_acl_update_mode may set acl
> +	 * to NULL,use temporary variables p for posix_acl_release.
> +	 */
> +	p = acl;
>  	ret = set_posix_acl(inode, handler->flags, acl);

IMO, variable acl in this function won't be affected, yes?
Am I missing something?

Thanks,
Gao Xiang

> -	posix_acl_release(acl);
> +
> +	posix_acl_release(p);
>  	return ret;
>  }
>  
> -- 
> 2.17.2
> 

Re: [PATCH v2] posix_acl: fix memleak when set posix acl.

[zhangxiaoxu (A)]

please ignore this patch.
the problem is exist in centos kernel.


在 2019/11/26 22:01, Gao Xiang 写道:
> IMO, variable acl in this function won't be affected, yes?
> Am I missing something?

Re: [PATCH v2] posix_acl: fix memleak when set posix acl.

[Al Viro]

On Tue, Nov 26, 2019 at 09:38:09PM +0800, Zhang Xiaoxu wrote:
> When set posix acl, it maybe call posix_acl_update_mode in some
> filesystem, eg. ext4. It may set acl to NULL, so, we can't free
> the acl which allocated in posix_acl_xattr_set.
> 
> Use an temp value to store the acl address for posix_acl_release.

Huh?

>  {
> -	struct posix_acl *acl = NULL;
> +	struct posix_acl *acl = NULL, *p = NULL;
>  	int ret;
>  
>  	if (value) {
> @@ -890,8 +890,15 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
>  		if (IS_ERR(acl))
>  			return PTR_ERR(acl);
>  	}
> +
> +	/*
> +	 * when call set_posix_acl, posix_acl_update_mode may set acl
> +	 * to NULL,use temporary variables p for posix_acl_release.
> +	 */
> +	p = acl;
>  	ret = set_posix_acl(inode, handler->flags, acl);
> -	posix_acl_release(acl);
> +
> +	posix_acl_release(p);

	How could set_posix_acl() possibly set a local variable of
posix_acl_xattr_set() to NULL or to anything else, for that matter?
That makes no sense.  C passes arguments by value; formal parameters
behave as local variables in the called function, initialized by
the values passed by caller.  Modifying those inside the called
function is perfectly valid, same as for any local variable.  And
it does _not_ modify anything in the caller's scope.

	Do yourself a favour, grab a textbook on C (or the actual
standard, if you are up for that - e.g. at
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf) and
read it through.  That'll save you a lot of frustration trying to
guess what some construct is doing.