[PATCH 0/6] xfsprogs: fix resource leaks

[PATCH 0/6] xfsprogs: fix resource leaks

[Eric Sandeen]

This is the result of getting bored while on "vacation: ;)

It knocks out all coverity-reported resource leaks, eliminating
16 reported defects.

Thanks,
-Eric

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

[PATCH 1/6] xfs_io: free fshandlep in parent_check()

[Eric Sandeen]

The allocated fshandle wasn't freed in either normal
exit or error paths.

Do this, and consolidate cleanup into an out: target.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 io/parent.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/io/parent.c b/io/parent.c
index ca989e9..56e126d 100644
--- a/io/parent.c
+++ b/io/parent.c
@@ -258,9 +258,8 @@ parent_check(void)
 	if (!bstatbuf || !parentbuf) {
 		fprintf(stderr, _("unable to allocate buffers: %s\n"),
 			strerror(errno));
-		free(bstatbuf);
-		free(parentbuf);
-		return 1;
+		err_status = 1;
+		goto out;
 	}
 
 	if (do_bulkstat(parentbuf, &parentbuf_size, bstatbuf, fsfd, fshandlep) != 0)
@@ -272,8 +271,10 @@ parent_check(void)
 		printf(_("succeeded checking %llu inodes\n"),
 			(unsigned long long) inodes_checked);
 
+out:
 	free(bstatbuf);
 	free(parentbuf);
+	free(fshandlep);
 	return err_status;
 }
 
-- 
1.7.1

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

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

[Eric Sandeen]

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);
 
-	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

[PATCH 3/6] xfs_fsr: free handlep in fsrfs

[Eric Sandeen]

We leaked the fshandlep in both error returns and
normal function exit.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fsr/xfs_fsr.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 752d2db..580604c 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -702,6 +702,7 @@ fsrfs(char *mntdir, xfs_ino_t startino, int targetrange)
 	if ((fsfd = open(mntdir, O_RDONLY)) < 0) {
 		fsrprintf(_("unable to open: %s: %s\n"),
 		          mntdir, strerror( errno ));
+		free(fshandlep);
 		return -1;
 	}
 
@@ -709,6 +710,7 @@ fsrfs(char *mntdir, xfs_ino_t startino, int targetrange)
 		fsrprintf(_("Skipping %s: could not get XFS geometry\n"),
 			  mntdir);
 		close(fsfd);
+		free(fshandlep);
 		return -1;
 	}
 
@@ -774,6 +776,7 @@ fsrfs(char *mntdir, xfs_ino_t startino, int targetrange)
 out0:
 	tmp_close(mntdir);
 	close(fsfd);
+	free(fshandlep);
 	return 0;
 }
 
-- 
1.7.1

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

[PATCH 4/6] libhandle: Fix handle leak in path_to_fshandle error paths

[Eric Sandeen]

path_to_fshandle calls obj_to_handle, which potentially
allocates a handle, but the handle isn't freed on
a subsequent error path.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 libhandle/handle.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/libhandle/handle.c b/libhandle/handle.c
index 9f81483..3c1395a 100644
--- a/libhandle/handle.c
+++ b/libhandle/handle.c
@@ -97,6 +97,7 @@ path_to_fshandle(
 		/* new filesystem. add it to the cache */
 		fdhp = malloc(sizeof(struct fdhash));
 		if (fdhp == NULL) {
+			free(*fshanp);
 			close(fd);
 			errno = ENOMEM;
 			return -1;
-- 
1.7.1

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

[PATCH 5/6] xfs_io: fix leaks in parent_list()

[Eric Sandeen]

parent_list() has instances where a handle is leaked,
both by going out of scope, and on error paths.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 io/parent.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/io/parent.c b/io/parent.c
index 56e126d..0313119 100644
--- a/io/parent.c
+++ b/io/parent.c
@@ -293,7 +293,7 @@ print_parent_entry(parent_t *parent, int fullpath)
 static int
 parent_list(int fullpath)
 {
-	void *handlep;
+	void *handlep = NULL;
 	size_t handlen;
 	int error, i;
 	int retval = 1;
@@ -313,6 +313,7 @@ parent_list(int fullpath)
 				progname, path, strerror(errno));
 			goto error;
 		}
+		free_handle(fshandle, fshlen);
 	}
 
 	if (path_to_handle(path, &handlep, &handlen) != 0) {
@@ -325,7 +326,7 @@ parent_list(int fullpath)
 		if (!parentbuf) {
 			fprintf(stderr, _("%s: unable to allocate parent buffer: %s\n"),
 				progname, strerror(errno));
-			return 1;
+			goto error;
 		}
 
 		if (fullpath) {
@@ -365,6 +366,7 @@ parent_list(int fullpath)
 
 	retval = 0;
 error:
+	free(handlep);
 	free(parentbuf);
 	return retval;
 }
-- 
1.7.1

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

[PATCH 6/6] xfs_db: free flist on error in write_struct()

[Eric Sandeen]

One error path in write_struct() wasn't freeing
the flist_t *fl which was allocated, so it leaks.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 db/write.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/db/write.c b/db/write.c
index 0157a44..a0f14f4 100644
--- a/db/write.c
+++ b/db/write.c
@@ -652,6 +652,7 @@ write_struct(
 	buf = convert_arg(argv[1], bit_length);
 	if (!buf) {
 		dbprintf(_("unable to convert value '%s'.\n"), argv[1]);
+		flist_free(fl);
 		return;
 	}
 
-- 
1.7.1

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

Re: [PATCH 1/6] xfs_io: free fshandlep in parent_check()

[Brian Foster]

On Fri, Aug 01, 2014 at 09:59:14AM -0500, Eric Sandeen wrote:
> The allocated fshandle wasn't freed in either normal
> exit or error paths.
> 
> Do this, and consolidate cleanup into an out: target.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  io/parent.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/io/parent.c b/io/parent.c
> index ca989e9..56e126d 100644
> --- a/io/parent.c
> +++ b/io/parent.c
> @@ -258,9 +258,8 @@ parent_check(void)
>  	if (!bstatbuf || !parentbuf) {
>  		fprintf(stderr, _("unable to allocate buffers: %s\n"),
>  			strerror(errno));
> -		free(bstatbuf);
> -		free(parentbuf);
> -		return 1;
> +		err_status = 1;
> +		goto out;
>  	}
>  
>  	if (do_bulkstat(parentbuf, &parentbuf_size, bstatbuf, fsfd, fshandlep) != 0)
> @@ -272,8 +271,10 @@ parent_check(void)
>  		printf(_("succeeded checking %llu inodes\n"),
>  			(unsigned long long) inodes_checked);
>  
> +out:
>  	free(bstatbuf);
>  	free(parentbuf);
> +	free(fshandlep);
>  	return err_status;
>  }
>  
> -- 
> 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

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

[Brian Foster]

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

Re: [PATCH 3/6] xfs_fsr: free handlep in fsrfs

[Brian Foster]

On Fri, Aug 01, 2014 at 09:59:16AM -0500, Eric Sandeen wrote:
> We leaked the fshandlep in both error returns and
> normal function exit.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Same thing in the 'if (endtime && endtime < time(0))' thing towards the
end of the for loop, but that ends with an exit(), so:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fsr/xfs_fsr.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 752d2db..580604c 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -702,6 +702,7 @@ fsrfs(char *mntdir, xfs_ino_t startino, int targetrange)
>  	if ((fsfd = open(mntdir, O_RDONLY)) < 0) {
>  		fsrprintf(_("unable to open: %s: %s\n"),
>  		          mntdir, strerror( errno ));
> +		free(fshandlep);
>  		return -1;
>  	}
>  
> @@ -709,6 +710,7 @@ fsrfs(char *mntdir, xfs_ino_t startino, int targetrange)
>  		fsrprintf(_("Skipping %s: could not get XFS geometry\n"),
>  			  mntdir);
>  		close(fsfd);
> +		free(fshandlep);
>  		return -1;
>  	}
>  
> @@ -774,6 +776,7 @@ fsrfs(char *mntdir, xfs_ino_t startino, int targetrange)
>  out0:
>  	tmp_close(mntdir);
>  	close(fsfd);
> +	free(fshandlep);
>  	return 0;
>  }
>  
> -- 
> 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

Re: [PATCH 4/6] libhandle: Fix handle leak in path_to_fshandle error paths

[Brian Foster]

On Fri, Aug 01, 2014 at 09:59:17AM -0500, Eric Sandeen wrote:
> path_to_fshandle calls obj_to_handle, which potentially
> allocates a handle, but the handle isn't freed on
> a subsequent error path.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  libhandle/handle.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/libhandle/handle.c b/libhandle/handle.c
> index 9f81483..3c1395a 100644
> --- a/libhandle/handle.c
> +++ b/libhandle/handle.c
> @@ -97,6 +97,7 @@ path_to_fshandle(
>  		/* new filesystem. add it to the cache */
>  		fdhp = malloc(sizeof(struct fdhash));
>  		if (fdhp == NULL) {
> +			free(*fshanp);

Might be a good idea to set *fshanp back to NULL here. It doesn't appear
to be an issue with current callers, however:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  			close(fd);
>  			errno = ENOMEM;
>  			return -1;
> -- 
> 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

Re: [PATCH 5/6] xfs_io: fix leaks in parent_list()

[Brian Foster]

On Fri, Aug 01, 2014 at 09:59:18AM -0500, Eric Sandeen wrote:
> parent_list() has instances where a handle is leaked,
> both by going out of scope, and on error paths.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  io/parent.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/io/parent.c b/io/parent.c
> index 56e126d..0313119 100644
> --- a/io/parent.c
> +++ b/io/parent.c
> @@ -293,7 +293,7 @@ print_parent_entry(parent_t *parent, int fullpath)
>  static int
>  parent_list(int fullpath)
>  {
> -	void *handlep;
> +	void *handlep = NULL;
>  	size_t handlen;
>  	int error, i;
>  	int retval = 1;
> @@ -313,6 +313,7 @@ parent_list(int fullpath)
>  				progname, path, strerror(errno));
>  			goto error;
>  		}
> +		free_handle(fshandle, fshlen);
>  	}
>  
>  	if (path_to_handle(path, &handlep, &handlen) != 0) {
> @@ -325,7 +326,7 @@ parent_list(int fullpath)
>  		if (!parentbuf) {
>  			fprintf(stderr, _("%s: unable to allocate parent buffer: %s\n"),
>  				progname, strerror(errno));
> -			return 1;
> +			goto error;
>  		}
>  
>  		if (fullpath) {
> @@ -365,6 +366,7 @@ parent_list(int fullpath)
>  
>  	retval = 0;
>  error:
> +	free(handlep);
>  	free(parentbuf);
>  	return retval;
>  }
> -- 
> 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

Re: [PATCH 6/6] xfs_db: free flist on error in write_struct()

[Brian Foster]

On Fri, Aug 01, 2014 at 09:59:19AM -0500, Eric Sandeen wrote:
> One error path in write_struct() wasn't freeing
> the flist_t *fl which was allocated, so it leaks.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  db/write.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/db/write.c b/db/write.c
> index 0157a44..a0f14f4 100644
> --- a/db/write.c
> +++ b/db/write.c
> @@ -652,6 +652,7 @@ write_struct(
>  	buf = convert_arg(argv[1], bit_length);
>  	if (!buf) {
>  		dbprintf(_("unable to convert value '%s'.\n"), argv[1]);
> +		flist_free(fl);
>  		return;
>  	}
>  
> -- 
> 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