[Cluster-devel] [PATCH 7/7] gfs2_edit: More static analysis fixes

[Andrew Price <anprice@redhat.com>]

Fix some more minor issues found by Coverity and Clang, including
unchecked lseek()s, unbound strcpy()s, branching based on an
uninitialised variable, potential NULL pointer derefs, some dead
assignments and a dangling reference to a local variable.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/extended.c |  5 +----
 gfs2/edit/gfs2hex.c  |  7 ++++---
 gfs2/edit/hexedit.c  | 13 +++++++------
 gfs2/edit/journal.c  | 36 +++++++++++++++---------------------
 gfs2/edit/savemeta.c | 18 ++++++------------
 5 files changed, 33 insertions(+), 46 deletions(-)

diff --git a/gfs2/edit/extended.c b/gfs2/edit/extended.c
index 793ef6b..e5cb12c 100644
--- a/gfs2/edit/extended.c
+++ b/gfs2/edit/extended.c
@@ -391,9 +391,8 @@ static void print_block_details(struct iinfo *ind, int level, int cur_height,
 		return;
 	}
 	while (thisblk) {
-		lseek(sbd.device_fd, thisblk * sbd.bsize, SEEK_SET);
 		/* read in the desired block */
-		if (read(sbd.device_fd, tmpbuf, sbd.bsize) != sbd.bsize) {
+		if (pread(sbd.device_fd, tmpbuf, sbd.bsize, thisblk * sbd.bsize) != sbd.bsize) {
 			fprintf(stderr, "bad read: %s from %s:%d: block %lld "
 				"(0x%llx)\n", strerror(errno), __FUNCTION__,
 				__LINE__,
@@ -446,7 +445,6 @@ static int print_gfs_jindex(struct gfs2_inode *dij)
 	char jbuf[sizeof(struct gfs_jindex)];
 
 	start_line = line;
-	error = 0;
 	print_gfs2("Journal index entries found: %d.",
 		   dij->i_di.di_size / sizeof(struct gfs_jindex));
 	eol(0);
@@ -516,7 +514,6 @@ static int parse_rindex(struct gfs2_inode *dip, int print_rindex)
 	char highlighted_addr[32];
 
 	start_line = line;
-	error = 0;
 	print_gfs2("RG index entries found: %d.", dip->i_di.di_size /
 		   sizeof(struct gfs2_rindex));
 	eol(0);
diff --git a/gfs2/edit/gfs2hex.c b/gfs2/edit/gfs2hex.c
index 979aee0..b6075f4 100644
--- a/gfs2/edit/gfs2hex.c
+++ b/gfs2/edit/gfs2hex.c
@@ -38,7 +38,6 @@ char efield[64];
 int edit_mode = 0;
 int edit_row[DMODES], edit_col[DMODES];
 int edit_size[DMODES], last_entry_onscreen[DMODES];
-char edit_fmt[80];
 enum dsp_mode dmode = HEX_MODE; /* display mode */
 uint64_t block = 0;
 int blockhist = 0;
@@ -201,9 +200,11 @@ void print_it(const char *label, const char *fmt, const char *fmt2, ...)
 		if (termlines) {
 			refresh();
 			if (line == (edit_row[dmode] * lines_per_row[dmode]) + 4) {
-				strcpy(efield, label + 2); /* it's indented */
+				strncpy(efield, label + 2, 63); /* it's indented */
+				efield[63] = '\0';
 				strcpy(estring, tmp_string);
-				strcpy(edit_fmt, fmt);
+				strncpy(edit_fmt, fmt, 79);
+				edit_fmt[79] = '\0';
 				edit_size[dmode] = strlen(estring);
 				COLORS_NORMAL;
 			}
diff --git a/gfs2/edit/hexedit.c b/gfs2/edit/hexedit.c
index 5c5ac89..bc3ca35 100644
--- a/gfs2/edit/hexedit.c
+++ b/gfs2/edit/hexedit.c
@@ -1583,7 +1583,7 @@ static uint64_t find_metablockoftype_rg(uint64_t startblk, int metatype, int pri
 	struct osi_node *next = NULL;
 	uint64_t blk, errblk;
 	int first = 1, found = 0;
-	struct rgrp_tree *rgd;
+	struct rgrp_tree *rgd = NULL;
 	struct gfs2_rindex *ri;
 
 	blk = 0;
@@ -1726,7 +1726,7 @@ uint64_t check_keywords(const char *kword)
 		blk = get_rg_addr(rgnum);
 	} else if (!strncmp(kword, "journals", 8)) {
 		blk = JOURNALS_DUMMY_BLOCK;
-	} else if (!strncmp(kword, "journal", 7) && isdigit(kword[7])) {
+	} else if (strlen(kword) > 7 && !strncmp(kword, "journal", 7) && isdigit(kword[7])) {
 		uint64_t j_size;
 
 		blk = find_journal_block(kword, &j_size);
@@ -1837,8 +1837,7 @@ static void hex_edit(int *exitch)
 					ch += (estring[i+1] - 'A' + 0x0a);
 				bh->b_data[offset + hexoffset] = ch;
 			}
-			lseek(sbd.device_fd, dev_offset, SEEK_SET);
-			if (write(sbd.device_fd, bh->b_data, sbd.bsize) !=
+			if (pwrite(sbd.device_fd, bh->b_data, sbd.bsize, dev_offset) !=
 			    sbd.bsize) {
 				fprintf(stderr, "write error: %s from %s:%d: "
 					"offset %lld (0x%llx)\n",
@@ -2676,8 +2675,10 @@ static void parameterpass1(int argc, char *argv[], int i)
 		termlines = 0;
 	else if (!strcasecmp(argv[i], "-x"))
 		dmode = HEX_MODE;
-	else if (!device[0] && strchr(argv[i],'/'))
-		strcpy(device, argv[i]);
+	else if (!device[0] && strchr(argv[i],'/')) {
+		strncpy(device, argv[i], NAME_MAX-1);
+		device[NAME_MAX-1] = '\0';
+	}
 }
 
 /* ------------------------------------------------------------------------ */
diff --git a/gfs2/edit/journal.c b/gfs2/edit/journal.c
index 3c2f5c7..5000c2e 100644
--- a/gfs2/edit/journal.c
+++ b/gfs2/edit/journal.c
@@ -98,16 +98,6 @@ static void check_journal_wrap(uint64_t seq, uint64_t *highest_seq)
 	*highest_seq = seq;
 }
 
-static int is_meta(struct gfs2_buffer_head *lbh)
-{
-	uint32_t check_magic = ((struct gfs2_meta_header *)(lbh->b_data))->mh_magic;
-
-	check_magic = be32_to_cpu(check_magic);
-	if (check_magic == GFS2_MAGIC)
-		return 1;
-	return 0;
-}
-
 /**
  * fsck_readi - same as libgfs2's gfs2_readi, but sets absolute block #
  *              of the first bit of data read.
@@ -115,16 +105,20 @@ static int is_meta(struct gfs2_buffer_head *lbh)
 static int fsck_readi(struct gfs2_inode *ip, void *rbuf, uint64_t roffset,
 	       unsigned int size, uint64_t *abs_block)
 {
-	struct gfs2_sbd *sdp = ip->i_sbd;
+	struct gfs2_sbd *sdp;
 	struct gfs2_buffer_head *lbh;
 	uint64_t lblock, dblock;
 	unsigned int o;
 	uint32_t extlen = 0;
 	unsigned int amount;
 	int not_new = 0;
-	int isdir = !!(S_ISDIR(ip->i_di.di_mode));
+	int isdir;
 	int copied = 0;
 
+	if (ip == NULL)
+		return 0;
+	sdp = ip->i_sbd;
+	isdir = !!(S_ISDIR(ip->i_di.di_mode));
 	*abs_block = 0;
 	if (roffset >= ip->i_di.di_size)
 		return 0;
@@ -287,9 +281,9 @@ static int print_ld_blks(const uint64_t *b, const char *end, int start_line,
 	}
 	if (prnt)
 		eol(0);
-	if (!found_tblk || !is_meta_ld)
+	if (tblk_off && (!found_tblk || !is_meta_ld))
 		*tblk_off = 0;
-	if (!found_bblk || !is_meta_ld)
+	if (bblk_off && (!found_bblk || !is_meta_ld))
 		*bblk_off = 0;
 	return bcount;
 }
@@ -443,7 +437,7 @@ void dump_journal(const char *journal, int tblk)
 {
 	struct gfs2_buffer_head *j_bh = NULL, dummy_bh;
 	uint64_t jblock, j_size, jb, abs_block, saveblk, wrappt = 0;
-	int error, start_line, journal_num;
+	int start_line, journal_num;
 	struct gfs2_inode *j_inode = NULL;
 	int ld_blocks = 0, offset_from_ld = 0;
 	uint64_t tblk_off = 0, bblk_off = 0, bitblk = 0;
@@ -454,7 +448,6 @@ void dump_journal(const char *journal, int tblk)
 
 	start_line = line;
 	lines_per_row[dmode] = 1;
-	error = 0;
 	journal_num = atoi(journal + 7);
 	print_gfs2("Dumping journal #%d.", journal_num);
 	if (tblk) {
@@ -518,6 +511,7 @@ void dump_journal(const char *journal, int tblk)
 
 	for (jb = 0; jb < j_size; jb += (sbd.gfs1 ? 1 : sbd.bsize)) {
 		int is_pertinent = 1;
+		uint32_t block_type = 0;
 
 		if (sbd.gfs1) {
 			if (j_bh)
@@ -526,7 +520,7 @@ void dump_journal(const char *journal, int tblk)
 			j_bh = bread(&sbd, abs_block);
 			dummy_bh.b_data = j_bh->b_data;
 		} else {
-			error = fsck_readi(j_inode, (void *)jbuf,
+			int error = fsck_readi(j_inode, (void *)jbuf,
 					   ((jb + wrappt) % j_size),
 					   sbd.bsize, &abs_block);
 			if (!error) /* end of file */
@@ -534,14 +528,14 @@ void dump_journal(const char *journal, int tblk)
 			dummy_bh.b_data = jbuf;
 		}
 		offset_from_ld++;
-		if (get_block_type(&dummy_bh, NULL) == GFS2_METATYPE_LD) {
+		block_type = get_block_type(&dummy_bh, NULL);
+		if (block_type == GFS2_METATYPE_LD) {
 			ld_blocks = process_ld(abs_block, wrappt, j_size, jb,
 					       &dummy_bh, tblk, &tblk_off,
 					       bitblk, rgd, &is_pertinent,
 					       &bblk_off, ld_blk_refs);
 			offset_from_ld = 0;
-		} else if (!tblk &&
-			   get_block_type(&dummy_bh, NULL) == GFS2_METATYPE_LH) {
+		} else if (!tblk && block_type == GFS2_METATYPE_LH) {
 			struct gfs2_log_header lh;
 			struct gfs_log_header lh1;
 
@@ -577,7 +571,7 @@ void dump_journal(const char *journal, int tblk)
 						    sbd.bsize), start_line,
 						   tblk, &tblk_off, 0, rgd,
 						   0, 1, NULL, 1, NULL);
-		} else if (!is_meta(&dummy_bh)) {
+		} else if (block_type == 0) {
 			continue;
 		}
 		/* Check if this metadata block references the block we're
diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index 5bf7963..c9f2d0a 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -662,7 +662,7 @@ static void save_allocated(struct rgrp_tree *rgd, struct metafd *mfd)
 		 * If we don't, we may run into metadata allocation issues. */
 		m = lgfs2_bm_scan(rgd, i, ibuf, GFS2_BLKST_UNLINKED);
 		for (j = 0; j < m; j++) {
-			blktype = save_block(sbd.device_fd, mfd, block);
+			save_block(sbd.device_fd, mfd, block);
 		}
 	}
 	free(ibuf);
@@ -933,6 +933,7 @@ static int restore_data(int fd, gzFile gzin_fd, int printblocksonly,
 			if (printblocksonly > 1 && printblocksonly == block) {
 				block_in_mem = block;
 				display(0, 0, 0, 0);
+				bh = NULL;
 				return 0;
 			} else if (printblocksonly == 1) {
 				print_gfs2("%d (l=0x%x): ", blks_saved,
@@ -944,19 +945,10 @@ static int restore_data(int fd, gzFile gzin_fd, int printblocksonly,
 			if (savedata->blk >= sbd.fssize) {
 				printf("\nOut of space on the destination "
 				       "device; quitting.\n");
+				bh = NULL;
 				break;
 			}
-			if (lseek(fd, savedata->blk * sbd.bsize, SEEK_SET) !=
-			    savedata->blk * sbd.bsize) {
-				fprintf(stderr, "bad seek: %s from %s:"
-					"%d: block %lld (0x%llx)\n",
-					strerror(errno), __FUNCTION__,
-					__LINE__,
-					(unsigned long long)savedata->blk,
-					(unsigned long long)savedata->blk);
-				exit(-1);
-			}
-			if (write(fd, savedata->buf, sbd.bsize) != sbd.bsize) {
+			if (pwrite(fd, savedata->buf, sbd.bsize, savedata->blk * sbd.bsize) != sbd.bsize) {
 				fprintf(stderr, "write error: %s from "
 					"%s:%d: block %lld (0x%llx)\n",
 					strerror(errno), __FUNCTION__,
@@ -970,6 +962,8 @@ static int restore_data(int fd, gzFile gzin_fd, int printblocksonly,
 				fsync(fd);
 		}
 		blks_saved++;
+		/* Don't leave a dangling reference to our local dummy_bh */
+		bh = NULL;
 	}
 	if (!printblocksonly && !find_highblk)
 		warm_fuzzy_stuff(sbd.fssize, TRUE);
-- 
1.8.5.3