[PVFS-developers] metadata file size checking
Rob Ross
rross at mcs.anl.gov
Fri May 14 16:35:45 EDT 2004
Hi Don,
Could you resend these patches to me? They got word-wrapped in here...
Thanks,
Rob
On Thu, 6 May 2004, Porter Don wrote:
> 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
> _______________________________________________
> PVFS-developers mailing list
> PVFS-developers at www.beowulf-underground.org
> http://www.beowulf-underground.org/mailman/listinfo/pvfs-developers
>
>
More information about the PVFS-developers
mailing list