[PVFS-developers] metadata file size checking

[Porter Don]

Greetings all,

I wanted to implement some protection against overwriting a metadata file
that has not had the migration script run on it after an upgrade.  So I
ended up stat-ing the file in meta-open and if it is not a directory and its
size is not the same as a struct fmeta, then the manager will return an
EINVAL (open to suggestions for better error code) and print a message to
logs.  See below:

diff -ur pvfs-1.6.3-pre1/mgr/meta/metaio.c
pvfs-1.6.3-pre1-mgr-check/mgr/meta/metaio.c
--- pvfs-1.6.3-pre1/mgr/meta/metaio.c	Thu Jan 15 08:59:38 2004
+++ pvfs-1.6.3-pre1-mgr-check/mgr/meta/metaio.c	Thu May  6 16:13:44 2004
@@ -75,6 +75,21 @@
 int meta_open(char *pathname, int flags)
 {
 	int fd;
+	struct stat st;
+
+	/* Check to make sure that the size is appropriate to prevent old
+		metadata from being clobbered.  This is intended to save the
+		bacon of one who forgets to run the migration script.*/
+	if(0 > stat(pathname, &st)){
+		fprintf(stderr, "Could not stat file: %s, Something is very
wrong here.  Errno = %d\n", pathname, errno);
+		return -1;
+	}
+
+	if(S_ISREG(st.st_mode) && st.st_size != sizeof(struct fmeta)){
+		fprintf(stderr, "Cowardly refusing to open %s because it is
not the size I expect a metadata file to be.\n", pathname);
+		errno = EINVAL;
+		return -1;
+	}
 
 	/* no creating files with this; use meta_creat() */
 	flags &= ~O_CREAT;
@@ -90,6 +105,7 @@
  */
 int meta_creat(char *pathname, int flags)
 {
+	static struct fmeta fm;
 	int fd, old_umask, cflags = O_RDWR | O_CREAT | O_EXCL,
 	    mode = S_IRWXU | S_IRGRP | S_IROTH;
 
@@ -101,7 +117,14 @@
 		return(-1);
 	}
 
-	/* MAYBE WRITE IN DUMMY METADATA? */
+	/* write in dummy fmeta so that meta_open will succeed.  This is ok
+	 * because only md_open calls us directly and it will overwrite
+	 * anyway
+	 */
+	if(sizeof(struct fmeta) != write(fd, &fm, sizeof(struct fmeta))){
+		return(-1);
+	}
+
 	close(fd);
 
 	/* reopen the new file using the specified flags */

Also, in the process of debugging this change, I found that the code for
md_stat could be seriously de-obfuscated with the addition of symbolic
links.  The code is much cleaner to read and a tad more efficient:

diff -ur pvfs-1.6.3-pre1/mgr/meta/md_stat.c
pvfs-1.6.3-pre1-mgr-check/mgr/meta/md_stat.c
--- pvfs-1.6.3-pre1/mgr/meta/md_stat.c	Tue Feb 10 13:47:28 2004
+++ pvfs-1.6.3-pre1-mgr-check/mgr/meta/md_stat.c	Thu May  6 15:16:38
2004
@@ -80,66 +80,45 @@
 	 	COPY_STAT_TO_PSTAT(&metar_p->u_stat, &statbuf);
 		metar_p->u_stat.st_mode = S_IFLNK | (S_IRWXO | S_IRWXG |
S_IRWXU);
 		return(0);
-	}
 
-	/* Open metadata file */	
-	if ((fd = meta_open(name, O_RDONLY)) < 0) {
-		if (errno != ENOENT) perror("md_stat, meta_open");
-		return (-1);
-	}
+	} else if(S_ISDIR(statbuf.st_mode)){
 
-	/* Read metadata file - read is done before access to determine if
its a
-	 * directory or not (if its directory we need to do execute
permission
-	 * checking on the entire directory and not just the parent)  */

-	if (meta_read(fd, &meta1) < 0) {
-		if (errno != EISDIR) {
-			perror("md_stat: meta_read");
-			meta_close(fd); /* try to close */
-			return(-1);
-		}
-		else /* directory - just return the normal stats */ {
-			/* check execute permission on parent dir */
-			strncpy(temp, name, MAXPATHLEN);
-			length = get_parent(temp); /* strip off filename */
-			if (length >= 0) {
-				if (meta_access(0, temp, request->uid,
request->gid, X_OK) < 0)
+		/* directory - just return the normal stats */ 
+		/* check execute permission on parent dir */
+		strncpy(temp, name, MAXPATHLEN);
+		length = get_parent(temp); /* strip off filename */
+		if (length >= 0) {
+			if (meta_access(0, temp, request->uid, request->gid,
X_OK) < 0)
 				{
-					meta_close(fd);
 					return(-1);
 				}
-			}
-			/* else CWD is being used and directory permissions
are not
-			 * checked */
-
-			/* get stats and fill in pvfs specific stuff */
-			{
-				struct stat sb;
-				if (fstat(fd, &sb) < 0) {
-					perror("md_stat: fstat");
-					meta_close(fd); /* try to close */
-					return(-1);
-				}
-				COPY_STAT_TO_PSTAT(&metar_p->u_stat, &sb);
-			}
-			/* Close metadata file */
-			if (meta_close(fd) < 0) {
-				perror("md_stat: meta_close");
-				return(-1);
-			}	
-			if (get_dmeta(name, &dir) < 0) {
-				perror("md_stat: get_dmeta");
-				return(-1);
-			}
-			metar_p->u_stat.st_mode =
-				S_IFDIR | ((S_IRWXO | S_IRWXG | S_IRWXU |
S_ISVTX | S_ISGID) & dir.dr_mode);
-
-			metar_p->u_stat.st_uid = dir.dr_uid;
-			metar_p->u_stat.st_gid = dir.dr_gid;
-			return(0);
 		}
+		/* else CWD is being used and directory permissions are not
+		 * checked */
+
+		if (get_dmeta(name, &dir) < 0) {
+			perror("md_stat: get_dmeta");
+			return(-1);
+		}
+
+		COPY_STAT_TO_PSTAT(&metar_p->u_stat, &statbuf);
+
+		metar_p->u_stat.st_mode =
+			S_IFDIR | ((S_IRWXO | S_IRWXG | S_IRWXU | S_ISVTX |
S_ISGID) & dir.dr_mode);
+
+		metar_p->u_stat.st_uid = dir.dr_uid;
+		metar_p->u_stat.st_gid = dir.dr_gid;
+		return(0);
+	} 
+
+	/* else regular file or followed link */
+		
+	/* Open metadata file */	
+	if ((fd = meta_open(name, O_RDONLY)) < 0) {
+		if (errno != ENOENT) perror("md_stat, meta_open");
+		return (-1);
 	}
 
-	/* ok, we know its a file */
 	/* check execute permission on parent directory */
 	strncpy(temp, name, MAXPATHLEN);
 	length = get_parent(temp); /* strip off filename */
@@ -155,6 +134,13 @@
 		}
 	}
 
+	/* Read metadata file */
+	if (meta_read(fd, &meta1) < 0) {
+		perror("md_stat: meta_read");
+		meta_close(fd); /* try to close */
+		return(-1);
+	}
+
 	/* Close metadata file */
 	if (meta_close(fd) < 0) {
 		perror("md_stat: meta_close");


Thanks and let me know what you all think,
don