[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

[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 <***@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;
}

[Brian Foster]

Post by Eric Sandeen
parent_list() has instances where a handle is leaked,
both by going out of scope, and on error paths.
---
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;
+ free(handlep);
free(parentbuf);
return retval;
}
--
1.7.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[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 <***@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;
}

[Brian Foster]

Post by Eric Sandeen
One error path in write_struct() wasn't freeing
the flist_t *fl which was allocated, so it leaks.
---
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
http://oss.sgi.com/mailman/listinfo/xfs

[Eric Sandeen]

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

Signed-off-by: Eric 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;
}

[Brian Foster]

Post by Eric Sandeen
We leaked the fshandlep in both error returns and
normal function exit.
---

Same thing in the 'if (endtime && endtime < time(0))' thing towards the

Post by Eric Sandeen
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)
tmp_close(mntdir);
close(fsfd);
+ free(fshandlep);
return 0;
}
--
1.7.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[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 <***@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;
}

[Brian Foster]

Post by Eric Sandeen
The allocated fshandle wasn't freed in either normal
exit or error paths.
Do this, and consolidate cleanup into an out: target.
---
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);
free(bstatbuf);
free(parentbuf);
+ free(fshandlep);
return err_status;
}
--
1.7.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[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 <***@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;

[Brian Foster]

Post by 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.
---
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

Post by Eric Sandeen
close(fd);
errno = ENOMEM;
return -1;
--
1.7.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[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 <***@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;
}

[Brian Foster]

Post by 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.
---
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. :-|

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