[Cluster-devel] [PATCH 1/2] mkfs.gfs2: Avoid a rename race when checking file contents

[Cluster-devel] [PATCH 1/2] mkfs.gfs2: Avoid a rename race when checking file contents

[Andrew Price]

Currently there is a slight chance that mkfs.gfs2 could report the
incorrect contents of the target if another file is renamed into place.
This is because we pass the target name to the file command and file
opens it again. This patch tries to avoid that condition by using the
/proc/$pid/fd/$device_fd symlink, which changes when the open file is
renamed, instead.

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

diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index 957b144..2d529d7 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -426,7 +426,7 @@ static void check_dev_content(const char *devname)
 	char content[1024] = { 0, };
 	char * args[] = {
 		(char *)"/usr/bin/file",
-		(char *)"-bs",
+		(char *)"-bsL",
 		(char *)devname,
 		NULL };
 	int p[2] = {-1, -1};
@@ -557,6 +557,7 @@ void main_mkfs(int argc, char *argv[])
 	int rgsize_specified = 0;
 	unsigned char uuid[16];
 	char *absname = NULL;
+	char *fdpath = NULL;
 	int islnk = 0;
 
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
@@ -587,13 +588,19 @@ void main_mkfs(int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
+	if (asprintf(&fdpath, "/proc/%d/fd/%d", getpid(), sdp->device_fd) < 0) {
+		perror(_("Failed to build string"));
+		exit(EXIT_FAILURE);
+	}
+
 	if (!sdp->override) {
 		islnk = is_symlink(sdp->device_name, &absname);
 		printf(_("This will destroy any data on %s.\n"), islnk ? absname : sdp->device_name);
-		check_dev_content(islnk ? absname : sdp->device_name);
 		free(absname);
+		check_dev_content(fdpath);
 		are_you_sure();
 	}
+	free(fdpath);
 
 	if (sdp->bsize == -1) {
 		if (S_ISREG(sdp->dinfo.stat.st_mode))
-- 
1.7.10.4

[Cluster-devel] [PATCH 2/2] fsck.gfs2: Fix buffer overflow in get_lockproto_table

[Andrew Price]

Coverity discovered a buffer overflow in this function where an overly
long cluster name in cluster.conf could cause a crash while repairing
the superblock. This patch fixes the bug by making sure the lock table
is composed sensibly, limiting the fsname to 16 chars as documented, and
only allowing the cluster name (which doesn't seem to have a documented
max size) to use the remaining space in the locktable name string.

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

diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index f07e0b2..e56161e 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -753,12 +753,13 @@ static int init_system_inodes(struct gfs2_sbd *sdp)
 static int get_lockproto_table(struct gfs2_sbd *sdp)
 {
 	FILE *fp;
-	char line[PATH_MAX], *p, *p2;
-	char fsname[PATH_MAX];
+	char line[PATH_MAX];
+	char *cluname, *end;
+	const char *fsname, *cfgfile = "/etc/cluster/cluster.conf";
 
 	memset(sdp->lockproto, 0, sizeof(sdp->lockproto));
 	memset(sdp->locktable, 0, sizeof(sdp->locktable));
-	fp = fopen("/etc/cluster/cluster.conf", "rt");
+	fp = fopen(cfgfile, "rt");
 	if (!fp) {
 		/* no cluster.conf; must be a stand-alone file system */
 		strcpy(sdp->lockproto, "lock_nolock");
@@ -771,28 +772,29 @@ static int get_lockproto_table(struct gfs2_sbd *sdp)
 	log_warn(_("Lock protocol assumed to be: " GFS2_DEFAULT_LOCKPROTO
 		   "\n"));
 	strcpy(sdp->lockproto, GFS2_DEFAULT_LOCKPROTO);
+
 	while (fgets(line, sizeof(line) - 1, fp)) {
-		p = strstr(line,"<cluster name=");
-		if (p) {
-			p += 15;
-			p2 = strchr(p,'"');
-			strncpy(sdp->locktable, p, p2 - p);
+		cluname = strstr(line,"<cluster name=");
+		if (cluname) {
+			cluname += 15;
+			end = strchr(cluname,'"');
+			if (end)
+				*end = '\0';
 			break;
 		}
 	}
-	if (sdp->locktable[0] == '\0') {
-		log_err(_("Error: Unable to determine cluster name from "
-			  "/etc/cluster.conf\n"));
+	if (cluname == NULL || end == NULL || end - cluname < 1) {
+		log_err(_("Error: Unable to determine cluster name from %s\n"),
+		                                                      cfgfile);
 	} else {
-		memset(fsname, 0, sizeof(fsname));
-		p = strrchr(opts.device, '/');
-		if (p) {
-			p++;
-			strncpy(fsname, p, sizeof(fsname));
-		} else
-			strcpy(fsname, "repaired");
-		strcat(sdp->locktable, ":");
-		strcat(sdp->locktable, fsname);
+		fsname = strrchr(opts.device, '/');
+		if (fsname)
+			fsname++;
+		else
+			fsname = "repaired";
+		snprintf(sdp->locktable, sizeof(sdp->locktable), "%.*s:%.16s",
+		         (int)(sizeof(sdp->locktable) - strlen(fsname) - 2),
+		         cluname, fsname);
 		log_warn(_("Lock table determined to be: %s\n"),
 			 sdp->locktable);
 	}
-- 
1.7.10.4

[Cluster-devel] [PATCH 2/2] fsck.gfs2: Fix buffer overflow in get_lockproto_table

[Bob Peterson]

----- Original Message -----
| Coverity discovered a buffer overflow in this function where an
| overly
| long cluster name in cluster.conf could cause a crash while repairing
| the superblock. This patch fixes the bug by making sure the lock
| table
| is composed sensibly, limiting the fsname to 16 chars as documented,
| and
| only allowing the cluster name (which doesn't seem to have a
| documented
| max size) to use the remaining space in the locktable name string.
| 
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---

Hi,

ACK to both patches.

Regards,

Bob Peterson
Red Hat File Systems