[PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[Mark Tinguely]

[-- Attachment #1: xfs-fix-leak-in-xfs_dir2_node_removename.patch --]
[-- Type: text/plain, Size: 1328 bytes --]

Free the memory pointed to by state before returning on error from 
xfs_dir2_node_removename.c

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
Found by Coverity (134681) in userspace, same patch applies there
also.

 fs/xfs/xfs_dir2_node.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: b/fs/xfs/xfs_dir2_node.c
===================================================================
--- a/fs/xfs/xfs_dir2_node.c
+++ b/fs/xfs/xfs_dir2_node.c
@@ -2131,10 +2131,9 @@ xfs_dir2_node_removename(
 	/*
 	 * Didn't find it, upper layer screwed up.
 	 */
-	if (rval != EEXIST) {
-		xfs_da_state_free(state);
-		return rval;
-	}
+	if (rval != EEXIST)
+		goto done;
+
 	blk = &state->path.blk[state->path.active - 1];
 	ASSERT(blk->magic == XFS_DIR2_LEAFN_MAGIC);
 	ASSERT(state->extravalid);
@@ -2145,7 +2144,7 @@ xfs_dir2_node_removename(
 	error = xfs_dir2_leafn_remove(args, blk->bp, blk->index,
 		&state->extrablk, &rval);
 	if (error)
-		return error;
+		goto done;
 	/*
 	 * Fix the hash values up the btree.
 	 */
@@ -2160,6 +2159,7 @@ xfs_dir2_node_removename(
 	 */
 	if (!error)
 		error = xfs_dir2_node_to_leaf(state);
+done:
 	xfs_da_state_free(state);
 	return error;
 }


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[Carlos Maiolino]

Looks good,

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

On Fri, Sep 27, 2013 at 08:01:31AM -0500, Mark Tinguely wrote:
> Free the memory pointed to by state before returning on error from 
> xfs_dir2_node_removename.c
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
> Found by Coverity (134681) in userspace, same patch applies there
> also.
> 
>  fs/xfs/xfs_dir2_node.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Index: b/fs/xfs/xfs_dir2_node.c
> ===================================================================
> --- a/fs/xfs/xfs_dir2_node.c
> +++ b/fs/xfs/xfs_dir2_node.c
> @@ -2131,10 +2131,9 @@ xfs_dir2_node_removename(
>  	/*
>  	 * Didn't find it, upper layer screwed up.
>  	 */
> -	if (rval != EEXIST) {
> -		xfs_da_state_free(state);
> -		return rval;
> -	}
> +	if (rval != EEXIST)
> +		goto done;
> +
>  	blk = &state->path.blk[state->path.active - 1];
>  	ASSERT(blk->magic == XFS_DIR2_LEAFN_MAGIC);
>  	ASSERT(state->extravalid);
> @@ -2145,7 +2144,7 @@ xfs_dir2_node_removename(
>  	error = xfs_dir2_leafn_remove(args, blk->bp, blk->index,
>  		&state->extrablk, &rval);
>  	if (error)
> -		return error;
> +		goto done;
>  	/*
>  	 * Fix the hash values up the btree.
>  	 */
> @@ -2160,6 +2159,7 @@ xfs_dir2_node_removename(
>  	 */
>  	if (!error)
>  		error = xfs_dir2_node_to_leaf(state);
> +done:
>  	xfs_da_state_free(state);
>  	return error;
>  }
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[Eric Sandeen]

On 9/27/13 8:01 AM, Mark Tinguely wrote:
> Free the memory pointed to by state before returning on error from 
> xfs_dir2_node_removename.c
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
> Found by Coverity (134681) in userspace, same patch applies there
> also.

Heh, looks like that one has been around since the dawn of time, thanks.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

how do we handle the matching userspace fixes, separate patch to
be explicit?  Wait for the next syncup?

Thanks,
-Eric

>  fs/xfs/xfs_dir2_node.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Index: b/fs/xfs/xfs_dir2_node.c
> ===================================================================
> --- a/fs/xfs/xfs_dir2_node.c
> +++ b/fs/xfs/xfs_dir2_node.c
> @@ -2131,10 +2131,9 @@ xfs_dir2_node_removename(
>  	/*
>  	 * Didn't find it, upper layer screwed up.
>  	 */
> -	if (rval != EEXIST) {
> -		xfs_da_state_free(state);
> -		return rval;
> -	}
> +	if (rval != EEXIST)
> +		goto done;
> +
>  	blk = &state->path.blk[state->path.active - 1];
>  	ASSERT(blk->magic == XFS_DIR2_LEAFN_MAGIC);
>  	ASSERT(state->extravalid);
> @@ -2145,7 +2144,7 @@ xfs_dir2_node_removename(
>  	error = xfs_dir2_leafn_remove(args, blk->bp, blk->index,
>  		&state->extrablk, &rval);
>  	if (error)
> -		return error;
> +		goto done;
>  	/*
>  	 * Fix the hash values up the btree.
>  	 */
> @@ -2160,6 +2159,7 @@ xfs_dir2_node_removename(
>  	 */
>  	if (!error)
>  		error = xfs_dir2_node_to_leaf(state);
> +done:
>  	xfs_da_state_free(state);
>  	return error;
>  }
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[Mark Tinguely]

On 09/27/13 11:44, Eric Sandeen wrote:
> On 9/27/13 8:01 AM, Mark Tinguely wrote:
>> Free the memory pointed to by state before returning on error from
>> xfs_dir2_node_removename.c
>>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>> ---
>> Found by Coverity (134681) in userspace, same patch applies there
>> also.
>
> Heh, looks like that one has been around since the dawn of time, thanks.
>
> Reviewed-by: Eric Sandeen<sandeen@redhat.com>
>
> how do we handle the matching userspace fixes, separate patch to
> be explicit?  Wait for the next syncup?
>
> Thanks,
> -Eric

<patch delete>

Good question.

The user space should be kept up to date with the kernel.

Since the patches will be identical except the directory name, I was 
hoping to submit one copy. But I am not trying to invent a policy, just 
being lazy.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[Eric Sandeen]

On 9/27/13 12:48 PM, Mark Tinguely wrote:
> On 09/27/13 11:44, Eric Sandeen wrote:
>> On 9/27/13 8:01 AM, Mark Tinguely wrote:
>>> Free the memory pointed to by state before returning on error from
>>> xfs_dir2_node_removename.c
>>>
>>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>>> ---
>>> Found by Coverity (134681) in userspace, same patch applies there
>>> also.
>>
>> Heh, looks like that one has been around since the dawn of time, thanks.
>>
>> Reviewed-by: Eric Sandeen<sandeen@redhat.com>
>>
>> how do we handle the matching userspace fixes, separate patch to
>> be explicit?  Wait for the next syncup?
>>
>> Thanks,
>> -Eric
> 
> <patch delete>
> 
> Good question.
> 
> The user space should be kept up to date with the kernel.
> 
> Since the patches will be identical except the directory name, I was hoping to submit one copy. But I am not trying to invent a policy, just being lazy.
> 
> --Mark.
> 

Was just an offhanded question; it'd just be good to know what we all expect.

I suppose that it could depend on the severity of the flaw; a minor leak before exit()
isn't a big deal and could wait for a global sync-up; a data corruption fix might
need to be quickly merged to both trees.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[Roger Willcocks]

On Fri, 2013-09-27 at 08:01 -0500, Mark Tinguely wrote:
> plain text document attachment
> (xfs-fix-leak-in-xfs_dir2_node_removename.patch)
> Free the memory pointed to by state before returning on error from 
> xfs_dir2_node_removename.c
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
> Found by Coverity (134681) in userspace, same patch applies there
> also.
> 

Is the first hunk right ?

xfs_da_node_lookup_int called as

        error = xfs_da_node_lookup_int(state, &rval);

and returns with

        *result = retval;
        return(0);

so, on return, error == 0 and rval == an error code. The next lines:

        if (error)
                rval = error;

won't change that. But previously if rval != EEXIST you returned rval.
With the change below, you return error, which is zero.

--
Roger


>  fs/xfs/xfs_dir2_node.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Index: b/fs/xfs/xfs_dir2_node.c
> ===================================================================
> --- a/fs/xfs/xfs_dir2_node.c
> +++ b/fs/xfs/xfs_dir2_node.c
> @@ -2131,10 +2131,9 @@ xfs_dir2_node_removename(
>  	/*
>  	 * Didn't find it, upper layer screwed up.
>  	 */
> -	if (rval != EEXIST) {
> -		xfs_da_state_free(state);
> -		return rval;
> -	}
> +	if (rval != EEXIST)
> +		goto done;
> +
>  	blk = &state->path.blk[state->path.active - 1];
>  	ASSERT(blk->magic == XFS_DIR2_LEAFN_MAGIC);
>  	ASSERT(state->extravalid);
> @@ -2145,7 +2144,7 @@ xfs_dir2_node_removename(
>  	error = xfs_dir2_leafn_remove(args, blk->bp, blk->index,
>  		&state->extrablk, &rval);
>  	if (error)
> -		return error;
> +		goto done;
>  	/*
>  	 * Fix the hash values up the btree.
>  	 */
> @@ -2160,6 +2159,7 @@ xfs_dir2_node_removename(
>  	 */
>  	if (!error)
>  		error = xfs_dir2_node_to_leaf(state);
> +done:
>  	xfs_da_state_free(state);
>  	return error;
>  }
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 
-- 
Roger Willcocks <roger@filmlight.ltd.uk>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[Mark Tinguely]

On 09/27/13 14:36, Roger Willcocks wrote:
>
> On Fri, 2013-09-27 at 08:01 -0500, Mark Tinguely wrote:
>> plain text document attachment
>> (xfs-fix-leak-in-xfs_dir2_node_removename.patch)
>> Free the memory pointed to by state before returning on error from
>> xfs_dir2_node_removename.c
>>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>> ---
>> Found by Coverity (134681) in userspace, same patch applies there
>> also.
>>
>
> Is the first hunk right ?
>
> xfs_da_node_lookup_int called as
>
>          error = xfs_da_node_lookup_int(state,&rval);
>
> and returns with
>
>          *result = retval;
>          return(0);
>
> so, on return, error == 0 and rval == an error code. The next lines:
>
>          if (error)
>                  rval = error;
>
> won't change that. But previously if rval != EEXIST you returned rval.
> With the change below, you return error, which is zero.
>
> --
> Roger


Thanks, guilty as charged. need to add the error = EEXIST.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix memory leak in xfs_dir2_node_removename

[Eric Sandeen]

On 9/27/13 2:36 PM, Roger Willcocks wrote:
> 
> On Fri, 2013-09-27 at 08:01 -0500, Mark Tinguely wrote:
>> plain text document attachment
>> (xfs-fix-leak-in-xfs_dir2_node_removename.patch)
>> Free the memory pointed to by state before returning on error from 
>> xfs_dir2_node_removename.c
>>
>> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
>> ---
>> Found by Coverity (134681) in userspace, same patch applies there
>> also.
>>
> 
> Is the first hunk right ?
> 
> xfs_da_node_lookup_int called as
> 
>         error = xfs_da_node_lookup_int(state, &rval);
> 
> and returns with
> 
>         *result = retval;
>         return(0);
> 
> so, on return, error == 0 and rval == an error code. The next lines:
> 
>         if (error)
>                 rval = error;
> 
> won't change that. But previously if rval != EEXIST you returned rval.
> With the change below, you return error, which is zero.

Oh geez.  o_O . . . Thanks Roger.

3rd review's the charm? ;)

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs