xfs
[Top] [All Lists]

Re: [PATCH 2/6] xfs_fsr: fix leaks & catch error in fsrfile()

To: Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH 2/6] xfs_fsr: fix leaks & catch error in fsrfile()
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Tue, 5 Aug 2014 09:16:18 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1406905159-12415-3-git-send-email-sandeen@xxxxxxxxxx>
References: <1406905159-12415-1-git-send-email-sandeen@xxxxxxxxxx> <1406905159-12415-3-git-send-email-sandeen@xxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Aug 01, 2014 at 09:59:15AM -0500, Eric Sandeen wrote:
> The allocated fshandlep leaks on most error paths;
> restructure with an out: target that does all necessary
> freeing, and initialize filehandles to -1 so that we
> know whether they need to be closed on the error path.
> 
> While we're at it, if gettmpname() fails, we still
> return 0 for an error, because error is initialized
> to 0 and only set otherwise by fsrfile_common.
> So if gettmpname() fails, we return success from the
> function even though we did no work.  Fix that
> as well by initializing error to -1.
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>  fsr/xfs_fsr.c |   28 ++++++++++++++--------------
>  1 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 48629fd..752d2db 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -809,15 +809,15 @@ fsrfile(char *fname, xfs_ino_t ino)
>  {
>       xfs_bstat_t     statbuf;
>       jdm_fshandle_t  *fshandlep;
> -     int     fd, fsfd;
> -     int     error = 0;
> +     int     fd = -1, fsfd = -1;
> +     int     error = -1;
>       char    *tname;
>  
>       fshandlep = jdm_getfshandle(getparent (fname) );
> -     if (! fshandlep) {
> +     if (!fshandlep) {
>               fsrprintf(_("unable to construct sys handle for %s: %s\n"),
>                       fname, strerror(errno));
> -             return -1;
> +             goto out;
>       }
>  
>       /*
> @@ -828,39 +828,39 @@ fsrfile(char *fname, xfs_ino_t ino)
>       if (fsfd < 0) {
>               fsrprintf(_("unable to open sys handle for %s: %s\n"),
>                       fname, strerror(errno));
> -             return -1;
> +             goto out;
>       }
>  
>       if ((xfs_bulkstat_single(fsfd, &ino, &statbuf)) < 0) {
>               fsrprintf(_("unable to get bstat on %s: %s\n"),
>                       fname, strerror(errno));
> -             close(fsfd);
> -             return -1;
> +             goto out;
>       }
>  
>       fd = jdm_open(fshandlep, &statbuf, O_RDWR|O_DIRECT);
>       if (fd < 0) {
>               fsrprintf(_("unable to open handle %s: %s\n"),
>                       fname, strerror(errno));
> -             close(fsfd);
> -             return -1;
> +             goto out;
>       }
>  
>       /* Get the fs geometry */
>       if (xfs_getgeom(fsfd, &fsgeom) < 0 ) {
>               fsrprintf(_("Unable to get geom on fs for: %s\n"), fname);
> -             close(fsfd);
> -             return -1;
> +             goto out;
>       }
>  
> -     close(fsfd);
> -
>       tname = gettmpname(fname);
>  
>       if (tname)
>               error = fsrfile_common(fname, tname, NULL, fd, &statbuf);
>  

I was wondering whether this bit to not fail if the path is bad was
intentional (e.g., to avoid breaking through a higher-level loop or
something), but we don't check the return value of this function
anyways. :-|

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

> -     close(fd);
> +out:
> +     if (fsfd >= 0)
> +             close(fsfd);
> +     if (fd >= 0)
> +             close(fd);
> +     free(fshandlep);
>  
>       return error;
>  }
> -- 
> 1.7.1
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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