[Cluster-devel] [PATCH 1/8] gfs2_edit: Check more error values from gfs2_get_bitmap

[Cluster-devel] [PATCH 1/8] gfs2_edit: Check more error values from gfs2_get_bitmap

[Andrew Price]

Spotted by coverity: gfs2_get_bitmap returns -1 on error and it was
being used as an array index in display_block_type. (2 remaining
occurrences.)

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/hexedit.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/gfs2/edit/hexedit.c b/gfs2/edit/hexedit.c
index 69d499e..808ebb1 100644
--- a/gfs2/edit/hexedit.c
+++ b/gfs2/edit/hexedit.c
@@ -697,8 +697,13 @@ int display_block_type(int from_restore)
 			if ((be32_to_cpu(mh->mh_type) == GFS2_METATYPE_RG) ||
 			    (be32_to_cpu(mh->mh_type) == GFS2_METATYPE_RB))
 				type = 4;
-			else
+			else {
 				type = gfs2_get_bitmap(&sbd, block, rgd);
+				if (type < 0) {
+					fprintf(stderr, "Failed to retrieve block state from bitmap\n");
+					exit(-1);
+				}
+			}
 		} else
 			type = 4;
 		screen_chunk_size = ((termlines - 4) * 16) >> 8 << 8;
@@ -724,6 +729,10 @@ int display_block_type(int from_restore)
 				print_gfs2(" blk ");
 				for (b = blknum; b < blknum + 4; b++) {
 					btype = gfs2_get_bitmap(&sbd, b, rgd);
+					if (btype < 0) {
+						fprintf(stderr, "Failed to retrieve block state from bitmap\n");
+						exit(-1);
+					}
 					print_gfs2("0x%x-%s  ", b,
 						   allocdesc[sbd.gfs1][btype]);
 				}
-- 
1.7.6.5

[Cluster-devel] [PATCH 2/8] gfs2_edit: Fix another resource leak in display_extended

[Andrew Price]

Spotted by coverity: Variable "tmp_inode" going out of scope leaks the
storage it points to. (1 remaining occurrence.)

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/extended.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gfs2/edit/extended.c b/gfs2/edit/extended.c
index 1066e1b..566fb5b 100644
--- a/gfs2/edit/extended.c
+++ b/gfs2/edit/extended.c
@@ -656,6 +656,7 @@ int display_extended(void)
 		tmp_bh = bread(&sbd, block);
 		tmp_inode = inode_get(&sbd, tmp_bh);
 		parse_rindex(tmp_inode, TRUE);
+		inode_put(&tmp_inode);
 		brelse(tmp_bh);
 	}
 	else if (has_indirect_blocks() && !indirect_blocks &&
-- 
1.7.6.5

[Cluster-devel] [PATCH 3/8] mkfs.gfs2: Fix use of uninitialized value in check_dev_content

[Andrew Price]

Spotted by coverity: Using uninitialized value "p[0]" when calling
"close".

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/mkfs/main_mkfs.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index 95afe41..3658fd4 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -428,7 +428,7 @@ static void check_dev_content(const char *devname)
 		(char *)"-bs",
 		(char *)devname,
 		NULL };
-	int p[2];
+	int p[2] = {-1, -1};
 	int ret;
 	int pid;
 
@@ -463,7 +463,8 @@ fail:
 				goto fail;
 			printf( _("It appears to contain: %s"), content);
 		}
-		close(p[0]);
+		if (p[0] >= 0)
+			close(p[0]);
 		return;
 	}
 
-- 
1.7.6.5

[Cluster-devel] [PATCH 4/8] gfs2_convert: Fix null pointer deref in journ_space_to_rg

[Andrew Price]

Spotted by coverity: Dereferencing null variable "rgdhigh".

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/convert/gfs2_convert.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gfs2/convert/gfs2_convert.c b/gfs2/convert/gfs2_convert.c
index 307b0f3..9b54670 100644
--- a/gfs2/convert/gfs2_convert.c
+++ b/gfs2/convert/gfs2_convert.c
@@ -1777,13 +1777,13 @@ static int journ_space_to_rg(struct gfs2_sbd *sdp)
 				 (rgd->ri.ri_addr > rgdhigh->ri.ri_addr)))
 				rgdhigh = rgd;
 		} /* for each rg */
-		log_info(_("Addr 0x%llx comes after rg at addr 0x%llx\n"),
-			 (unsigned long long)jndx->ji_addr,
-			 (unsigned long long)rgdhigh->ri.ri_addr);
 		if (!rgdhigh) { /* if we somehow didn't find one. */
 			log_crit(_("Error: No suitable rg found for journal.\n"));
 			return -1;
 		}
+		log_info(_("Addr 0x%llx comes after rg at addr 0x%llx\n"),
+			 (unsigned long long)jndx->ji_addr,
+			 (unsigned long long)rgdhigh->ri.ri_addr);
 		ri_addr = jndx->ji_addr;
 		/* Allocate a new rgd entry which includes rg and ri. */
 		rgd = rgrp_insert(&sdp->rgtree, ri_addr);
-- 
1.7.6.5

[Cluster-devel] [PATCH 5/8] gfs2_convert: Fix null pointer deref in conv_build_jindex

[Andrew Price]

Spotted by coverity: Dereferencing a null pointer "sdp->md.journal".

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/convert/gfs2_convert.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gfs2/convert/gfs2_convert.c b/gfs2/convert/gfs2_convert.c
index 9b54670..9944d23 100644
--- a/gfs2/convert/gfs2_convert.c
+++ b/gfs2/convert/gfs2_convert.c
@@ -1918,6 +1918,9 @@ static int conv_build_jindex(struct gfs2_sbd *sdp)
 
 	sdp->md.journal = malloc(sdp->md.journals *
 				 sizeof(struct gfs2_inode *));
+	if (sdp->md.journal == NULL) {
+		return errno;
+	}
 	for (j = 0; j < sdp->md.journals; j++) {
 		char name[256];
 
-- 
1.7.6.5

[Cluster-devel] [PATCH 6/8] fsck.gfs2: Remove unsigned comparisons with zero

[Andrew Price]

Spotted by coverity: This less-than-zero comparison of an unsigned value
is never true. "q < 0". (Fixes 6 occurrences)

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/metawalk.c |    9 +++------
 gfs2/fsck/pass1b.c   |    2 +-
 gfs2/fsck/pass2.c    |    2 +-
 gfs2/fsck/pass5.c    |    6 ------
 4 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index d78df72..eb80ccf 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1530,8 +1530,7 @@ static int alloc_metalist(struct gfs2_inode *ip, uint64_t block,
 	   after the bitmap has been set but before the blockmap has. */
 	*bh = bread(ip->i_sbd, block);
 	q = block_type(block);
-	if (q < 0 ||
-	    blockmap_to_bitmap(q, ip->i_sbd->gfs1) == GFS2_BLKST_FREE) {
+	if (blockmap_to_bitmap(q, ip->i_sbd->gfs1) == GFS2_BLKST_FREE) {
 		log_debug(_("%s reference to new metadata block "
 			    "%lld (0x%llx) is now marked as indirect.\n"),
 			  desc, (unsigned long long)block,
@@ -1550,8 +1549,7 @@ static int alloc_data(struct gfs2_inode *ip, uint64_t block, void *private)
 	/* We can't check the bitmap here because this function is called
 	   after the bitmap has been set but before the blockmap has. */
 	q = block_type(block);
-	if (q < 0 ||
-	    blockmap_to_bitmap(q, ip->i_sbd->gfs1) == GFS2_BLKST_FREE) {
+	if (blockmap_to_bitmap(q, ip->i_sbd->gfs1) == GFS2_BLKST_FREE) {
 		log_debug(_("%s reference to new data block "
 			    "%lld (0x%llx) is now marked as data.\n"),
 			  desc, (unsigned long long)block,
@@ -1569,8 +1567,7 @@ static int alloc_leaf(struct gfs2_inode *ip, uint64_t block, void *private)
 	/* We can't check the bitmap here because this function is called
 	   after the bitmap has been set but before the blockmap has. */
 	q = block_type(block);
-	if (q < 0 ||
-	    blockmap_to_bitmap(q, ip->i_sbd->gfs1) == GFS2_BLKST_FREE)
+	if (blockmap_to_bitmap(q, ip->i_sbd->gfs1) == GFS2_BLKST_FREE)
 		fsck_blockmap_set(ip, block, _("newly allocated leaf"),
 				  gfs2_leaf_blk);
 	return 0;
diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index 1879cc4..ae6d45c 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -701,7 +701,7 @@ static int handle_dup_blk(struct gfs2_sbd *sdp, struct duptree *b)
 		ip = fsck_load_inode(sdp, id->block_no);
 
 		q = block_type(id->block_no);
-		if (q < 0 || q == gfs2_inode_invalid) {
+		if (q == gfs2_inode_invalid) {
 			log_debug( _("The remaining reference inode %lld "
 				     "(0x%llx) is marked invalid: Marking "
 				     "the block as free.\n"),
diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index 8562b6a..d89dd4f 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -328,7 +328,7 @@ static int check_dentry(struct gfs2_inode *ip, struct gfs2_dirent *dent,
 	 * 2. Blocks marked "bad" need to have their entire
 	 * metadata tree deleted.
 	*/
-	if (q < 0 || q == gfs2_inode_invalid || q == gfs2_bad_block) {
+	if (q == gfs2_inode_invalid || q == gfs2_bad_block) {
 		/* This entry's inode has bad blocks in it */
 
 		/* Handle bad blocks */
diff --git a/gfs2/fsck/pass5.c b/gfs2/fsck/pass5.c
index cb18b5a..6c08e13 100644
--- a/gfs2/fsck/pass5.c
+++ b/gfs2/fsck/pass5.c
@@ -123,12 +123,6 @@ static int check_block_status(struct gfs2_sbd *sdp, char *buffer,
 			return 0;
 
 		q = block_type(block);
-		if (q < 0) {
-			log_err( _("Invalid block type for block #%llu "
-				   "(0x%llx)\n"), (unsigned long long)block,
-				 (unsigned long long)block);
-			return q;
-		}
 
 		if (sdp->gfs1)
 			block_status = gfs1_convert_mark(q, count);
-- 
1.7.6.5

[Cluster-devel] [PATCH 7/8] fsck.gfs2: Plug a leak in init_system_inodes()

[Andrew Price]

Spotted by coverity: Variable "buf" going out of scope leaks the storage
it points to.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/initialize.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index a1f99d6..bab61c1 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -688,6 +688,7 @@ static int init_system_inodes(struct gfs2_sbd *sdp)
 			if (err != sdp->md.statfs->i_di.di_size) {
 				log_crit(_("Error %d reading statfs file. "
 					   "Aborting.\n"), err);
+				free(buf);
 				goto fail;
 			}
 			/* call gfs2_inum_range_in() to retrieve range */
-- 
1.7.6.5

[Cluster-devel] [PATCH 8/8] libgfs2: Set errno in dirent_alloc and use dir_add consistently

[Andrew Price]

Initially spotted by coverity: "error" is passed to a parameter that
cannot be negative.

It turned out that dirent_alloc was returning -ENOSPC and some calling
code (via dir_add) was assuming that errno was set. Other code was
assuming that dir_add returned -errno. This patch changes dirent_alloc
to set errno and return -1 and also updates the code which calls dir_add
to use errno.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/initialize.c |   12 ++++++------
 gfs2/fsck/pass2.c      |    8 ++++----
 gfs2/fsck/pass3.c      |    2 +-
 gfs2/libgfs2/fs_ops.c  |   12 ++++++++----
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index bab61c1..3daf12d 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -375,7 +375,7 @@ static int rebuild_master(struct gfs2_sbd *sdp)
 		err = dir_add(sdp->master_dir, "jindex", 6, &inum,
 		              IF2DT(S_IFDIR | 0700));
 		if (err) {
-			log_crit(_("Error %d adding jindex directory\n"), err);
+			log_crit(_("Error %d adding jindex directory\n"), errno);
 			exit(FSCK_ERROR);
 		}
 		sdp->master_dir->i_di.di_nlink++;
@@ -394,7 +394,7 @@ static int rebuild_master(struct gfs2_sbd *sdp)
 			IF2DT(S_IFDIR | 0700));
 		if (err) {
 			log_crit(_("Error %d adding per_node directory\n"),
-				 err);
+				 errno);
 			exit(FSCK_ERROR);
 		}
 		sdp->master_dir->i_di.di_nlink++;
@@ -413,7 +413,7 @@ static int rebuild_master(struct gfs2_sbd *sdp)
 		err = dir_add(sdp->master_dir, "inum", 4, &inum,
 			IF2DT(S_IFREG | 0600));
 		if (err) {
-			log_crit(_("Error %d adding inum inode\n"), err);
+			log_crit(_("Error %d adding inum inode\n"), errno);
 			exit(FSCK_ERROR);
 		}
 	} else {
@@ -431,7 +431,7 @@ static int rebuild_master(struct gfs2_sbd *sdp)
 		err = dir_add(sdp->master_dir, "statfs", 6, &inum,
 			      IF2DT(S_IFREG | 0600));
 		if (err) {
-			log_crit(_("Error %d adding statfs inode\n"), err);
+			log_crit(_("Error %d adding statfs inode\n"), errno);
 			exit(FSCK_ERROR);
 		}
 	} else {
@@ -449,7 +449,7 @@ static int rebuild_master(struct gfs2_sbd *sdp)
 		err = dir_add(sdp->master_dir, "rindex", 6, &inum,
 			IF2DT(S_IFREG | 0600));
 		if (err) {
-			log_crit(_("Error %d adding rindex inode\n"), err);
+			log_crit(_("Error %d adding rindex inode\n"), errno);
 			exit(FSCK_ERROR);
 		}
 	} else {
@@ -466,7 +466,7 @@ static int rebuild_master(struct gfs2_sbd *sdp)
 		err = dir_add(sdp->master_dir, "quota", 5, &inum,
 			IF2DT(S_IFREG | 0600));
 		if (err) {
-			log_crit(_("Error %d adding quota inode\n"), err);
+			log_crit(_("Error %d adding quota inode\n"), errno);
 			exit(FSCK_ERROR);
 		}
 	} else {
diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index d89dd4f..4201bb2 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -677,8 +677,8 @@ static int check_system_dir(struct gfs2_inode *sysinode, const char *dirname,
 					 GFS_FILE_DIR : DT_DIR));
 			if (error) {
 				log_err(_("Error adding directory %s: %s\n"),
-				        filename, strerror(error));
-				return -error;
+				        filename, strerror(errno));
+				return -errno;
 			}
 			if (cur_blks != sysinode->i_di.di_blocks)
 				reprocess_inode(sysinode, dirname);
@@ -897,8 +897,8 @@ int pass2(struct gfs2_sbd *sdp)
 						 DT_DIR));
 				if (error) {
 					log_err(_("Error adding directory %s: %s\n"),
-					        filename, strerror(error));
-					return -error;
+					        filename, strerror(errno));
+					return -errno;
 				}
 				if (cur_blks != ip->i_di.di_blocks) {
 					char dirname[80];
diff --git a/gfs2/fsck/pass3.c b/gfs2/fsck/pass3.c
index f0b35e2..ef4340e 100644
--- a/gfs2/fsck/pass3.c
+++ b/gfs2/fsck/pass3.c
@@ -58,7 +58,7 @@ static int attach_dotdot_to(struct gfs2_sbd *sdp, uint64_t newdotdot,
 		      (sdp->gfs1 ? GFS_FILE_DIR : DT_DIR));
 	if (err) {
 		log_err(_("Error adding directory %s: %s\n"),
-		        filename, strerror(err));
+		        filename, strerror(errno));
 		exit(FSCK_ERROR);
 	}
 	if (cur_blks != ip->i_di.di_blocks) {
diff --git a/gfs2/libgfs2/fs_ops.c b/gfs2/libgfs2/fs_ops.c
index befa25a..def6f80 100644
--- a/gfs2/libgfs2/fs_ops.c
+++ b/gfs2/libgfs2/fs_ops.c
@@ -737,6 +737,11 @@ int gfs2_dirent_next(struct gfs2_inode *dip, struct gfs2_buffer_head *bh,
 	return 0;
 }
 
+/**
+ * Allocate a gfs2 dirent
+ * Returns 0 on success, with *dent_out pointing to the new dirent,
+ * or -1 on failure, with errno set
+ */
 static int dirent_alloc(struct gfs2_inode *dip, struct gfs2_buffer_head *bh,
 			int name_len, struct gfs2_dirent **dent_out)
 {
@@ -807,7 +812,8 @@ static int dirent_alloc(struct gfs2_inode *dip, struct gfs2_buffer_head *bh,
 		}
 	} while (gfs2_dirent_next(dip, bh, &dent) == 0);
 
-	return -ENOSPC;
+	errno = ENOSPC;
+	return -1;
 }
 
 void dirent2_del(struct gfs2_inode *dip, struct gfs2_buffer_head *bh,
@@ -1395,10 +1401,8 @@ static struct gfs2_inode *__createi(struct gfs2_inode *dip,
 		inum.no_addr = bn;
 
 		err = dir_add(dip, filename, strlen(filename), &inum, IF2DT(mode));
-		if (err) {
-			errno = -err;
+		if (err)
 			return NULL;
-		}
 
 		if (if_gfs1)
 			is_dir = (IF2DT(mode) == GFS_FILE_DIR);
-- 
1.7.6.5

[Cluster-devel] [PATCH 8/8] libgfs2: Set errno in dirent_alloc and use dir_add consistently

[Bob Peterson]

----- Original Message -----
| Initially spotted by coverity: "error" is passed to a parameter that
| cannot be negative.
| 
| It turned out that dirent_alloc was returning -ENOSPC and some
| calling
| code (via dir_add) was assuming that errno was set. Other code was
| assuming that dir_add returned -errno. This patch changes
| dirent_alloc
| to set errno and return -1 and also updates the code which calls
| dir_add
| to use errno.
| 
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---

Hi Andy,

ACK to all 8. Thanks for cleaning these up.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 8/8] libgfs2: Set errno in dirent_alloc and use dir_add consistently

[Abhijith Das]

Hi,

ACK all 8 patches.

Cheers!
--Abhi

----- Original Message -----
> From: "Andrew Price" <anprice@redhat.com>
> To: cluster-devel at redhat.com
> Sent: Wednesday, January 11, 2012 12:21:22 PM
> Subject: [Cluster-devel] [PATCH 8/8] libgfs2: Set errno in dirent_alloc and	use dir_add consistently
> 
> Initially spotted by coverity: "error" is passed to a parameter that
> cannot be negative.
> 
> It turned out that dirent_alloc was returning -ENOSPC and some
> calling
> code (via dir_add) was assuming that errno was set. Other code was
> assuming that dir_add returned -errno. This patch changes
> dirent_alloc
> to set errno and return -1 and also updates the code which calls
> dir_add
> to use errno.
> 
> Signed-off-by: Andrew Price <anprice@redhat.com>