xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Wed, 02 Oct 2013 16:04:37 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131002203241.GV12541@dastard>
References: <20131002125110.745269864@xxxxxxx> <20131002125409.826742020@xxxxxxx> <20131002203241.GV12541@dastard>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 10/02/13 15:32, Dave Chinner wrote:
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@xxxxxxx>
---
  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.

nod.

thank

--Mark.

<Prev in Thread] Current Thread [Next in Thread>