Re: [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename

[Dave Chinner <david@fromorbit.com>]

On Wed, Oct 02, 2013 at 07:51:11AM -0500, Mark Tinguely wrote:
> Fix the leak of kernel memory in xfs_dir2_node_removename()
> when xfs_dir2_leafn_remove() returns an error code.
> 
> Found by Coverity in userspace same patch applies there also.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  v2 corrected bad return code as pointed out by Roger Willcocks.
> 
>  fs/xfs/xfs_dir2_node.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> Index: b/fs/xfs/xfs_dir2_node.c
> ===================================================================
> --- a/fs/xfs/xfs_dir2_node.c
> +++ b/fs/xfs/xfs_dir2_node.c
> @@ -2105,12 +2105,12 @@ xfs_dir2_node_lookup(
>   */
>  int						/* error */
>  xfs_dir2_node_removename(
> -	xfs_da_args_t		*args)		/* operation arguments */
> +	struct xfs_da_args	*args)		/* operation arguments */
>  {
> -	xfs_da_state_blk_t	*blk;		/* leaf block */
> +	struct xfs_da_state_blk	*blk;		/* leaf block */
>  	int			error;		/* error return value */
>  	int			rval;		/* operation return value */
> -	xfs_da_state_t		*state;		/* btree cursor */
> +	struct xfs_da_state	*state;		/* btree cursor */
>  
>  	trace_xfs_dir2_node_removename(args);
>  
> @@ -2132,9 +2132,10 @@ xfs_dir2_node_removename(
>  	 * Didn't find it, upper layer screwed up.
>  	 */
>  	if (rval != EEXIST) {
> -		xfs_da_state_free(state);
> -		return rval;
> +		error = rval;
> +		goto done;

Can you make this something like "out_free"? We tend to name jump
labels according to the action that needs to be taken, like
"out_unlock", "error_trans_cancel", etc...

Also, this code is now "funky" in how it handles rval. it will do
this:

        /*
         * Look up the entry we're deleting, set up the cursor.
         */
        error = xfs_da3_node_lookup_int(state, &rval);
        if (error)
                rval = error;
        /*
         * Didn't find it, upper layer screwed up.
         */
        if (rval != EEXIST) {
		error = rval;
                goto out_free;
        }

That's kind funky with the reassignment of rval if there's an error.
Better would be:

        /* Look up the entry we're deleting, set up the cursor. */
        error = xfs_da3_node_lookup_int(state, &rval);
        if (error)
                goto out_free;

        /* Didn't find it, upper layer screwed up. */
	if (rval != EEXIST) {
		error = rval;
		goto out_free;
	}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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