Skip to contentSkip to conversation

xfsprogs: varius & sundry fixes for coverity defects

PATCH In review Eric Sandeen 4 participants 25 messages Apr 8, 2014 – Apr 11, 2014

lore
Share & tools
lore
#1 lore raw

PATCH 1/14 xfsprogs: fix various fd leaks

Coverity spotted these; several paths where we don't close fds when we return.

---
 fsr/xfs_fsr.c      |   10 +++++++++-
 io/readdir.c       |    1 +
 libhandle/handle.c |    1 +
 mkfs/proto.c       |   12 +++++++++---
 quota/quot.c       |    1 +
 rtcp/xfs_rtcp.c    |    2 ++
 6 files changed, 23 insertions(+), 4 deletions(-)
fsr/xfs_fsr.c +9 -1
index 6f00b41..6b5d260 100644
@@ -706,6 +706,7 @@ fsrfs(char *mntdir, xfs_ino_t startino, int targetrange)
 	if (xfs_getgeom(fsfd, &fsgeom) < 0 ) {
 		fsrprintf(_("Skipping %s: could not get XFS geometry\n"),
 			  mntdir);
+		close(fsfd);
 		return -1;
 	}
 
@@ -1346,6 +1347,8 @@ packfile(char *fname, char *tname, int fd,
 	if (lseek64(tfd, 0, SEEK_SET)) {
 		fsrprintf(_("Couldn't rewind on temporary file\n"));
 		close(tfd);
+		if (ffd != -1)
+			close(ffd);
 		free(fbuf);
 		return -1;
 	}
@@ -1359,6 +1362,8 @@ packfile(char *fname, char *tname, int fd,
 			fsrprintf(_("No improvement will be made (skipping): %s\n"), fname);
 		free(fbuf);
 		close(tfd);
+		if (ffd != -1)
+			close(ffd);
 		return 1; /* no change/no error */
 	}
 
@@ -1432,6 +1437,8 @@ packfile(char *fname, char *tname, int fd,
 				}
 				free(fbuf);
 				close(tfd);
+				if (ffd != -1)
+					close(ffd);
 				return -1;
 			}
 			if (nfrags) {
@@ -1446,7 +1453,8 @@ packfile(char *fname, char *tname, int fd,
 		}
 	}
 	ftruncate64(tfd, statp->bs_size);
-	if (ffd > 0) close(ffd);
+	if (ffd != -1)
+		close(ffd);
 	fsync(tfd);
 
 	free(fbuf);
io/readdir.c +1 -0
index 822818a..7133204 100644
@@ -171,6 +171,7 @@ readdir_f(
 	gettimeofday(&t2, NULL);
 
 	closedir(dir);
+	close(dfd);
 
 	t2 = tsub(t2, t1);
 	timestr(&t2, ts, sizeof(ts), 0);
libhandle/handle.c +1 -0
index 9a232fa..9f81483 100644
@@ -97,6 +97,7 @@ path_to_fshandle(
 		/* new filesystem. add it to the cache */
 		fdhp = malloc(sizeof(struct fdhash));
 		if (fdhp == NULL) {
+			close(fd);
 			errno = ENOMEM;
 			return -1;
 		}
mkfs/proto.c +9 -3
index 4d3680d..b7e0761 100644
@@ -59,18 +59,19 @@ setup_proto(
 	if ((fd = open(fname, O_RDONLY)) < 0 || (size = filesize(fd)) < 0) {
 		fprintf(stderr, _("%s: failed to open %s: %s\n"),
 			progname, fname, strerror(errno));
-		exit(1);
+		goto out_fail;
 	}
+
 	buf = malloc(size + 1);
 	if (read(fd, buf, size) < size) {
 		fprintf(stderr, _("%s: read failed on %s: %s\n"),
 			progname, fname, strerror(errno));
-		exit(1);
+		goto out_fail;
 	}
 	if (buf[size - 1] != '\n') {
 		fprintf(stderr, _("%s: proto file %s premature EOF\n"),
 			progname, fname);
-		exit(1);
+		goto out_fail;
 	}
 	buf[size] = '\0';
 	/*
@@ -79,7 +80,12 @@ setup_proto(
 	(void)getstr(&buf);	/* boot image name */
 	(void)getnum(&buf);	/* block count */
 	(void)getnum(&buf);	/* inode count */
+	close(fd);
 	return buf;
+
+out_fail:
+	close(fd);
+	exit(1);
 }
 
 static long
quota/quot.c +1 -0
index a7782b4..96d449e 100644
@@ -165,6 +165,7 @@ quot_bulkstat_mount(
 	buf = (xfs_bstat_t *)calloc(NBSTAT, sizeof(xfs_bstat_t));
 	if (!buf) {
 		perror("calloc");
+		close(fsfd);
 		return;
 	}
 
rtcp/xfs_rtcp.c +2 -0
index f0b5936..b81b1e1 100644
@@ -224,6 +224,7 @@ rtcp( char *source, char *target, int fextsize)
 		if ( !(fsxattr.fsx_xflags & XFS_XFLAG_REALTIME) ) {
 			fprintf(stderr, _("%s: %s is not a realtime file.\n"),
 				progname, tbuf);
+			close( tofd );
 			return( -1 );
 		}
 
@@ -234,6 +235,7 @@ rtcp( char *source, char *target, int fextsize)
 			fprintf(stderr, _("%s: %s file extent size is %d, "
 					"instead of %d.\n"),
 				progname, tbuf, fsxattr.fsx_extsize, fextsize);
+			close( tofd );
 			return( -1 );
 		}
 	}
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#2 lore raw

PATCH 2/14 xfsprogs: refactor fsrall_cleanup in xfs_fsr

In reply to Eric Sandeen

fsrall_cleanup leaked an fd in the non-timeout case - but the logic was weird and tortured, refactor it to make more sense and fix the fd leak as well.

---
 fsr/xfs_fsr.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)
fsr/xfs_fsr.c +12 -12
index 6b5d260..3818f02 100644
@@ -647,14 +647,19 @@ fsrall_cleanup(int timeout)
 	int ret;
 	char buf[SMBUFSZ];
 
-	/* record where we left off */
 	unlink(leftofffile);
-	fd = open(leftofffile, O_WRONLY|O_CREAT|O_EXCL, 0644);
-	if (fd == -1)
-		fsrprintf(_("open(%s) failed: %s\n"),
-		          leftofffile, strerror(errno));
-	else {
-		if (timeout) {
+
+	if (timeout) {
+		fsrprintf(_("%s startpass %d, endpass %d, time %d seconds\n"),
+			progname, startpass, fs->npass,
+			time(0) - endtime + howlong);
+
+		/* record where we left off */
+		fd = open(leftofffile, O_WRONLY|O_CREAT|O_EXCL, 0644);
+		if (fd == -1) {
+			fsrprintf(_("open(%s) failed: %s\n"),
+			          leftofffile, strerror(errno));
+		} else {
 			ret = sprintf(buf, "%s %d %llu\n", fs->dev,
 			        fs->npass, (unsigned long long)leftoffino);
 			if (write(fd, buf, ret) < strlen(buf))
@@ -663,11 +668,6 @@ fsrall_cleanup(int timeout)
 			close(fd);
 		}
 	}
-
-	if (timeout)
-		fsrprintf(_("%s startpass %d, endpass %d, time %d seconds\n"),
-			progname, startpass, fs->npass,
-			time(0) - endtime + howlong);
 }
 
 /*
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#3 lore raw

PATCH 3/14 xfsprogs: trivial buffer frees on error paths

In reply to Eric Sandeen

Lots of memory leaks on error paths etc, spotted by coverity. This patch rolls up the super-straightforward fixes across xfsprogs.

---
 db/addr.c    |   15 +++++++--------
 db/check.c   |    4 +++-
 db/write.c   |    2 ++
 io/parent.c  |    2 ++
 mkfs/proto.c |    3 ++-
 5 files changed, 16 insertions(+), 10 deletions(-)
db/addr.c +7 -8
index f74dd62..f9f760d 100644
@@ -85,16 +85,14 @@ addr_f(
 	fl = flist_scan(argv[1]);
 	if (fl == NULL)
 		return 0;
-	if (!flist_parse(fld, fl, iocur_top->data, 0)) {
-		flist_free(fl);
-		return 0;
-	}
+	if (!flist_parse(fld, fl, iocur_top->data, 0))
+		goto out;
+	
 	flist_print(fl);
 	for (tfl = fl; tfl->child != NULL; tfl = tfl->child) {
 		if ((tfl->flags & FL_OKLOW) && tfl->low < tfl->high) {
 			dbprintf(_("array not allowed for addr command\n"));
-			flist_free(fl);
-			return 0;
+			goto out;
 		}
 	}
 	fld = tfl->fld;
@@ -103,7 +101,7 @@ addr_f(
 		next = inode_next_type();
 	if (next == TYP_NONE) {
 		dbprintf(_("no next type for field %s\n"), fld->name);
-		return 0;
+		goto out;
 	}
 	fa = &ftattrtab[fld->ftyp];
 	ASSERT(fa->ftyp == fld->ftyp);
@@ -111,9 +109,10 @@ addr_f(
 	if (adf == NULL) {
 		dbprintf(_("no addr function for field %s (type %s)\n"),
 			fld->name, fa->name);
-		return 0;
+		goto out;
 	}
 	(*adf)(iocur_top->data, tfl->offset, next);
+out:
 	flist_free(fl);
 	return 0;
 }
db/check.c +3 -1
index 4867698..baf7f9f 100644
@@ -1136,7 +1136,7 @@ blocktrash_f(
 	}
 	if (blocks == 0) {
 		dbprintf(_("blocktrash: no matching blocks\n"));
-		return 0;
+		goto out;
 	}
 	if (!sopt)
 		dbprintf(_("blocktrash: seed %u\n"), seed);
@@ -1161,6 +1161,7 @@ blocktrash_f(
 			}
 		}
 	}
+out:
 	xfree(lentab);
 	return 0;
 }
@@ -1907,6 +1908,7 @@ ncheck_f(
 			break;
 		default:
 			dbprintf(_("bad option -%c for ncheck command\n"), c);
+			xfree(ilist);
 			return 0;
 		}
 	}
db/write.c +2 -0
index 7b34fc0..ca8bd0f 100644
@@ -233,6 +233,7 @@ bwrite_lrot(
 	memcpy(hold_region, base, shift);
 	memcpy(base, base+shift, len-shift);
 	memcpy(base+(len-shift), hold_region, shift);
+	free(hold_region);
 }
 
 /* ARGSUSED */
@@ -265,6 +266,7 @@ bwrite_rrot(
 	memcpy(hold_region, base+(len-shift), shift);
 	memmove(base+shift, base, len-shift);
 	memcpy(base, hold_region, shift);
+	free(hold_region);
 }
 
 /* ARGSUSED */
io/parent.c +2 -0
index 47faaa0..ca989e9 100644
@@ -258,6 +258,8 @@ parent_check(void)
 	if (!bstatbuf || !parentbuf) {
 		fprintf(stderr, _("unable to allocate buffers: %s\n"),
 			strerror(errno));
+		free(bstatbuf);
+		free(parentbuf);
 		return 1;
 	}
 
mkfs/proto.c +2 -1
index b7e0761..95583c9 100644
@@ -49,7 +49,7 @@ char *
 setup_proto(
 	char	*fname)
 {
-	char		*buf;
+	char		*buf = NULL;
 	static char	dflt[] = "d--755 0 0 $";
 	int		fd;
 	long		size;
@@ -85,6 +85,7 @@ setup_proto(
 
 out_fail:
 	close(fd);
+	free(buf);
 	exit(1);
 }
 
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#4 lore raw

Re: [PATCH 03/14] xfsprogs: trivial buffer frees on error paths

Quoted 18 lines · Eric Sandeen

Lots of memory leaks on error paths etc, spotted by coverity. This patch rolls up the super-straightforward fixes across xfsprogs.

---
 db/addr.c    |   15 +++++++--------
 db/check.c   |    4 +++-
 db/write.c   |    2 ++
 io/parent.c  |    2 ++
 mkfs/proto.c |    3 ++-
 5 files changed, 16 insertions(+), 10 deletions(-)
db/addr.c +0 -0
index f74dd62..f9f760d 100644

db/addr.c addr_f( · line 90

Quoted 10 lines
db/addr.c +3 -4
@@ -85,16 +85,14 @@ addr_f(
 	fl = flist_scan(argv[1]);
 	if (fl == NULL)
 		return 0;
-	if (!flist_parse(fld, fl, iocur_top->data, 0)) {
-		flist_free(fl);
-		return 0;
-	}
+	if (!flist_parse(fld, fl, iocur_top->data, 0))
+		goto out;
+	

You've got some trailing whitespace here (tab in the empty line).

Brian

Quoted 121 lines
db/addr.c +4 -4
 	flist_print(fl);
 	for (tfl = fl; tfl->child != NULL; tfl = tfl->child) {
 		if ((tfl->flags & FL_OKLOW) && tfl->low < tfl->high) {
 			dbprintf(_("array not allowed for addr command\n"));
-			flist_free(fl);
-			return 0;
+			goto out;
 		}
 	}
 	fld = tfl->fld;
@@ -103,7 +101,7 @@ addr_f(
 		next = inode_next_type();
 	if (next == TYP_NONE) {
 		dbprintf(_("no next type for field %s\n"), fld->name);
-		return 0;
+		goto out;
 	}
 	fa = &ftattrtab[fld->ftyp];
 	ASSERT(fa->ftyp == fld->ftyp);
@@ -111,9 +109,10 @@ addr_f(
 	if (adf == NULL) {
 		dbprintf(_("no addr function for field %s (type %s)\n"),
 			fld->name, fa->name);
-		return 0;
+		goto out;
 	}
 	(*adf)(iocur_top->data, tfl->offset, next);
+out:
 	flist_free(fl);
 	return 0;
 }
db/check.c +3 -1
index 4867698..baf7f9f 100644
@@ -1136,7 +1136,7 @@ blocktrash_f(
 	}
 	if (blocks == 0) {
 		dbprintf(_("blocktrash: no matching blocks\n"));
-		return 0;
+		goto out;
 	}
 	if (!sopt)
 		dbprintf(_("blocktrash: seed %u\n"), seed);
@@ -1161,6 +1161,7 @@ blocktrash_f(
 			}
 		}
 	}
+out:
 	xfree(lentab);
 	return 0;
 }
@@ -1907,6 +1908,7 @@ ncheck_f(
 			break;
 		default:
 			dbprintf(_("bad option -%c for ncheck command\n"), c);
+			xfree(ilist);
 			return 0;
 		}
 	}
db/write.c +2 -0
index 7b34fc0..ca8bd0f 100644
@@ -233,6 +233,7 @@ bwrite_lrot(
 	memcpy(hold_region, base, shift);
 	memcpy(base, base+shift, len-shift);
 	memcpy(base+(len-shift), hold_region, shift);
+	free(hold_region);
 }
 
 /* ARGSUSED */
@@ -265,6 +266,7 @@ bwrite_rrot(
 	memcpy(hold_region, base+(len-shift), shift);
 	memmove(base+shift, base, len-shift);
 	memcpy(base, hold_region, shift);
+	free(hold_region);
 }
 
 /* ARGSUSED */
io/parent.c +2 -0
index 47faaa0..ca989e9 100644
@@ -258,6 +258,8 @@ parent_check(void)
 	if (!bstatbuf || !parentbuf) {
 		fprintf(stderr, _("unable to allocate buffers: %s\n"),
 			strerror(errno));
+		free(bstatbuf);
+		free(parentbuf);
 		return 1;
 	}
 
mkfs/proto.c +2 -1
index b7e0761..95583c9 100644
@@ -49,7 +49,7 @@ char *
 setup_proto(
 	char	*fname)
 {
-	char		*buf;
+	char		*buf = NULL;
 	static char	dflt[] = "d--755 0 0 $";
 	int		fd;
 	long		size;
@@ -85,6 +85,7 @@ setup_proto(
 
 out_fail:
 	close(fd);
+	free(buf);
 	exit(1);
 }
 
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#5 lore raw

PATCH v2 3/14 xfsprogs: trivial buffer frees on error paths

In reply to Eric Sandeen

Lots of memory leaks on error paths etc, spotted by coverity. This patch rolls up the super-straightforward fixes across xfsprogs.

---

V2: Fix whitespace error

 db/addr.c    |   15 +++++++--------
 db/check.c   |    4 +++-
 db/write.c   |    2 ++
 io/parent.c  |    2 ++
 mkfs/proto.c |    3 ++-
 5 files changed, 16 insertions(+), 10 deletions(-)
db/addr.c +7 -8
index f74dd62..f9f760d 100644
@@ -85,16 +85,14 @@ addr_f(
 	fl = flist_scan(argv[1]);
 	if (fl == NULL)
 		return 0;
-	if (!flist_parse(fld, fl, iocur_top->data, 0)) {
-		flist_free(fl);
-		return 0;
-	}
+	if (!flist_parse(fld, fl, iocur_top->data, 0))
+		goto out;
+
 	flist_print(fl);
 	for (tfl = fl; tfl->child != NULL; tfl = tfl->child) {
 		if ((tfl->flags & FL_OKLOW) && tfl->low < tfl->high) {
 			dbprintf(_("array not allowed for addr command\n"));
-			flist_free(fl);
-			return 0;
+			goto out;
 		}
 	}
 	fld = tfl->fld;
@@ -103,7 +101,7 @@ addr_f(
 		next = inode_next_type();
 	if (next == TYP_NONE) {
 		dbprintf(_("no next type for field %s\n"), fld->name);
-		return 0;
+		goto out;
 	}
 	fa = &ftattrtab[fld->ftyp];
 	ASSERT(fa->ftyp == fld->ftyp);
@@ -111,9 +109,10 @@ addr_f(
 	if (adf == NULL) {
 		dbprintf(_("no addr function for field %s (type %s)\n"),
 			fld->name, fa->name);
-		return 0;
+		goto out;
 	}
 	(*adf)(iocur_top->data, tfl->offset, next);
+out:
 	flist_free(fl);
 	return 0;
 }
db/check.c +3 -1
index 4867698..baf7f9f 100644
@@ -1136,7 +1136,7 @@ blocktrash_f(
 	}
 	if (blocks == 0) {
 		dbprintf(_("blocktrash: no matching blocks\n"));
-		return 0;
+		goto out;
 	}
 	if (!sopt)
 		dbprintf(_("blocktrash: seed %u\n"), seed);
@@ -1161,6 +1161,7 @@ blocktrash_f(
 			}
 		}
 	}
+out:
 	xfree(lentab);
 	return 0;
 }
@@ -1907,6 +1908,7 @@ ncheck_f(
 			break;
 		default:
 			dbprintf(_("bad option -%c for ncheck command\n"), c);
+			xfree(ilist);
 			return 0;
 		}
 	}
db/write.c +2 -0
index 7b34fc0..ca8bd0f 100644
@@ -233,6 +233,7 @@ bwrite_lrot(
 	memcpy(hold_region, base, shift);
 	memcpy(base, base+shift, len-shift);
 	memcpy(base+(len-shift), hold_region, shift);
+	free(hold_region);
 }
 
 /* ARGSUSED */
@@ -265,6 +266,7 @@ bwrite_rrot(
 	memcpy(hold_region, base+(len-shift), shift);
 	memmove(base+shift, base, len-shift);
 	memcpy(base, hold_region, shift);
+	free(hold_region);
 }
 
 /* ARGSUSED */
io/parent.c +2 -0
index 47faaa0..ca989e9 100644
@@ -258,6 +258,8 @@ parent_check(void)
 	if (!bstatbuf || !parentbuf) {
 		fprintf(stderr, _("unable to allocate buffers: %s\n"),
 			strerror(errno));
+		free(bstatbuf);
+		free(parentbuf);
 		return 1;
 	}
 
mkfs/proto.c +2 -1
index b7e0761..95583c9 100644
@@ -49,7 +49,7 @@ char *
 setup_proto(
 	char	*fname)
 {
-	char		*buf;
+	char		*buf = NULL;
 	static char	dflt[] = "d--755 0 0 $";
 	int		fd;
 	long		size;
@@ -85,6 +85,7 @@ setup_proto(
 
 out_fail:
 	close(fd);
+	free(buf);
 	exit(1);
 }
 

-- 1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#6 lore raw

PATCH 4/14 xfsprogs: fix memory leak in xlog_recover_add_to_trans

In reply to Eric Sandeen

Free the memory in error path of xlog_recover_add_to_trans(). Normally this memory is freed in recovery pass2, but is leaked in the error path.

Userspace version of kernel commits 519ccb8 & aaaae98

---
 libxlog/xfs_log_recover.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
libxlog/xfs_log_recover.c +1 -0
index 3f22921..d1fa535 100644
@@ -1131,6 +1131,7 @@ xlog_recover_add_to_trans(
 		"bad number of regions (%d) in inode log format",
 				  in_f->ilf_size);
 			ASSERT(0);
+			kmem_free(ptr);
 			return XFS_ERROR(EIO);
 		}
 
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#7 lore raw

PATCH 5/14 libxfs: fix memory leak in xfs_dir2_node_removename

In reply to Eric Sandeen

Fix the leak of kernel memory in xfs_dir2_node_removename() when xfs_dir2_leafn_remove() returns an error code.

Cross-port of kernel commit 3a8c9208

---
 libxfs/xfs_dir2_node.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)
libxfs/xfs_dir2_node.c +13 -13
index 3256756..3737e4e 100644
@@ -2088,12 +2088,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);
 
@@ -2105,19 +2105,18 @@ xfs_dir2_node_removename(
 	state->mp = args->dp->i_mount;
 	state->blocksize = state->mp->m_dirblksize;
 	state->node_ents = state->mp->m_dir_node_ents;
-	/*
-	 * Look up the entry we're deleting, set up the cursor.
-	 */
+
+	/* 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.
-	 */
+		goto out_free;
+
+	/* Didn't find it, upper layer screwed up. */
 	if (rval != EEXIST) {
-		xfs_da_state_free(state);
-		return rval;
+		error = rval;
+		goto out_free;
 	}
+
 	blk = &state->path.blk[state->path.active - 1];
 	ASSERT(blk->magic == XFS_DIR2_LEAFN_MAGIC);
 	ASSERT(state->extravalid);
@@ -2128,7 +2127,7 @@ xfs_dir2_node_removename(
 	error = xfs_dir2_leafn_remove(args, blk->bp, blk->index,
 		&state->extrablk, &rval);
 	if (error)
-		return error;
+		goto out_free;
 	/*
 	 * Fix the hash values up the btree.
 	 */
@@ -2143,6 +2142,7 @@ xfs_dir2_node_removename(
 	 */
 	if (!error)
 		error = xfs_dir2_node_to_leaf(state);
+out_free:
 	xfs_da_state_free(state);
 	return error;
 }
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#8 lore raw

PATCH 6/14 xfs_quota: fix memory leak in quota_group_type() error path

In reply to Eric Sandeen

quota_group_type has some rather contorted logic that's been around since 2005.

In the (!name) case, if any of the 3 calls setting up ngroups fails, we fall back to using just one group.

However, if it's the getgroups() call that fails, we overwrite the allocated gid ptr with &gid, thus leaking that allocated memory. Worse, we set "dofree" to 1, so will free non-allocated local var gid. And that last else case is redundant; if we get there, gids is guaranteed to be non-null.

Refactor it a bit to be more clear (I hope) and correct.

---
 quota/quota.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)
quota/quota.c +12 -8
index 7e52ad2..367da8c 100644
@@ -289,15 +289,19 @@ quota_group_type(
 		}
 		gids = &gid;
 		ngroups = 1;
-	} else if ( ((ngroups = sysconf(_SC_NGROUPS_MAX)) < 0) ||
-		    ((gids = malloc(ngroups * sizeof(gid_t))) == NULL) ||
-		    ((ngroups = getgroups(ngroups, gids)) < 0)) {
-		dofree = (gids != NULL);
-		gid = getgid();
-		gids = &gid;
-		ngroups = 1;
 	} else {
-		dofree = (gids != NULL);
+		if ( ((ngroups = sysconf(_SC_NGROUPS_MAX)) < 0) ||
+		     ((gids = malloc(ngroups * sizeof(gid_t))) == NULL) ||
+		     ((ngroups = getgroups(ngroups, gids)) < 0)) {
+			/* something failed.  Fall back to 1 group */
+			free(gids);
+			gid = getgid();
+			gids = &gid;
+			ngroups = 1;
+		} else {
+			/* It all worked, and we allocated memory */
+			dofree = 1;
+		}
 	}
 
 	for (i = 0; i < ngroups; i++, name = NULL) {
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#9 lore raw

PATCH 7/14 xfs_logprint: fix leak in error path of xlog_print_record()

In reply to Eric Sandeen

In 2 error paths we returned without freeing the allocated buf. Collapse them into a compound test & free buf on the way out.

---
 logprint/log_misc.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)
logprint/log_misc.c +6 -5
index 52f1e85..a022df4 100644
@@ -942,11 +942,12 @@ xlog_print_record(
 	     */
 	    if (be32_to_cpu(rhead->h_cycle) !=
 			be32_to_cpu(*(__be32 *)ptr)) {
-		if (*read_type == FULL_READ)
-		    return -1;
-		else if (be32_to_cpu(rhead->h_cycle) + 1 !=
-			be32_to_cpu(*(__be32 *)ptr))
-		    return -1;
+		if ((*read_type == FULL_READ) ||
+		    (be32_to_cpu(rhead->h_cycle) + 1 !=
+				be32_to_cpu(*(__be32 *)ptr))) {
+			free(buf);
+			return -1;
+		}
 	    }
 	}
 
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#10 lore raw

PATCH 8/14 xfsprogs: free resources in libxfs_alloc_file_space error paths

In reply to Eric Sandeen

The bmap freelist & transaction pointer weren't being freed in libxfs_alloc_file_space error paths; more or less copy the error handling that exists in kernelspace to resolve this.

---
 libxfs/util.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)
libxfs/util.c +17 -3
index 1b05540..f1aa4c6 100644
@@ -582,8 +582,17 @@ libxfs_alloc_file_space(
 		resblks = (uint)XFS_DIOSTRAT_SPACE_RES(mp, datablocks);
 		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_write,
 					  resblks, 0);
-		if (error)
+		/*
+		 * Check for running out of space
+		 */
+		if (error) {
+			/*
+			 * Free the transaction structure.
+			 */
+			ASSERT(error == ENOSPC); /* XXX ERS? */
+			xfs_trans_cancel(tp, 0);
 			break;
+		}
 		xfs_trans_ijoin(tp, ip, 0);
 		xfs_trans_ihold(tp, ip);
 
@@ -593,12 +602,12 @@ libxfs_alloc_file_space(
 				&reccount, &free_list);
 
 		if (error)
-			break;
+			goto error0;
 
 		/* complete the transaction */
 		error = xfs_bmap_finish(&tp, &free_list, &committed);
 		if (error)
-			break;
+			goto error0;
 
 		error = xfs_trans_commit(tp, 0);
 		if (error)
@@ -612,6 +621,11 @@ libxfs_alloc_file_space(
 		allocatesize_fsb -= allocated_fsb;
 	}
 	return error;
+
+error0:	/* Cancel bmap, unlock inode, cancel trans */
+	xfs_bmap_cancel(&free_list);
+	xfs_trans_cancel(tp, 0);
+	return error;
 }
 
 unsigned int
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#11 lore raw

Re: [PATCH 08/14] xfsprogs: free resources in libxfs_alloc_file_space error paths

Quoted 17 lines · Eric Sandeen

The bmap freelist & transaction pointer weren't being freed in libxfs_alloc_file_space error paths; more or less copy the error handling that exists in kernelspace to resolve this.

---
 libxfs/util.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)
libxfs/util.c +0 -0
index 1b05540..f1aa4c6 100644
@@ -582,8 +582,17 @@ libxfs_alloc_file_space(
 		resblks = (uint)XFS_DIOSTRAT_SPACE_RES(mp, datablocks);
 		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_write,

libxfs/util.c libxfs_alloc_file_space( · line 592

Quoted 10 lines
libxfs/util.c +8 -1
 					  resblks, 0);
-		if (error)
+		/*
+		 * Check for running out of space
+		 */
+		if (error) {
+			/*
+			 * Free the transaction structure.
+			 */
+			ASSERT(error == ENOSPC); /* XXX ERS? */

XXX ?

Quoted 22 lines
libxfs/util.c +4 -2
+			xfs_trans_cancel(tp, 0);
 			break;
+		}
 		xfs_trans_ijoin(tp, ip, 0);
 		xfs_trans_ihold(tp, ip);
 
@@ -593,12 +602,12 @@ libxfs_alloc_file_space(
 				&reccount, &free_list);
 
 		if (error)
-			break;
+			goto error0;
 
 		/* complete the transaction */
 		error = xfs_bmap_finish(&tp, &free_list, &committed);
 		if (error)
-			break;
+			goto error0;
 
 		error = xfs_trans_commit(tp, 0);
 		if (error)

libxfs/util.c libxfs_alloc_file_space( · line 625

Quoted 5 lines
libxfs/util.c +2 -0
@@ -612,6 +621,11 @@ libxfs_alloc_file_space(
 		allocatesize_fsb -= allocated_fsb;
 	}
 	return error;
+
+error0:	/* Cancel bmap, unlock inode, cancel trans */

Nit, but since it appears we don't lock the inode, the comment is wrong.

Brian

Quoted 13 lines
libxfs/util.c +3 -0
+	xfs_bmap_cancel(&free_list);
+	xfs_trans_cancel(tp, 0);
+	return error;
 }
 
 unsigned int
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#12 lore raw

PATCH v2 8/14 xfsprogs: free resources in libxfs_alloc_file_space error paths

In reply to Eric Sandeen

The bmap freelist & transaction pointer weren't being freed in libxfs_alloc_file_space error paths; more or less copy the error handling that exists in kernelspace to resolve this.

---

V2: Fix comment, remove XXX :)

Although: That ASSERT, while matching kernelspace, seems weird. We *can* get other errors out of xfs_trans_reserve. EIO etc... What's the assert all about?

 libxfs/util.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)
libxfs/util.c +17 -3
index 1b05540..f1aa4c6 100644
@@ -582,8 +582,17 @@ libxfs_alloc_file_space(
 		resblks = (uint)XFS_DIOSTRAT_SPACE_RES(mp, datablocks);
 		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_write,
 					  resblks, 0);
-		if (error)
+		/*
+		 * Check for running out of space
+		 */
+		if (error) {
+			/*
+			 * Free the transaction structure.
+			 */
+			ASSERT(error == ENOSPC);
+			xfs_trans_cancel(tp, 0);
 			break;
+		}
 		xfs_trans_ijoin(tp, ip, 0);
 		xfs_trans_ihold(tp, ip);
 
@@ -593,12 +602,12 @@ libxfs_alloc_file_space(
 				&reccount, &free_list);
 
 		if (error)
-			break;
+			goto error0;
 
 		/* complete the transaction */
 		error = xfs_bmap_finish(&tp, &free_list, &committed);
 		if (error)
-			break;
+			goto error0;
 
 		error = xfs_trans_commit(tp, 0);
 		if (error)
@@ -612,6 +621,11 @@ libxfs_alloc_file_space(
 		allocatesize_fsb -= allocated_fsb;
 	}
 	return error;
+
+error0:	/* Cancel bmap, cancel trans */
+	xfs_bmap_cancel(&free_list);
+	xfs_trans_cancel(tp, 0);
+	return error;
 }
 
 unsigned int

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#13 lore raw

PATCH 9/14 xfsprogs: annotate a case fallthrough in libxfs_ialloc

In reply to Eric Sandeen

This is all working as intended, but add a comment to make it more obvious to readers and static code checkers.

---
 libxfs/util.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
libxfs/util.c +1 -0
index f1aa4c6..1e48079 100644
@@ -265,6 +265,7 @@ libxfs_ialloc(
 	case S_IFSOCK:
 		/* doesn't make sense to set an rdev for these */
 		rdev = 0;
+		/* FALLTHROUGH */
 	case S_IFCHR:
 	case S_IFBLK:
 		ip->i_d.di_format = XFS_DINODE_FMT_DEV;
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#14 lore raw

PATCH 10/14 xfsprogs: fix too-large memset value in xfs_db's attr code

In reply to Eric Sandeen
	memset(value, 0xfeedface, valuelen);

seemed to be trying to fill a new attr with some
recognizeable magic, but of course memset can
only set a single byte; switch this to use 'v'

---
 db/attrset.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
db/attrset.c +1 -1
index 75be72e..762b3bf 100644
@@ -146,7 +146,7 @@ attr_set_f(
 			dbprintf(_("cannot allocate buffer (%d)\n"), valuelen);
 			goto out;
 		}
-		memset(value, 0xfeedface, valuelen);
+		memset(value, 'v', valuelen);
 	} else {
 		value = NULL;
 	}
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#15 lore raw

PATCH 11/14 xfs_quota: remove impossible tests in printpath

In reply to Eric Sandeen

printpath() had some cut & paste tests of "c" - but nothing had set it yet other than c=0, so testing it is pointless. Just remove tests for non-zero "c" until we might have set it to something interesting.

---
 quota/path.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
quota/path.c +2 -2
index e9a6834..47fdd93 100644
@@ -54,9 +54,9 @@ printpath(
 		c = 0;
 		printf(" (");
 		if (qstat.qs_flags & XFS_QUOTA_UDQ_ENFD)
-			c = printf("%suquota", c ? ", " : "");
+			c = printf("uquota");
 		else if (qstat.qs_flags & XFS_QUOTA_UDQ_ACCT)
-			c = printf("%suqnoenforce", c ? ", " : "");
+			c = printf("uqnoenforce");
 		if (qstat.qs_flags & XFS_QUOTA_GDQ_ENFD)
 			c = printf("%sgquota", c ? ", " : "");
 		else if (qstat.qs_flags & XFS_QUOTA_GDQ_ACCT)
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#16 lore raw

PATCH 12/14 xfs_repair: address never-true tests in repair/bmap.c on 64 bit

In reply to Eric Sandeen

The test "if (new_naexts > BLKMAP_NEXTS_MAX)" is never true on a 64-bit platform; new_naexts is an int, and BLKMAP_NEXTS_MAX is INT_MAX. So just wrap the whole thing in the #ifdef.

---
 repair/bmap.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
repair/bmap.c +4 -4
index 85d66dc..b81efb9 100644
@@ -47,16 +47,16 @@ blkmap_alloc(
 	if (nex < 1)
 		nex = 1;
 
+#if (BITS_PER_LONG == 32)	/* on 64-bit platformsethis is never true */
 	if (nex > BLKMAP_NEXTS_MAX) {
-#if (BITS_PER_LONG == 32)
 		do_warn(
 	_("Number of extents requested in blkmap_alloc (%d) overflows 32 bits.\n"
 	  "If this is not a corruption, then you will need a 64 bit system\n"
 	  "to repair this filesystem.\n"),
 			nex);
-#endif
 		return NULL;
 	}
+#endif
 
 	key = whichfork ? ablkmap_key : dblkmap_key;
 	blkmap = pthread_getspecific(key);
@@ -267,15 +267,15 @@ blkmap_grow(
 		ASSERT(pthread_getspecific(key) == blkmap);
 	}
 
+#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
 	if (new_naexts > BLKMAP_NEXTS_MAX) {
-#if (BITS_PER_LONG == 32)
 		do_error(
 	_("Number of extents requested in blkmap_grow (%d) overflows 32 bits.\n"
 	  "You need a 64 bit system to repair this filesystem.\n"),
 			new_naexts);
-#endif
 		return NULL;
 	}
+#endif
 	if (new_naexts <= 0) {
 		do_error(
 	_("Number of extents requested in blkmap_grow (%d) overflowed the\n"
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v1 12/14
#17 lore raw

Re: [PATCH 12/14] xfs_repair: address never-true tests in repair/bmap.c on 64 bit

Quoted 14 lines · Eric Sandeen

The test "if (new_naexts > BLKMAP_NEXTS_MAX)" is never true on a 64-bit platform; new_naexts is an int, and BLKMAP_NEXTS_MAX is INT_MAX. So just wrap the whole thing in the #ifdef.

---
  repair/bmap.c |    8 ++++----
  1 files changed, 4 insertions(+), 4 deletions(-)
repair/bmap.c +0 -0
index 85d66dc..b81efb9 100644

repair/bmap.c blkmap_alloc( · line 50

Quoted 4 lines
repair/bmap.c +1 -0
@@ -47,16 +47,16 @@ blkmap_alloc(
  	if (nex < 1)
  		nex = 1;
 
+#if (BITS_PER_LONG == 32)	/* on 64-bit platformsethis is never true */

platforms this ..,

repair/bmap.c blkmap_alloc( · line 59

Quoted 11 lines
repair/bmap.c +1 -2
  	if (nex > BLKMAP_NEXTS_MAX) {
-#if (BITS_PER_LONG == 32)
  		do_warn(
  	_("Number of extents requested in blkmap_alloc (%d) overflows 32 bits.\n"
  	  "If this is not a corruption, then you will need a 64 bit system\n"
  	  "to repair this filesystem.\n"),
  			nex);
-#endif
  		return NULL;
  	}
+#endif

Looks good. The other patches look good too.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#18 lore raw

PATCH v2 12/14 xfs_repair: address never-true tests in repair/bmap.c on 64 bit

In reply to Eric Sandeen

The test "if (new_naexts > BLKMAP_NEXTS_MAX)" is never true on a 64-bit platform; new_naexts is an int, and BLKMAP_NEXTS_MAX is INT_MAX. So just wrap the whole thing in the #ifdef.

---

V2: Fix typo in comment in first hunk

 repair/bmap.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
repair/bmap.c +4 -4
index 85d66dc..b81efb9 100644
@@ -47,16 +47,16 @@ blkmap_alloc(
 	if (nex < 1)
 		nex = 1;
 
+#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
 	if (nex > BLKMAP_NEXTS_MAX) {
-#if (BITS_PER_LONG == 32)
 		do_warn(
 	_("Number of extents requested in blkmap_alloc (%d) overflows 32 bits.\n"
 	  "If this is not a corruption, then you will need a 64 bit system\n"
 	  "to repair this filesystem.\n"),
 			nex);
-#endif
 		return NULL;
 	}
+#endif
 
 	key = whichfork ? ablkmap_key : dblkmap_key;
 	blkmap = pthread_getspecific(key);
@@ -267,15 +267,15 @@ blkmap_grow(
 		ASSERT(pthread_getspecific(key) == blkmap);
 	}
 
+#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
 	if (new_naexts > BLKMAP_NEXTS_MAX) {
-#if (BITS_PER_LONG == 32)
 		do_error(
 	_("Number of extents requested in blkmap_grow (%d) overflows 32 bits.\n"
 	  "You need a 64 bit system to repair this filesystem.\n"),
 			new_naexts);
-#endif
 		return NULL;
 	}
+#endif
 	if (new_naexts <= 0) {
 		do_error(
 	_("Number of extents requested in blkmap_grow (%d) overflowed the\n"

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#19 lore raw

PATCH 13/14 mkfs: catch unknown format in protofile parsing

In reply to Eric Sandeen

As the code stands today we can't get an unknown format in the last case statement, but Coverity warns that if we ever do, we'll use an uninitialized "ip" in the call to libxfs_trans_log_inode().

Adding a default: case to catch unknown formats is defensive and makes the checker happy.

---
 mkfs/proto.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
mkfs/proto.c +3 -0
index 95583c9..308325b 100644
@@ -591,6 +591,9 @@ parseproto(
 		}
 		libxfs_iput(ip, 0);
 		return;
+	default:
+		ASSERT(0);
+		fail(_("Unknown format"), EINVAL);
 	}
 	libxfs_trans_log_inode(tp, ip, flags);
 	error = libxfs_bmap_finish(&tp, &flist, &committed);
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#20 lore raw

PATCH 14/14 xfs_db: don't use invalid index in ring_f

In reply to Eric Sandeen

ring_f() tests for an invalid index which would overrun the iocur_ring[] array and warns, but then uses it anyway.

Return immediately if it's out of bounds.

---
 db/io.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
db/io.c +3 -1
index 9a787c8..6aba54e 100644
@@ -358,8 +358,10 @@ ring_f(
 	}
 
 	index = (int)strtoul(argv[1], NULL, 0);
-	if (index < 0 || index >= RING_ENTRIES)
+	if (index < 0 || index >= RING_ENTRIES) {
 		dbprintf(_("invalid entry: %d\n"), index);
+		return 0;
+	}
 
 	ring_current = index;
 
Signature · 6 lines
1.7.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#21 lore raw

[PATCH 15/14] xfs_io: free memory on error path exit from bmap_f()

In reply to Eric Sandeen

If realloc() fails, the memory it was trying to realloc remains; free it on the error path.

---

io/bmap.c +1 -0
index a78cbb1..8e5d75e 100644
@@ -227,6 +227,7 @@ bmap_f(
 				fprintf(stderr,
 					_("%s: cannot realloc %d bytes\n"),
 					progname, (int)(map_size*sizeof(*map)));
+				free(map);
 				exitcode = 1;
 				return 0;
 			}

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#22 lore raw

Re: [PATCH 15/14] xfs_io: free memory on error path exit from bmap_f()

Quoted 11 lines · Eric Sandeen

If realloc() fails, the memory it was trying to realloc remains; free it on the error path.

---

io/bmap.c +0 -0
index a78cbb1..8e5d75e 100644

io/bmap.c bmap_f( · line 233

Quoted 7 lines
io/bmap.c +1 -0
@@ -227,6 +227,7 @@ bmap_f(
  				fprintf(stderr,
  					_("%s: cannot realloc %d bytes\n"),
  					progname, (int)(map_size*sizeof(*map)));
+				free(map);
  				exitcode = 1;
  				return 0;
  			}

Didn't it get into this block by comparing map to NULL?

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#23 lore raw

Re: [PATCH 15/14] xfs_io: free memory on error path exit from bmap_f()

Quoted 7 lines · Mark Tinguely
Quoted 6 lines · Eric Sandeen

If realloc() fails, the memory it was trying to realloc remains; free it on the error path.

---

Quoted 15 lines

io/bmap.c bmap_f( · line 233

Quoted 12 lines
io/bmap.c +1 -0
index a78cbb1..8e5d75e 100644
@@ -227,6 +227,7 @@ bmap_f(
                  fprintf(stderr,
                      _("%s: cannot realloc %d bytes\n"),
                      progname, (int)(map_size*sizeof(*map)));
+                free(map);
                  exitcode = 1;
                  return 0;
              }

Didn't it get into this block by comparing map to NULL?

Uh yes, yes we did. Sorry, was getting a bit slap-happy by then. I guess p = realloc(p) will always lose the memory if it fails, w/o a temporary var.

Sorry about that!
-Eric

Quoted 2 lines

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on the v1 series
#24 lore raw
In reply to Eric Sandeen
Quoted 2 lines · Eric Sandeen

This series knocks out 30 or so coverity defects.

Noted a few nits... things that could probably be fixed up at commit time if so desired. Otherwise, the series looks good to me. Thanks Eric.

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs