[Cluster-devel] [PATCH 1/5] fsck.gfs2: Plug memory leak in check_system_dir()

[Cluster-devel] [PATCH 1/5] fsck.gfs2: Plug memory leak in check_system_dir()

[Andrew Price]

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

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

diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index 4201bb2..c530695 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -667,6 +667,7 @@ static int check_system_dir(struct gfs2_inode *sysinode, const char *dirname,
 				    filename_len))) {
 				log_err( _("Unable to zero name string\n"));
 				stack;
+				free(filename);
 				return -1;
 			}
 			memcpy(filename, tmp_name, filename_len);
@@ -678,6 +679,7 @@ static int check_system_dir(struct gfs2_inode *sysinode, const char *dirname,
 			if (error) {
 				log_err(_("Error adding directory %s: %s\n"),
 				        filename, strerror(errno));
+				free(filename);
 				return -errno;
 			}
 			if (cur_blks != sysinode->i_di.di_blocks)
-- 
1.7.7.5

[Cluster-devel] [PATCH 2/5] fsck.gfs2: Fix null pointer deref in check_system_dir()

[Andrew Price]

Spotted by coverity: Dereferencing null variable "sysinode".

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

diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index c530695..78c9f47 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -623,10 +623,15 @@ static int check_system_dir(struct gfs2_inode *sysinode, const char *dirname,
 
 	log_info( _("Checking system directory inode '%s'\n"), dirname);
 
-	if (sysinode) {
-		iblock = sysinode->i_di.di_num.no_addr;
-		ds.q = block_type(iblock);
+	if (!sysinode) {
+		log_err( _("Failed to check '%s': sysinode is null\n"), dirname);
+		stack;
+		return -1;
 	}
+
+	iblock = sysinode->i_di.di_num.no_addr;
+	ds.q = block_type(iblock);
+
 	pass2_fxns.private = (void *) &ds;
 	if (ds.q == gfs2_bad_block) {
 		/* First check that the directory's metatree is valid */
-- 
1.7.7.5

[Cluster-devel] [PATCH 3/5] fsck.gfs2: Plug a leak in find_block_ref()

[Andrew Price]

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

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

diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index ae6d45c..9e6f376 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -344,6 +344,7 @@ static int find_block_ref(struct gfs2_sbd *sdp, uint64_t inode)
 		log_debug( _("Block %lld (0x%llx) is not gfs2 metadata.\n"),
 			     (unsigned long long)inode,
 			     (unsigned long long)inode);
+		fsck_inode_put(&ip);
 		return 1;
 	}
 	/* Check to see if this inode was referenced by another by mistake */
-- 
1.7.7.5

[Cluster-devel] [PATCH 4/5] fsck.gfs2: Remove unused hash.c, hash.h

[Andrew Price]

The functions in hash.c aren't being used so it can be removed.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/Makefile.am  |    4 +-
 gfs2/fsck/hash.c       |   93 ------------------------------------------------
 gfs2/fsck/hash.h       |    7 ----
 gfs2/fsck/inode_hash.c |    1 -
 gfs2/fsck/metawalk.c   |    1 -
 5 files changed, 2 insertions(+), 104 deletions(-)
 delete mode 100644 gfs2/fsck/hash.c
 delete mode 100644 gfs2/fsck/hash.h

diff --git a/gfs2/fsck/Makefile.am b/gfs2/fsck/Makefile.am
index e6a621f..261f6b1 100644
--- a/gfs2/fsck/Makefile.am
+++ b/gfs2/fsck/Makefile.am
@@ -10,10 +10,10 @@ sbindir := $(shell rpl=0; test '$(exec_prefix):$(sbindir)' = /usr:/usr/sbin \
 
 sbin_PROGRAMS		= fsck.gfs2
 
-noinst_HEADERS		= eattr.h fs_bits.h fsck.h fs_recovery.h hash.h \
+noinst_HEADERS		= eattr.h fs_bits.h fsck.h fs_recovery.h \
 			  inode_hash.h link.h lost_n_found.h metawalk.h util.h
 
-fsck_gfs2_SOURCES	= eattr.c fs_recovery.c hash.c initialize.c \
+fsck_gfs2_SOURCES	= eattr.c fs_recovery.c initialize.c \
 			  inode_hash.c link.c lost_n_found.c main.c metawalk.c \
 			  pass1b.c pass1.c pass1c.c pass2.c pass3.c pass4.c \
 			  pass5.c rgrepair.c util.c
diff --git a/gfs2/fsck/hash.c b/gfs2/fsck/hash.c
deleted file mode 100644
index f55c2a7..0000000
--- a/gfs2/fsck/hash.c
+++ /dev/null
@@ -1,93 +0,0 @@
-#include "clusterautoconfig.h"
-
-/* This is the same hash algorithm used by the glocks in gfs */
-
-#include <stdint.h>
-#include <unistd.h>
-#include "libgfs2.h"
-#include "hash.h"
-#include "osi_list.h"
-
-/**
- * hash_more_internal - hash an array of data
- * @data: the data to be hashed
- * @len: the length of data to be hashed
- * @hash: the hash from a previous call
- *
- * Take some data and convert it to a 32-bit hash.
- *
- * This is the 32-bit FNV-1a hash from:
- * http://www.isthe.com/chongo/tech/comp/fnv/
- *
- * Hash guts
- *
- * Returns: the hash
- */
-
-static __inline__ uint32_t
-hash_more_internal(const void *data, unsigned int len, uint32_t hash)
-{
-	unsigned char *p = (unsigned char *) data;
-	unsigned char *e = p + len;
-	uint32_t h = hash;
-
-	while (p < e) {
-		h ^= (uint32_t) (*p++);
-		h *= 0x01000193;
-	}
-
-	return h;
-}
-
-/**
- * fsck_hash - hash an array of data
- * @data: the data to be hashed
- * @len: the length of data to be hashed
- *
- * Take some data and convert it to a 32-bit hash.
- *
- * This is the 32-bit FNV-1a hash from:
- * http://www.isthe.com/chongo/tech/comp/fnv/
- *
- * Returns: the hash
- */
-
-uint32_t
-fsck_hash(const void *data, unsigned int len)
-{
-	uint32_t h = 0x811C9DC5;
-	h = hash_more_internal(data, len, h);
-	return h;
-}
-
-/**
- * fsck_hash_more - hash an array of data
- * @data: the data to be hashed
- * @len: the length of data to be hashed
- * @hash: the hash from a previous call
- *
- * Take some data and convert it to a 32-bit hash.
- *
- * This is the 32-bit FNV-1a hash from:
- * http://www.isthe.com/chongo/tech/comp/fnv/
- *
- * This version let's you hash together discontinuous regions.
- * For example, to compute the combined hash of the memory in
- * (data1, len1), (data2, len2), and (data3, len3) you:
- *
- *   h = fsck_hash(data1, len1);
- *   h = fsck_hash_more(data2, len2, h);
- *   h = fsck_hash_more(data3, len3, h);
- *
- * Returns: the hash
- */
-
-uint32_t
-fsck_hash_more(const void *data, unsigned int len, uint32_t hash)
-{
-	uint32_t h;
-	h = hash_more_internal(data, len, hash);
-	return h;
-}
-
-
diff --git a/gfs2/fsck/hash.h b/gfs2/fsck/hash.h
deleted file mode 100644
index d5fe8e9..0000000
--- a/gfs2/fsck/hash.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#ifndef _HASH_H
-#define _HASH_H
-
-uint32_t fsck_hash(const void *data, unsigned int len);
-uint32_t fsck_hash_more(const void *data, unsigned int len, uint32_t hash);
-
-#endif				/* _HASH_H  */
diff --git a/gfs2/fsck/inode_hash.c b/gfs2/fsck/inode_hash.c
index 731c9c6..e2d3c29 100644
--- a/gfs2/fsck/inode_hash.c
+++ b/gfs2/fsck/inode_hash.c
@@ -7,7 +7,6 @@
 
 #include "libgfs2.h"
 #include "osi_list.h"
-#include "hash.h"
 #include "inode_hash.h"
 #include "fsck.h"
 #define _(String) gettext(String)
diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index eb80ccf..fc87d2f 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -16,7 +16,6 @@
 #include "fsck.h"
 #include "util.h"
 #include "metawalk.h"
-#include "hash.h"
 #include "inode_hash.h"
 
 #define COMFORTABLE_BLKS 5242880 /* 20GB in 4K blocks */
-- 
1.7.7.5

[Cluster-devel] [PATCH 5/5] mkfs.gfs2: Improve error messages

[Andrew Price]

Currently mkfs.gfs2 does not warn that the lock table has been omitted,
it just complains about a missing colon in the lock table. It also
prefixes error messages with the absolute path of the source file. This
patch removes the path from error messages (in mkfs and edit) and adds a
check for an empty lock table.

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

diff --git a/gfs2/edit/hexedit.h b/gfs2/edit/hexedit.h
index b05b5c0..dd76810 100644
--- a/gfs2/edit/hexedit.h
+++ b/gfs2/edit/hexedit.h
@@ -129,7 +129,6 @@ static __inline__ __attribute__((noreturn, format (printf, 1, 2)))
 void die(const char *fmt, ...)
 {
 	va_list ap;
-	fprintf(stderr, "%s: ", __FILE__);
 	va_start(ap, fmt);
 	vfprintf(stderr, fmt, ap);
 	va_end(ap);
diff --git a/gfs2/mkfs/gfs2_mkfs.h b/gfs2/mkfs/gfs2_mkfs.h
index d7d7da7..231f370 100644
--- a/gfs2/mkfs/gfs2_mkfs.h
+++ b/gfs2/mkfs/gfs2_mkfs.h
@@ -24,7 +24,6 @@ static __inline__ __attribute__((noreturn, format (printf, 1, 2)))
 void die(const char *fmt, ...)
 {
 	va_list ap;
-	fprintf(stderr, "%s: ", __FILE__);
 	va_start(ap, fmt);
 	vfprintf(stderr, fmt, ap);
 	va_end(ap);
diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index 3658fd4..7c93621 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -257,6 +257,10 @@ static void test_locking(char *lockproto, char *locktable)
 		/*  Nolock is always ok.  */
 	} else if (strcmp(lockproto, "lock_gulm") == 0 ||
 		   strcmp(lockproto, "lock_dlm") == 0) {
+		if (locktable == NULL || *locktable == '\0') {
+			fprintf(stderr, _("No lock table specified.\n"));
+			exit(-1);
+		}
 		for (c = locktable; *c; c++) {
 			if (isspace(*c))
 				die( _("locktable error: contains space characters\n"));
-- 
1.7.7.5

[Cluster-devel] [PATCH 5/5] mkfs.gfs2: Improve error messages

[Steven Whitehouse]

Hi,

Those all look good to me. Thanks,

Steve.

On Wed, 2012-01-18 at 16:39 +0000, Andrew Price wrote:
> Currently mkfs.gfs2 does not warn that the lock table has been omitted,
> it just complains about a missing colon in the lock table. It also
> prefixes error messages with the absolute path of the source file. This
> patch removes the path from error messages (in mkfs and edit) and adds a
> check for an empty lock table.
> 
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  gfs2/edit/hexedit.h   |    1 -
>  gfs2/mkfs/gfs2_mkfs.h |    1 -
>  gfs2/mkfs/main_mkfs.c |    4 ++++
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/gfs2/edit/hexedit.h b/gfs2/edit/hexedit.h
> index b05b5c0..dd76810 100644
> --- a/gfs2/edit/hexedit.h
> +++ b/gfs2/edit/hexedit.h
> @@ -129,7 +129,6 @@ static __inline__ __attribute__((noreturn, format (printf, 1, 2)))
>  void die(const char *fmt, ...)
>  {
>  	va_list ap;
> -	fprintf(stderr, "%s: ", __FILE__);
>  	va_start(ap, fmt);
>  	vfprintf(stderr, fmt, ap);
>  	va_end(ap);
> diff --git a/gfs2/mkfs/gfs2_mkfs.h b/gfs2/mkfs/gfs2_mkfs.h
> index d7d7da7..231f370 100644
> --- a/gfs2/mkfs/gfs2_mkfs.h
> +++ b/gfs2/mkfs/gfs2_mkfs.h
> @@ -24,7 +24,6 @@ static __inline__ __attribute__((noreturn, format (printf, 1, 2)))
>  void die(const char *fmt, ...)
>  {
>  	va_list ap;
> -	fprintf(stderr, "%s: ", __FILE__);
>  	va_start(ap, fmt);
>  	vfprintf(stderr, fmt, ap);
>  	va_end(ap);
> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
> index 3658fd4..7c93621 100644
> --- a/gfs2/mkfs/main_mkfs.c
> +++ b/gfs2/mkfs/main_mkfs.c
> @@ -257,6 +257,10 @@ static void test_locking(char *lockproto, char *locktable)
>  		/*  Nolock is always ok.  */
>  	} else if (strcmp(lockproto, "lock_gulm") == 0 ||
>  		   strcmp(lockproto, "lock_dlm") == 0) {
> +		if (locktable == NULL || *locktable == '\0') {
> +			fprintf(stderr, _("No lock table specified.\n"));
> +			exit(-1);
> +		}
>  		for (c = locktable; *c; c++) {
>  			if (isspace(*c))
>  				die( _("locktable error: contains space characters\n"));