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

[Brian Foster <bfoster@redhat.com>]

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@redhat.com>
> ---
>  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@redhat.com>

> -	close(fd);
> +out:
> +	if (fsfd >= 0)
> +		close(fsfd);
> +	if (fd >= 0)
> +		close(fd);
> +	free(fshandlep);
>  
>  	return error;
>  }
> -- 
> 1.7.1
> 
> _______________________________________________
> 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