[PATCH] Avoid leftover temporary mount points when using -P (mfs)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Avoid leftover temporary mount points when using -P (mfs)

Rafael Neves-2
Hi,

Submitting to tech@ to broader audience.

When using -P option in mfs with a directory or a block device that
doen't exist, for example when the device roams, newfs(2) leaves
leftovers of temporary mount points.

With my /etc/fstab:
        ca7552589896b01e.b none swap sw
        ca7552589896b01e.a / ffs rw 1 1
        ca7552589896b01e.k /home ffs rw,nodev,nosuid 1 2
        #ca7552589896b01e.d /tmp ffs rw,nodev,nosuid 1 2
        swap /tmp mfs rw,nodev,nosuid,-s=300M 0 0
        ca7552589896b01e.f /usr ffs rw,nodev 1 2
        ca7552589896b01e.g /usr/X11R6 ffs rw,nodev 1 2
        ca7552589896b01e.h /usr/local ffs rw,nodev,wxallowed 1 2
        ca7552589896b01e.j /usr/build ffs rw,noperm,noauto 1 2
        swap /usr/obj mfs rw,nodev,nosuid,noauto,-s=4G,-P=/foo/bar  0 0
        ca7552589896b01e.i /usr/src ffs rw,nodev,nosuid 1 2
        ca7552589896b01e.e /var ffs rw,nodev,nosuid 1 2

Result when trying to mount /usr/obj:
        root@orus [rneves]# mount /usr/obj
        mount_mfs: cannot stat /foo/bar: No such file or directory
        root@orus [rneves]# mount
        /dev/sd2a on / type ffs (local)
        /dev/sd2k on /home type ffs (local, nodev, nosuid)
        mfs:28249 on /tmp type mfs (asynchronous, local, nodev, nosuid, size=614400 512-blocks)
        /dev/sd2f on /usr type ffs (local, nodev)
        /dev/sd2g on /usr/X11R6 type ffs (local, nodev)
        /dev/sd2h on /usr/local type ffs (local, nodev, wxallowed)
        /dev/sd2i on /usr/src type ffs (local, nodev, nosuid)
        /dev/sd2e on /var type ffs (local, nodev, nosuid)
        mfs:44634 on /tmp/mntoMG6WmZTT7 type mfs (asynchronous, local, nodev, nosuid, size=8388608 512-blocks)


Tracking down the issue I found that:
        + When -P is specified, pop != NULL.
        + After fork, waitformount() is called. It creates the temporary
          places to store the data.
        + copy() is called, and it it fails the following umount() and
          rmdir() is not called, leaving the temporary mounts.

As copy() can fail in a couple of ways, I thought about the following change:
        + Make all errors a warning, but after then return to the first
          caller indicating an error. Getting the erro the clean up is
          done, and exit(1).

        + Make copy() return an int: -1 in fail, and 0 otherwise.
        + isdir(), gettmpmnt(): Convert errors to warnings + return(-1).

There is a change of messages printing if you don't have a /tmp
is read-only, first the error of cannot fstat, and after
"Cannot create tmp mountpoint for -P".

There still a chance to get a inconsistent state: if there is no
/bin/pax, or errors in do_exec(). But erros in do_exec seem very
critical, the same with a missing /bin/pax. So I didn't change them.

Otto@ said that another alternative is using atexit(3), but we
pointed out what it is very difficult to get it right, and almost
always has race conditions. Given that and what manpage says I have no
hope that I can get it right.

What do you think?

Note that the check in line 519 (newfs.c) was changed to add the new
possible return value. Actually, currently it is not allowed -P with a
read-only /tmp, because in this case gettmpmnt() returns 0, and by this
early check newfs(2) throws an error. Actually, gettmpmnt() must changed
to it work properly. The `created` if uses a <= by symmetry.

But this is a differente issue, that I think could be changed in a
separated diff.

Regards,
Rafael Neves


Patch:


Index: newfs.c
===================================================================
RCS file: /cvs/src/sbin/newfs/newfs.c,v
retrieving revision 1.112
diff -u -p -r1.112 newfs.c
--- newfs.c 28 Jun 2019 13:32:45 -0000 1.112
+++ newfs.c 17 Aug 2019 14:27:46 -0000
@@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
 static void waitformount(char *, pid_t);
 static int do_exec(const char *, const char *, char *const[]);
 static int isdir(const char *);
-static void copy(char *, char *);
+static int copy(char *, char *);
 static int gettmpmnt(char *, size_t);
 #endif
 
@@ -179,6 +179,7 @@ main(int argc, char *argv[])
 #ifdef MFS
  char mountfromname[BUFSIZ];
  char *pop = NULL, node[PATH_MAX];
+ int ret;
  pid_t pid;
  struct stat mountpoint;
 #endif
@@ -516,7 +517,7 @@ havelabel:
  struct mfs_args args;
  char tmpnode[PATH_MAX];
 
- if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
+ if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
  errx(1, "Cannot create tmp mountpoint for -P");
  memset(&args, 0, sizeof(args));
  args.base = membase;
@@ -537,9 +538,11 @@ havelabel:
  default:
  if (pop != NULL) {
  waitformount(tmpnode, pid);
- copy(pop, tmpnode);
+ ret = copy(pop, tmpnode);
  unmount(tmpnode, 0);
  rmdir(tmpnode);
+ if (ret == -1)
+ exit(1);
  }
  waitformount(node, pid);
  exit(0);
@@ -740,14 +743,18 @@ isdir(const char *path)
 {
  struct stat st;
 
- if (stat(path, &st) != 0)
- err(1, "cannot stat %s", path);
- if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
- errx(1, "%s: not a dir or a block device", path);
+ if (stat(path, &st) != 0) {
+ warn("cannot stat %s", path);
+ return (-1);
+ }
+ if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
+ warn("%s: not a dir or a block device", path);
+ return  (-1);
+ }
  return (S_ISDIR(st.st_mode));
 }
 
-static void
+static int
 copy(char *src, char *dst)
 {
  int ret, dir, created = 0;
@@ -755,11 +762,16 @@ copy(char *src, char *dst)
  char mountpoint[MNAMELEN];
  char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
 
- dir = isdir(src);
+ if ((dir = isdir(src)) == -1)
+ return (-1);
+
  if (dir)
  strlcpy(mountpoint, src, sizeof(mountpoint));
  else {
  created = gettmpmnt(mountpoint, sizeof(mountpoint));
+ if (created <= 0)
+ return (-1);
+
  memset(&mount_args, 0, sizeof(mount_args));
  mount_args.fspec = src;
  ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
@@ -769,7 +781,8 @@ copy(char *src, char *dst)
  warn("rmdir %s", mountpoint);
  if (unmount(dst, 0) != 0)
  warn("unmount %s", dst);
- errc(1, saved_errno, "mount %s %s", src, mountpoint);
+ warnc(saved_errno, "mount %s %s", src, mountpoint);
+ return (-1);
  }
  }
  ret = do_exec(mountpoint, "/bin/pax", argv);
@@ -780,8 +793,10 @@ copy(char *src, char *dst)
  if (ret != 0) {
  if (unmount(dst, 0) != 0)
  warn("unmount %s", dst);
- errx(1, "copy %s to %s failed", mountpoint, dst);
+ warnx("copy %s to %s failed", mountpoint, dst);
+ return (-1);
  }
+ return (0);
 }
 
 static int
@@ -792,27 +807,42 @@ gettmpmnt(char *mountpoint, size_t len)
  struct statfs fs;
  size_t n;
 
- if (statfs(tmp, &fs) != 0)
- err(1, "statfs %s", tmp);
+ if (statfs(tmp, &fs) != 0) {
+ warn("statfs %s", tmp);
+ return (-1);
+ }
+
  if (fs.f_flags & MNT_RDONLY) {
- if (statfs(mnt, &fs) != 0)
- err(1, "statfs %s", mnt);
- if (strcmp(fs.f_mntonname, "/") != 0)
- errx(1, "tmp mountpoint %s busy", mnt);
- if (strlcpy(mountpoint, mnt, len) >= len)
- errx(1, "tmp mountpoint %s too long", mnt);
+ if (statfs(mnt, &fs) != 0) {
+ warn("statfs %s", mnt);
+ return (-1);
+ }
+ if (strcmp(fs.f_mntonname, "/") != 0) {
+ warnx("tmp mountpoint %s busy", mnt);
+ return (-1);
+ }
+ if (strlcpy(mountpoint, mnt, len) >= len) {
+ warnx("tmp mountpoint %s too long", mnt);
+ return (-1);
+ }
  return (0);
  }
  n = strlcpy(mountpoint, tmp, len);
- if (n >= len)
- errx(1, "tmp mount point too long");
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
  if (mountpoint[n - 1] != '/')
  strlcat(mountpoint, "/", len);
  n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
- if (n >= len)
- errx(1, "tmp mount point too long");
- if (mkdtemp(mountpoint) == NULL)
- err(1, "mkdtemp %s", mountpoint);
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
+ if (mkdtemp(mountpoint) == NULL) {
+ warn("mkdtemp %s", mountpoint);
+ return (-1);
+ }
  return (1);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Avoid leftover temporary mount points when using -P (mfs)

Otto Moerbeek
On Sat, Aug 17, 2019 at 12:13:50PM -0300, Rafael Neves wrote:

> Hi,
>
> Submitting to tech@ to broader audience.
>
> When using -P option in mfs with a directory or a block device that
> doen't exist, for example when the device roams, newfs(2) leaves
> leftovers of temporary mount points.
>
> With my /etc/fstab:
> ca7552589896b01e.b none swap sw
> ca7552589896b01e.a / ffs rw 1 1
> ca7552589896b01e.k /home ffs rw,nodev,nosuid 1 2
> #ca7552589896b01e.d /tmp ffs rw,nodev,nosuid 1 2
> swap /tmp mfs rw,nodev,nosuid,-s=300M 0 0
> ca7552589896b01e.f /usr ffs rw,nodev 1 2
> ca7552589896b01e.g /usr/X11R6 ffs rw,nodev 1 2
> ca7552589896b01e.h /usr/local ffs rw,nodev,wxallowed 1 2
> ca7552589896b01e.j /usr/build ffs rw,noperm,noauto 1 2
> swap /usr/obj mfs rw,nodev,nosuid,noauto,-s=4G,-P=/foo/bar  0 0
> ca7552589896b01e.i /usr/src ffs rw,nodev,nosuid 1 2
> ca7552589896b01e.e /var ffs rw,nodev,nosuid 1 2
>
> Result when trying to mount /usr/obj:
> root@orus [rneves]# mount /usr/obj
> mount_mfs: cannot stat /foo/bar: No such file or directory
> root@orus [rneves]# mount
> /dev/sd2a on / type ffs (local)
> /dev/sd2k on /home type ffs (local, nodev, nosuid)
> mfs:28249 on /tmp type mfs (asynchronous, local, nodev, nosuid, size=614400 512-blocks)
> /dev/sd2f on /usr type ffs (local, nodev)
> /dev/sd2g on /usr/X11R6 type ffs (local, nodev)
> /dev/sd2h on /usr/local type ffs (local, nodev, wxallowed)
> /dev/sd2i on /usr/src type ffs (local, nodev, nosuid)
> /dev/sd2e on /var type ffs (local, nodev, nosuid)
> mfs:44634 on /tmp/mntoMG6WmZTT7 type mfs (asynchronous, local, nodev, nosuid, size=8388608 512-blocks)
>
>
> Tracking down the issue I found that:
> + When -P is specified, pop != NULL.
> + After fork, waitformount() is called. It creates the temporary
>  places to store the data.
> + copy() is called, and it it fails the following umount() and
>  rmdir() is not called, leaving the temporary mounts.
>
> As copy() can fail in a couple of ways, I thought about the following change:
> + Make all errors a warning, but after then return to the first
>           caller indicating an error. Getting the erro the clean up is
>  done, and exit(1).
>
> + Make copy() return an int: -1 in fail, and 0 otherwise.
> + isdir(), gettmpmnt(): Convert errors to warnings + return(-1).
>
> There is a change of messages printing if you don't have a /tmp
> is read-only, first the error of cannot fstat, and after
> "Cannot create tmp mountpoint for -P".
>
> There still a chance to get a inconsistent state: if there is no
> /bin/pax, or errors in do_exec(). But erros in do_exec seem very
> critical, the same with a missing /bin/pax. So I didn't change them.
>
> Otto@ said that another alternative is using atexit(3), but we
> pointed out what it is very difficult to get it right, and almost
> always has race conditions. Given that and what manpage says I have no
> hope that I can get it right.
>
> What do you think?
>
> Note that the check in line 519 (newfs.c) was changed to add the new
> possible return value. Actually, currently it is not allowed -P with a
> read-only /tmp, because in this case gettmpmnt() returns 0, and by this
> early check newfs(2) throws an error. Actually, gettmpmnt() must changed
> to it work properly. The `created` if uses a <= by symmetry.
>
> But this is a differente issue, that I think could be changed in a
> separated diff.
>
> Regards,
> Rafael Neves
>
>
> Patch:
>
>
> Index: newfs.c
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/newfs.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 newfs.c
> --- newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> +++ newfs.c 17 Aug 2019 14:27:46 -0000
> @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
>  static void waitformount(char *, pid_t);
>  static int do_exec(const char *, const char *, char *const[]);
>  static int isdir(const char *);
> -static void copy(char *, char *);
> +static int copy(char *, char *);
>  static int gettmpmnt(char *, size_t);
>  #endif
>  
> @@ -179,6 +179,7 @@ main(int argc, char *argv[])
>  #ifdef MFS
>   char mountfromname[BUFSIZ];
>   char *pop = NULL, node[PATH_MAX];
> + int ret;

Please move this one inside the scope where it is used, see below

>   pid_t pid;
>   struct stat mountpoint;
>  #endif
> @@ -516,7 +517,7 @@ havelabel:
>   struct mfs_args args;
>   char tmpnode[PATH_MAX];

here

>  
> - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
>   errx(1, "Cannot create tmp mountpoint for -P");
>   memset(&args, 0, sizeof(args));
>   args.base = membase;
> @@ -537,9 +538,11 @@ havelabel:
>   default:
>   if (pop != NULL) {
>   waitformount(tmpnode, pid);
> - copy(pop, tmpnode);
> + ret = copy(pop, tmpnode);
>   unmount(tmpnode, 0);
>   rmdir(tmpnode);
> + if (ret == -1)
> + exit(1);

>   }
>   waitformount(node, pid);

It might be an idea to undo the mount an return 1 if ret is != 0.
For that to work ret should be initialized to 0.

        -Otto


>   exit(0);
> @@ -740,14 +743,18 @@ isdir(const char *path)
>  {
>   struct stat st;
>  
> - if (stat(path, &st) != 0)
> - err(1, "cannot stat %s", path);
> - if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
> - errx(1, "%s: not a dir or a block device", path);
> + if (stat(path, &st) != 0) {
> + warn("cannot stat %s", path);
> + return (-1);
> + }
> + if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
> + warn("%s: not a dir or a block device", path);
> + return  (-1);
> + }
>   return (S_ISDIR(st.st_mode));
>  }
>  
> -static void
> +static int
>  copy(char *src, char *dst)
>  {
>   int ret, dir, created = 0;
> @@ -755,11 +762,16 @@ copy(char *src, char *dst)
>   char mountpoint[MNAMELEN];
>   char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
>  
> - dir = isdir(src);
> + if ((dir = isdir(src)) == -1)
> + return (-1);
> +
>   if (dir)
>   strlcpy(mountpoint, src, sizeof(mountpoint));
>   else {
>   created = gettmpmnt(mountpoint, sizeof(mountpoint));
> + if (created <= 0)
> + return (-1);
> +
>   memset(&mount_args, 0, sizeof(mount_args));
>   mount_args.fspec = src;
>   ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
> @@ -769,7 +781,8 @@ copy(char *src, char *dst)
>   warn("rmdir %s", mountpoint);
>   if (unmount(dst, 0) != 0)
>   warn("unmount %s", dst);
> - errc(1, saved_errno, "mount %s %s", src, mountpoint);
> + warnc(saved_errno, "mount %s %s", src, mountpoint);
> + return (-1);
>   }
>   }
>   ret = do_exec(mountpoint, "/bin/pax", argv);
> @@ -780,8 +793,10 @@ copy(char *src, char *dst)
>   if (ret != 0) {
>   if (unmount(dst, 0) != 0)
>   warn("unmount %s", dst);
> - errx(1, "copy %s to %s failed", mountpoint, dst);
> + warnx("copy %s to %s failed", mountpoint, dst);
> + return (-1);
>   }
> + return (0);
>  }
>  
>  static int
> @@ -792,27 +807,42 @@ gettmpmnt(char *mountpoint, size_t len)
>   struct statfs fs;
>   size_t n;
>  
> - if (statfs(tmp, &fs) != 0)
> - err(1, "statfs %s", tmp);
> + if (statfs(tmp, &fs) != 0) {
> + warn("statfs %s", tmp);
> + return (-1);
> + }
> +
>   if (fs.f_flags & MNT_RDONLY) {
> - if (statfs(mnt, &fs) != 0)
> - err(1, "statfs %s", mnt);
> - if (strcmp(fs.f_mntonname, "/") != 0)
> - errx(1, "tmp mountpoint %s busy", mnt);
> - if (strlcpy(mountpoint, mnt, len) >= len)
> - errx(1, "tmp mountpoint %s too long", mnt);
> + if (statfs(mnt, &fs) != 0) {
> + warn("statfs %s", mnt);
> + return (-1);
> + }
> + if (strcmp(fs.f_mntonname, "/") != 0) {
> + warnx("tmp mountpoint %s busy", mnt);
> + return (-1);
> + }
> + if (strlcpy(mountpoint, mnt, len) >= len) {
> + warnx("tmp mountpoint %s too long", mnt);
> + return (-1);
> + }
>   return (0);
>   }
>   n = strlcpy(mountpoint, tmp, len);
> - if (n >= len)
> - errx(1, "tmp mount point too long");
> + if (n >= len) {
> + warnx("tmp mount point too long");
> + return (-1);
> + }
>   if (mountpoint[n - 1] != '/')
>   strlcat(mountpoint, "/", len);
>   n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
> - if (n >= len)
> - errx(1, "tmp mount point too long");
> - if (mkdtemp(mountpoint) == NULL)
> - err(1, "mkdtemp %s", mountpoint);
> + if (n >= len) {
> + warnx("tmp mount point too long");
> + return (-1);
> + }
> + if (mkdtemp(mountpoint) == NULL) {
> + warn("mkdtemp %s", mountpoint);
> + return (-1);
> + }
>   return (1);
>  }
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Avoid leftover temporary mount points when using -P (mfs)

Rafael Neves-2
On Wed, Aug 28, 2019 at 03:39:17PM +0200, Otto Moerbeek wrote:

> On Sat, Aug 17, 2019 at 12:13:50PM -0300, Rafael Neves wrote:
>
> > Hi,
> >
> > Submitting to tech@ to broader audience.
> >
> > When using -P option in mfs with a directory or a block device that
> > doen't exist, for example when the device roams, newfs(2) leaves
> > leftovers of temporary mount points.
> >
> > With my /etc/fstab:
> > ca7552589896b01e.b none swap sw
> > ca7552589896b01e.a / ffs rw 1 1
> > ca7552589896b01e.k /home ffs rw,nodev,nosuid 1 2
> > #ca7552589896b01e.d /tmp ffs rw,nodev,nosuid 1 2
> > swap /tmp mfs rw,nodev,nosuid,-s=300M 0 0
> > ca7552589896b01e.f /usr ffs rw,nodev 1 2
> > ca7552589896b01e.g /usr/X11R6 ffs rw,nodev 1 2
> > ca7552589896b01e.h /usr/local ffs rw,nodev,wxallowed 1 2
> > ca7552589896b01e.j /usr/build ffs rw,noperm,noauto 1 2
> > swap /usr/obj mfs rw,nodev,nosuid,noauto,-s=4G,-P=/foo/bar  0 0
> > ca7552589896b01e.i /usr/src ffs rw,nodev,nosuid 1 2
> > ca7552589896b01e.e /var ffs rw,nodev,nosuid 1 2
> >
> > Result when trying to mount /usr/obj:
> > root@orus [rneves]# mount /usr/obj
> > mount_mfs: cannot stat /foo/bar: No such file or directory
> > root@orus [rneves]# mount
> > /dev/sd2a on / type ffs (local)
> > /dev/sd2k on /home type ffs (local, nodev, nosuid)
> > mfs:28249 on /tmp type mfs (asynchronous, local, nodev, nosuid, size=614400 512-blocks)
> > /dev/sd2f on /usr type ffs (local, nodev)
> > /dev/sd2g on /usr/X11R6 type ffs (local, nodev)
> > /dev/sd2h on /usr/local type ffs (local, nodev, wxallowed)
> > /dev/sd2i on /usr/src type ffs (local, nodev, nosuid)
> > /dev/sd2e on /var type ffs (local, nodev, nosuid)
> > mfs:44634 on /tmp/mntoMG6WmZTT7 type mfs (asynchronous, local, nodev, nosuid, size=8388608 512-blocks)
> >
> >
> > Tracking down the issue I found that:
> > + When -P is specified, pop != NULL.
> > + After fork, waitformount() is called. It creates the temporary
> >  places to store the data.
> > + copy() is called, and it it fails the following umount() and
> >  rmdir() is not called, leaving the temporary mounts.
> >
> > As copy() can fail in a couple of ways, I thought about the following change:
> > + Make all errors a warning, but after then return to the first
> >           caller indicating an error. Getting the erro the clean up is
> >  done, and exit(1).
> >
> > + Make copy() return an int: -1 in fail, and 0 otherwise.
> > + isdir(), gettmpmnt(): Convert errors to warnings + return(-1).
> >
> > There is a change of messages printing if you don't have a /tmp
> > is read-only, first the error of cannot fstat, and after
> > "Cannot create tmp mountpoint for -P".
> >
> > There still a chance to get a inconsistent state: if there is no
> > /bin/pax, or errors in do_exec(). But erros in do_exec seem very
> > critical, the same with a missing /bin/pax. So I didn't change them.
> >
> > Otto@ said that another alternative is using atexit(3), but we
> > pointed out what it is very difficult to get it right, and almost
> > always has race conditions. Given that and what manpage says I have no
> > hope that I can get it right.
> >
> > What do you think?
> >
> > Note that the check in line 519 (newfs.c) was changed to add the new
> > possible return value. Actually, currently it is not allowed -P with a
> > read-only /tmp, because in this case gettmpmnt() returns 0, and by this
> > early check newfs(2) throws an error. Actually, gettmpmnt() must changed
> > to it work properly. The `created` if uses a <= by symmetry.
> >
> > But this is a differente issue, that I think could be changed in a
> > separated diff.
> >
> > Regards,
> > Rafael Neves
> >
> >
> > Patch:
> >
> >
> > Index: newfs.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/newfs/newfs.c,v
> > retrieving revision 1.112
> > diff -u -p -r1.112 newfs.c
> > --- newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> > +++ newfs.c 17 Aug 2019 14:27:46 -0000
> > @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
> >  static void waitformount(char *, pid_t);
> >  static int do_exec(const char *, const char *, char *const[]);
> >  static int isdir(const char *);
> > -static void copy(char *, char *);
> > +static int copy(char *, char *);
> >  static int gettmpmnt(char *, size_t);
> >  #endif
> >  
> > @@ -179,6 +179,7 @@ main(int argc, char *argv[])
> >  #ifdef MFS
> >   char mountfromname[BUFSIZ];
> >   char *pop = NULL, node[PATH_MAX];
> > + int ret;
>
> Please move this one inside the scope where it is used, see below
>
> >   pid_t pid;
> >   struct stat mountpoint;
> >  #endif
> > @@ -516,7 +517,7 @@ havelabel:
> >   struct mfs_args args;
> >   char tmpnode[PATH_MAX];
>
> here
>
> >  
> > - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> > + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
> >   errx(1, "Cannot create tmp mountpoint for -P");
> >   memset(&args, 0, sizeof(args));
> >   args.base = membase;
> > @@ -537,9 +538,11 @@ havelabel:
> >   default:
> >   if (pop != NULL) {
> >   waitformount(tmpnode, pid);
> > - copy(pop, tmpnode);
> > + ret = copy(pop, tmpnode);
> >   unmount(tmpnode, 0);
> >   rmdir(tmpnode);
> > + if (ret == -1)
> > + exit(1);
>
> >   }
> >   waitformount(node, pid);
>
> It might be an idea to undo the mount an return 1 if ret is != 0.
> For that to work ret should be initialized to 0.
>
> -Otto
> [snip]
>

Updated patch follows. I understood that return is through exit(3) as
it is pattern used in other parts of the code.


Index: sbin/newfs/newfs.c
===================================================================
RCS file: /cvs/src/sbin/newfs/newfs.c,v
retrieving revision 1.112
diff -u -p -r1.112 newfs.c
--- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112
+++ sbin/newfs/newfs.c 1 Sep 2019 18:52:26 -0000
@@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
 static void waitformount(char *, pid_t);
 static int do_exec(const char *, const char *, char *const[]);
 static int isdir(const char *);
-static void copy(char *, char *);
+static int copy(char *, char *);
 static int gettmpmnt(char *, size_t);
 #endif
 
@@ -515,8 +515,9 @@ havelabel:
  if (mfs) {
  struct mfs_args args;
  char tmpnode[PATH_MAX];
+ int ret = 0;
 
- if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
+ if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
  errx(1, "Cannot create tmp mountpoint for -P");
  memset(&args, 0, sizeof(args));
  args.base = membase;
@@ -537,9 +538,11 @@ havelabel:
  default:
  if (pop != NULL) {
  waitformount(tmpnode, pid);
- copy(pop, tmpnode);
+ ret = copy(pop, tmpnode);
  unmount(tmpnode, 0);
  rmdir(tmpnode);
+ if (ret != 0)
+ exit(1);
  }
  waitformount(node, pid);
  exit(0);
@@ -740,14 +743,18 @@ isdir(const char *path)
 {
  struct stat st;
 
- if (stat(path, &st) != 0)
- err(1, "cannot stat %s", path);
- if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
- errx(1, "%s: not a dir or a block device", path);
+ if (stat(path, &st) != 0) {
+ warn("cannot stat %s", path);
+ return (-1);
+ }
+ if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
+ warn("%s: not a dir or a block device", path);
+ return  (-1);
+ }
  return (S_ISDIR(st.st_mode));
 }
 
-static void
+static int
 copy(char *src, char *dst)
 {
  int ret, dir, created = 0;
@@ -755,11 +762,16 @@ copy(char *src, char *dst)
  char mountpoint[MNAMELEN];
  char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
 
- dir = isdir(src);
+ if ((dir = isdir(src)) == -1)
+ return (-1);
+
  if (dir)
  strlcpy(mountpoint, src, sizeof(mountpoint));
  else {
  created = gettmpmnt(mountpoint, sizeof(mountpoint));
+ if (created <= 0)
+ return (-1);
+
  memset(&mount_args, 0, sizeof(mount_args));
  mount_args.fspec = src;
  ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
@@ -769,7 +781,8 @@ copy(char *src, char *dst)
  warn("rmdir %s", mountpoint);
  if (unmount(dst, 0) != 0)
  warn("unmount %s", dst);
- errc(1, saved_errno, "mount %s %s", src, mountpoint);
+ warnc(saved_errno, "mount %s %s", src, mountpoint);
+ return (-1);
  }
  }
  ret = do_exec(mountpoint, "/bin/pax", argv);
@@ -780,8 +793,10 @@ copy(char *src, char *dst)
  if (ret != 0) {
  if (unmount(dst, 0) != 0)
  warn("unmount %s", dst);
- errx(1, "copy %s to %s failed", mountpoint, dst);
+ warnx("copy %s to %s failed", mountpoint, dst);
+ return (-1);
  }
+ return (0);
 }
 
 static int
@@ -792,27 +807,42 @@ gettmpmnt(char *mountpoint, size_t len)
  struct statfs fs;
  size_t n;
 
- if (statfs(tmp, &fs) != 0)
- err(1, "statfs %s", tmp);
+ if (statfs(tmp, &fs) != 0) {
+ warn("statfs %s", tmp);
+ return (-1);
+ }
+
  if (fs.f_flags & MNT_RDONLY) {
- if (statfs(mnt, &fs) != 0)
- err(1, "statfs %s", mnt);
- if (strcmp(fs.f_mntonname, "/") != 0)
- errx(1, "tmp mountpoint %s busy", mnt);
- if (strlcpy(mountpoint, mnt, len) >= len)
- errx(1, "tmp mountpoint %s too long", mnt);
+ if (statfs(mnt, &fs) != 0) {
+ warn("statfs %s", mnt);
+ return (-1);
+ }
+ if (strcmp(fs.f_mntonname, "/") != 0) {
+ warnx("tmp mountpoint %s busy", mnt);
+ return (-1);
+ }
+ if (strlcpy(mountpoint, mnt, len) >= len) {
+ warnx("tmp mountpoint %s too long", mnt);
+ return (-1);
+ }
  return (0);
  }
  n = strlcpy(mountpoint, tmp, len);
- if (n >= len)
- errx(1, "tmp mount point too long");
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
  if (mountpoint[n - 1] != '/')
  strlcat(mountpoint, "/", len);
  n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
- if (n >= len)
- errx(1, "tmp mount point too long");
- if (mkdtemp(mountpoint) == NULL)
- err(1, "mkdtemp %s", mountpoint);
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
+ if (mkdtemp(mountpoint) == NULL) {
+ warn("mkdtemp %s", mountpoint);
+ return (-1);
+ }
  return (1);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Avoid leftover temporary mount points when using -P (mfs)

Otto Moerbeek
On Sun, Sep 01, 2019 at 05:01:55PM -0300, Rafael Neves wrote:

> On Wed, Aug 28, 2019 at 03:39:17PM +0200, Otto Moerbeek wrote:
> > On Sat, Aug 17, 2019 at 12:13:50PM -0300, Rafael Neves wrote:
> >
> > > Hi,
> > >
> > > Submitting to tech@ to broader audience.
> > >
> > > When using -P option in mfs with a directory or a block device that
> > > doen't exist, for example when the device roams, newfs(2) leaves
> > > leftovers of temporary mount points.
> > >
> > > With my /etc/fstab:
> > > ca7552589896b01e.b none swap sw
> > > ca7552589896b01e.a / ffs rw 1 1
> > > ca7552589896b01e.k /home ffs rw,nodev,nosuid 1 2
> > > #ca7552589896b01e.d /tmp ffs rw,nodev,nosuid 1 2
> > > swap /tmp mfs rw,nodev,nosuid,-s=300M 0 0
> > > ca7552589896b01e.f /usr ffs rw,nodev 1 2
> > > ca7552589896b01e.g /usr/X11R6 ffs rw,nodev 1 2
> > > ca7552589896b01e.h /usr/local ffs rw,nodev,wxallowed 1 2
> > > ca7552589896b01e.j /usr/build ffs rw,noperm,noauto 1 2
> > > swap /usr/obj mfs rw,nodev,nosuid,noauto,-s=4G,-P=/foo/bar  0 0
> > > ca7552589896b01e.i /usr/src ffs rw,nodev,nosuid 1 2
> > > ca7552589896b01e.e /var ffs rw,nodev,nosuid 1 2
> > >
> > > Result when trying to mount /usr/obj:
> > > root@orus [rneves]# mount /usr/obj
> > > mount_mfs: cannot stat /foo/bar: No such file or directory
> > > root@orus [rneves]# mount
> > > /dev/sd2a on / type ffs (local)
> > > /dev/sd2k on /home type ffs (local, nodev, nosuid)
> > > mfs:28249 on /tmp type mfs (asynchronous, local, nodev, nosuid, size=614400 512-blocks)
> > > /dev/sd2f on /usr type ffs (local, nodev)
> > > /dev/sd2g on /usr/X11R6 type ffs (local, nodev)
> > > /dev/sd2h on /usr/local type ffs (local, nodev, wxallowed)
> > > /dev/sd2i on /usr/src type ffs (local, nodev, nosuid)
> > > /dev/sd2e on /var type ffs (local, nodev, nosuid)
> > > mfs:44634 on /tmp/mntoMG6WmZTT7 type mfs (asynchronous, local, nodev, nosuid, size=8388608 512-blocks)
> > >
> > >
> > > Tracking down the issue I found that:
> > > + When -P is specified, pop != NULL.
> > > + After fork, waitformount() is called. It creates the temporary
> > >  places to store the data.
> > > + copy() is called, and it it fails the following umount() and
> > >  rmdir() is not called, leaving the temporary mounts.
> > >
> > > As copy() can fail in a couple of ways, I thought about the following change:
> > > + Make all errors a warning, but after then return to the first
> > >           caller indicating an error. Getting the erro the clean up is
> > >  done, and exit(1).
> > >
> > > + Make copy() return an int: -1 in fail, and 0 otherwise.
> > > + isdir(), gettmpmnt(): Convert errors to warnings + return(-1).
> > >
> > > There is a change of messages printing if you don't have a /tmp
> > > is read-only, first the error of cannot fstat, and after
> > > "Cannot create tmp mountpoint for -P".
> > >
> > > There still a chance to get a inconsistent state: if there is no
> > > /bin/pax, or errors in do_exec(). But erros in do_exec seem very
> > > critical, the same with a missing /bin/pax. So I didn't change them.
> > >
> > > Otto@ said that another alternative is using atexit(3), but we
> > > pointed out what it is very difficult to get it right, and almost
> > > always has race conditions. Given that and what manpage says I have no
> > > hope that I can get it right.
> > >
> > > What do you think?
> > >
> > > Note that the check in line 519 (newfs.c) was changed to add the new
> > > possible return value. Actually, currently it is not allowed -P with a
> > > read-only /tmp, because in this case gettmpmnt() returns 0, and by this
> > > early check newfs(2) throws an error. Actually, gettmpmnt() must changed
> > > to it work properly. The `created` if uses a <= by symmetry.
> > >
> > > But this is a differente issue, that I think could be changed in a
> > > separated diff.
> > >
> > > Regards,
> > > Rafael Neves
> > >
> > >
> > > Patch:
> > >
> > >
> > > Index: newfs.c
> > > ===================================================================
> > > RCS file: /cvs/src/sbin/newfs/newfs.c,v
> > > retrieving revision 1.112
> > > diff -u -p -r1.112 newfs.c
> > > --- newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> > > +++ newfs.c 17 Aug 2019 14:27:46 -0000
> > > @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
> > >  static void waitformount(char *, pid_t);
> > >  static int do_exec(const char *, const char *, char *const[]);
> > >  static int isdir(const char *);
> > > -static void copy(char *, char *);
> > > +static int copy(char *, char *);
> > >  static int gettmpmnt(char *, size_t);
> > >  #endif
> > >  
> > > @@ -179,6 +179,7 @@ main(int argc, char *argv[])
> > >  #ifdef MFS
> > >   char mountfromname[BUFSIZ];
> > >   char *pop = NULL, node[PATH_MAX];
> > > + int ret;
> >
> > Please move this one inside the scope where it is used, see below
> >
> > >   pid_t pid;
> > >   struct stat mountpoint;
> > >  #endif
> > > @@ -516,7 +517,7 @@ havelabel:
> > >   struct mfs_args args;
> > >   char tmpnode[PATH_MAX];
> >
> > here
> >
> > >  
> > > - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> > > + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
> > >   errx(1, "Cannot create tmp mountpoint for -P");
> > >   memset(&args, 0, sizeof(args));
> > >   args.base = membase;
> > > @@ -537,9 +538,11 @@ havelabel:
> > >   default:
> > >   if (pop != NULL) {
> > >   waitformount(tmpnode, pid);
> > > - copy(pop, tmpnode);
> > > + ret = copy(pop, tmpnode);
> > >   unmount(tmpnode, 0);
> > >   rmdir(tmpnode);
> > > + if (ret == -1)
> > > + exit(1);
> >
> > >   }
> > >   waitformount(node, pid);
> >
> > It might be an idea to undo the mount an return 1 if ret is != 0.
> > For that to work ret should be initialized to 0.
> >
> > -Otto
> > [snip]
> >
>
> Updated patch follows. I understood that return is through exit(3) as
> it is pattern used in other parts of the code.

I don't code undoing the mount below...

        -Otto

>
>
> Index: sbin/newfs/newfs.c
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/newfs.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 newfs.c
> --- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> +++ sbin/newfs/newfs.c 1 Sep 2019 18:52:26 -0000
> @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
>  static void waitformount(char *, pid_t);
>  static int do_exec(const char *, const char *, char *const[]);
>  static int isdir(const char *);
> -static void copy(char *, char *);
> +static int copy(char *, char *);
>  static int gettmpmnt(char *, size_t);
>  #endif
>  
> @@ -515,8 +515,9 @@ havelabel:
>   if (mfs) {
>   struct mfs_args args;
>   char tmpnode[PATH_MAX];
> + int ret = 0;
>  
> - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
>   errx(1, "Cannot create tmp mountpoint for -P");
>   memset(&args, 0, sizeof(args));
>   args.base = membase;
> @@ -537,9 +538,11 @@ havelabel:
>   default:
>   if (pop != NULL) {
>   waitformount(tmpnode, pid);
> - copy(pop, tmpnode);
> + ret = copy(pop, tmpnode);
>   unmount(tmpnode, 0);
>   rmdir(tmpnode);
> + if (ret != 0)
> + exit(1);
>   }
>   waitformount(node, pid);

Would expect it here...

>   exit(0);
> @@ -740,14 +743,18 @@ isdir(const char *path)
>  {
>   struct stat st;
>  
> - if (stat(path, &st) != 0)
> - err(1, "cannot stat %s", path);
> - if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
> - errx(1, "%s: not a dir or a block device", path);
> + if (stat(path, &st) != 0) {
> + warn("cannot stat %s", path);
> + return (-1);
> + }
> + if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
> + warn("%s: not a dir or a block device", path);
> + return  (-1);
> + }
>   return (S_ISDIR(st.st_mode));
>  }
>  
> -static void
> +static int
>  copy(char *src, char *dst)
>  {
>   int ret, dir, created = 0;
> @@ -755,11 +762,16 @@ copy(char *src, char *dst)
>   char mountpoint[MNAMELEN];
>   char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
>  
> - dir = isdir(src);
> + if ((dir = isdir(src)) == -1)
> + return (-1);
> +
>   if (dir)
>   strlcpy(mountpoint, src, sizeof(mountpoint));
>   else {
>   created = gettmpmnt(mountpoint, sizeof(mountpoint));
> + if (created <= 0)
> + return (-1);
> +
>   memset(&mount_args, 0, sizeof(mount_args));
>   mount_args.fspec = src;
>   ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
> @@ -769,7 +781,8 @@ copy(char *src, char *dst)
>   warn("rmdir %s", mountpoint);
>   if (unmount(dst, 0) != 0)
>   warn("unmount %s", dst);
> - errc(1, saved_errno, "mount %s %s", src, mountpoint);
> + warnc(saved_errno, "mount %s %s", src, mountpoint);
> + return (-1);
>   }
>   }
>   ret = do_exec(mountpoint, "/bin/pax", argv);
> @@ -780,8 +793,10 @@ copy(char *src, char *dst)
>   if (ret != 0) {
>   if (unmount(dst, 0) != 0)
>   warn("unmount %s", dst);
> - errx(1, "copy %s to %s failed", mountpoint, dst);
> + warnx("copy %s to %s failed", mountpoint, dst);
> + return (-1);
>   }
> + return (0);
>  }
>  
>  static int
> @@ -792,27 +807,42 @@ gettmpmnt(char *mountpoint, size_t len)
>   struct statfs fs;
>   size_t n;
>  
> - if (statfs(tmp, &fs) != 0)
> - err(1, "statfs %s", tmp);
> + if (statfs(tmp, &fs) != 0) {
> + warn("statfs %s", tmp);
> + return (-1);
> + }
> +
>   if (fs.f_flags & MNT_RDONLY) {
> - if (statfs(mnt, &fs) != 0)
> - err(1, "statfs %s", mnt);
> - if (strcmp(fs.f_mntonname, "/") != 0)
> - errx(1, "tmp mountpoint %s busy", mnt);
> - if (strlcpy(mountpoint, mnt, len) >= len)
> - errx(1, "tmp mountpoint %s too long", mnt);
> + if (statfs(mnt, &fs) != 0) {
> + warn("statfs %s", mnt);
> + return (-1);
> + }
> + if (strcmp(fs.f_mntonname, "/") != 0) {
> + warnx("tmp mountpoint %s busy", mnt);
> + return (-1);
> + }
> + if (strlcpy(mountpoint, mnt, len) >= len) {
> + warnx("tmp mountpoint %s too long", mnt);
> + return (-1);
> + }
>   return (0);
>   }
>   n = strlcpy(mountpoint, tmp, len);
> - if (n >= len)
> - errx(1, "tmp mount point too long");
> + if (n >= len) {
> + warnx("tmp mount point too long");
> + return (-1);
> + }
>   if (mountpoint[n - 1] != '/')
>   strlcat(mountpoint, "/", len);
>   n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
> - if (n >= len)
> - errx(1, "tmp mount point too long");
> - if (mkdtemp(mountpoint) == NULL)
> - err(1, "mkdtemp %s", mountpoint);
> + if (n >= len) {
> + warnx("tmp mount point too long");
> + return (-1);
> + }
> + if (mkdtemp(mountpoint) == NULL) {
> + warn("mkdtemp %s", mountpoint);
> + return (-1);
> + }
>   return (1);
>  }
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Avoid leftover temporary mount points when using -P (mfs)

Rafael Neves-2
On Mon, Sep 02, 2019 at 07:26:27AM +0200, Otto Moerbeek wrote:

> On Sun, Sep 01, 2019 at 05:01:55PM -0300, Rafael Neves wrote:
>
> > On Wed, Aug 28, 2019 at 03:39:17PM +0200, Otto Moerbeek wrote:
> > > On Sat, Aug 17, 2019 at 12:13:50PM -0300, Rafael Neves wrote:
> > >
> > > > Hi,
> > > >
> > > > Submitting to tech@ to broader audience.
> > > >
> > > > When using -P option in mfs with a directory or a block device that
> > > > doen't exist, for example when the device roams, newfs(2) leaves
> > > > leftovers of temporary mount points.
> > > >
> > > > With my /etc/fstab:
> > > > ca7552589896b01e.b none swap sw
> > > > ca7552589896b01e.a / ffs rw 1 1
> > > > ca7552589896b01e.k /home ffs rw,nodev,nosuid 1 2
> > > > #ca7552589896b01e.d /tmp ffs rw,nodev,nosuid 1 2
> > > > swap /tmp mfs rw,nodev,nosuid,-s=300M 0 0
> > > > ca7552589896b01e.f /usr ffs rw,nodev 1 2
> > > > ca7552589896b01e.g /usr/X11R6 ffs rw,nodev 1 2
> > > > ca7552589896b01e.h /usr/local ffs rw,nodev,wxallowed 1 2
> > > > ca7552589896b01e.j /usr/build ffs rw,noperm,noauto 1 2
> > > > swap /usr/obj mfs rw,nodev,nosuid,noauto,-s=4G,-P=/foo/bar  0 0
> > > > ca7552589896b01e.i /usr/src ffs rw,nodev,nosuid 1 2
> > > > ca7552589896b01e.e /var ffs rw,nodev,nosuid 1 2
> > > >
> > > > Result when trying to mount /usr/obj:
> > > > root@orus [rneves]# mount /usr/obj
> > > > mount_mfs: cannot stat /foo/bar: No such file or directory
> > > > root@orus [rneves]# mount
> > > > /dev/sd2a on / type ffs (local)
> > > > /dev/sd2k on /home type ffs (local, nodev, nosuid)
> > > > mfs:28249 on /tmp type mfs (asynchronous, local, nodev, nosuid, size=614400 512-blocks)
> > > > /dev/sd2f on /usr type ffs (local, nodev)
> > > > /dev/sd2g on /usr/X11R6 type ffs (local, nodev)
> > > > /dev/sd2h on /usr/local type ffs (local, nodev, wxallowed)
> > > > /dev/sd2i on /usr/src type ffs (local, nodev, nosuid)
> > > > /dev/sd2e on /var type ffs (local, nodev, nosuid)
> > > > mfs:44634 on /tmp/mntoMG6WmZTT7 type mfs (asynchronous, local, nodev, nosuid, size=8388608 512-blocks)
> > > >
> > > >
> > > > Tracking down the issue I found that:
> > > > + When -P is specified, pop != NULL.
> > > > + After fork, waitformount() is called. It creates the temporary
> > > >  places to store the data.
> > > > + copy() is called, and it it fails the following umount() and
> > > >  rmdir() is not called, leaving the temporary mounts.
> > > >
> > > > As copy() can fail in a couple of ways, I thought about the following change:
> > > > + Make all errors a warning, but after then return to the first
> > > >           caller indicating an error. Getting the erro the clean up is
> > > >  done, and exit(1).
> > > >
> > > > + Make copy() return an int: -1 in fail, and 0 otherwise.
> > > > + isdir(), gettmpmnt(): Convert errors to warnings + return(-1).
> > > >
> > > > There is a change of messages printing if you don't have a /tmp
> > > > is read-only, first the error of cannot fstat, and after
> > > > "Cannot create tmp mountpoint for -P".
> > > >
> > > > There still a chance to get a inconsistent state: if there is no
> > > > /bin/pax, or errors in do_exec(). But erros in do_exec seem very
> > > > critical, the same with a missing /bin/pax. So I didn't change them.
> > > >
> > > > Otto@ said that another alternative is using atexit(3), but we
> > > > pointed out what it is very difficult to get it right, and almost
> > > > always has race conditions. Given that and what manpage says I have no
> > > > hope that I can get it right.
> > > >
> > > > What do you think?
> > > >
> > > > Note that the check in line 519 (newfs.c) was changed to add the new
> > > > possible return value. Actually, currently it is not allowed -P with a
> > > > read-only /tmp, because in this case gettmpmnt() returns 0, and by this
> > > > early check newfs(2) throws an error. Actually, gettmpmnt() must changed
> > > > to it work properly. The `created` if uses a <= by symmetry.
> > > >
> > > > But this is a differente issue, that I think could be changed in a
> > > > separated diff.
> > > >
> > > > Regards,
> > > > Rafael Neves
> > > >
> > > >
> > > > Patch:
> > > >
> > > >
> > > > Index: newfs.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sbin/newfs/newfs.c,v
> > > > retrieving revision 1.112
> > > > diff -u -p -r1.112 newfs.c
> > > > --- newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> > > > +++ newfs.c 17 Aug 2019 14:27:46 -0000
> > > > @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
> > > >  static void waitformount(char *, pid_t);
> > > >  static int do_exec(const char *, const char *, char *const[]);
> > > >  static int isdir(const char *);
> > > > -static void copy(char *, char *);
> > > > +static int copy(char *, char *);
> > > >  static int gettmpmnt(char *, size_t);
> > > >  #endif
> > > >  
> > > > @@ -179,6 +179,7 @@ main(int argc, char *argv[])
> > > >  #ifdef MFS
> > > >   char mountfromname[BUFSIZ];
> > > >   char *pop = NULL, node[PATH_MAX];
> > > > + int ret;
> > >
> > > Please move this one inside the scope where it is used, see below
> > >
> > > >   pid_t pid;
> > > >   struct stat mountpoint;
> > > >  #endif
> > > > @@ -516,7 +517,7 @@ havelabel:
> > > >   struct mfs_args args;
> > > >   char tmpnode[PATH_MAX];
> > >
> > > here
> > >
> > > >  
> > > > - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> > > > + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
> > > >   errx(1, "Cannot create tmp mountpoint for -P");
> > > >   memset(&args, 0, sizeof(args));
> > > >   args.base = membase;
> > > > @@ -537,9 +538,11 @@ havelabel:
> > > >   default:
> > > >   if (pop != NULL) {
> > > >   waitformount(tmpnode, pid);
> > > > - copy(pop, tmpnode);
> > > > + ret = copy(pop, tmpnode);
> > > >   unmount(tmpnode, 0);
> > > >   rmdir(tmpnode);
> > > > + if (ret == -1)
> > > > + exit(1);
> > >
> > > >   }
> > > >   waitformount(node, pid);
> > >
> > > It might be an idea to undo the mount an return 1 if ret is != 0.
> > > For that to work ret should be initialized to 0.
> > >
> > > -Otto
> > > [snip]
> > >
> >
> > Updated patch follows. I understood that return is through exit(3) as
> > it is pattern used in other parts of the code.
>
> I don't code undoing the mount below...
>
> -Otto
> >
> >
> > Index: sbin/newfs/newfs.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/newfs/newfs.c,v
> > retrieving revision 1.112
> > diff -u -p -r1.112 newfs.c
> > --- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> > +++ sbin/newfs/newfs.c 1 Sep 2019 18:52:26 -0000
> > @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
> >  static void waitformount(char *, pid_t);
> >  static int do_exec(const char *, const char *, char *const[]);
> >  static int isdir(const char *);
> > -static void copy(char *, char *);
> > +static int copy(char *, char *);
> >  static int gettmpmnt(char *, size_t);
> >  #endif
> >  
> > @@ -515,8 +515,9 @@ havelabel:
> >   if (mfs) {
> >   struct mfs_args args;
> >   char tmpnode[PATH_MAX];
> > + int ret = 0;
> >  
> > - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> > + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
> >   errx(1, "Cannot create tmp mountpoint for -P");
> >   memset(&args, 0, sizeof(args));
> >   args.base = membase;
> > @@ -537,9 +538,11 @@ havelabel:
> >   default:
> >   if (pop != NULL) {
> >   waitformount(tmpnode, pid);
> > - copy(pop, tmpnode);
> > + ret = copy(pop, tmpnode);
> >   unmount(tmpnode, 0);
> >   rmdir(tmpnode);
> > + if (ret != 0)
> > + exit(1);
> >   }
> >   waitformount(node, pid);
>
> Would expect it here...
>
> [snip]

Sure! Updated patch below.


Index: sbin/newfs/newfs.c
===================================================================
RCS file: /cvs/src/sbin/newfs/newfs.c,v
retrieving revision 1.112
diff -u -p -r1.112 newfs.c
--- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112
+++ sbin/newfs/newfs.c 2 Sep 2019 18:44:04 -0000
@@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
 static void waitformount(char *, pid_t);
 static int do_exec(const char *, const char *, char *const[]);
 static int isdir(const char *);
-static void copy(char *, char *);
+static int copy(char *, char *);
 static int gettmpmnt(char *, size_t);
 #endif
 
@@ -515,8 +515,9 @@ havelabel:
  if (mfs) {
  struct mfs_args args;
  char tmpnode[PATH_MAX];
+ int ret = 0;
 
- if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
+ if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
  errx(1, "Cannot create tmp mountpoint for -P");
  memset(&args, 0, sizeof(args));
  args.base = membase;
@@ -537,11 +538,15 @@ havelabel:
  default:
  if (pop != NULL) {
  waitformount(tmpnode, pid);
- copy(pop, tmpnode);
+ ret = copy(pop, tmpnode);
  unmount(tmpnode, 0);
  rmdir(tmpnode);
  }
  waitformount(node, pid);
+ if (ret != 0) {
+ unmount(node, 0);
+ exit(1);
+ }
  exit(0);
  /* NOTREACHED */
  }
@@ -740,14 +745,18 @@ isdir(const char *path)
 {
  struct stat st;
 
- if (stat(path, &st) != 0)
- err(1, "cannot stat %s", path);
- if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
- errx(1, "%s: not a dir or a block device", path);
+ if (stat(path, &st) != 0) {
+ warn("cannot stat %s", path);
+ return (-1);
+ }
+ if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
+ warn("%s: not a dir or a block device", path);
+ return  (-1);
+ }
  return (S_ISDIR(st.st_mode));
 }
 
-static void
+static int
 copy(char *src, char *dst)
 {
  int ret, dir, created = 0;
@@ -755,11 +764,16 @@ copy(char *src, char *dst)
  char mountpoint[MNAMELEN];
  char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
 
- dir = isdir(src);
+ if ((dir = isdir(src)) == -1)
+ return (-1);
+
  if (dir)
  strlcpy(mountpoint, src, sizeof(mountpoint));
  else {
  created = gettmpmnt(mountpoint, sizeof(mountpoint));
+ if (created <= 0)
+ return (-1);
+
  memset(&mount_args, 0, sizeof(mount_args));
  mount_args.fspec = src;
  ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
@@ -769,7 +783,8 @@ copy(char *src, char *dst)
  warn("rmdir %s", mountpoint);
  if (unmount(dst, 0) != 0)
  warn("unmount %s", dst);
- errc(1, saved_errno, "mount %s %s", src, mountpoint);
+ warnc(saved_errno, "mount %s %s", src, mountpoint);
+ return (-1);
  }
  }
  ret = do_exec(mountpoint, "/bin/pax", argv);
@@ -780,8 +795,10 @@ copy(char *src, char *dst)
  if (ret != 0) {
  if (unmount(dst, 0) != 0)
  warn("unmount %s", dst);
- errx(1, "copy %s to %s failed", mountpoint, dst);
+ warnx("copy %s to %s failed", mountpoint, dst);
+ return (-1);
  }
+ return (0);
 }
 
 static int
@@ -792,27 +809,42 @@ gettmpmnt(char *mountpoint, size_t len)
  struct statfs fs;
  size_t n;
 
- if (statfs(tmp, &fs) != 0)
- err(1, "statfs %s", tmp);
+ if (statfs(tmp, &fs) != 0) {
+ warn("statfs %s", tmp);
+ return (-1);
+ }
+
  if (fs.f_flags & MNT_RDONLY) {
- if (statfs(mnt, &fs) != 0)
- err(1, "statfs %s", mnt);
- if (strcmp(fs.f_mntonname, "/") != 0)
- errx(1, "tmp mountpoint %s busy", mnt);
- if (strlcpy(mountpoint, mnt, len) >= len)
- errx(1, "tmp mountpoint %s too long", mnt);
+ if (statfs(mnt, &fs) != 0) {
+ warn("statfs %s", mnt);
+ return (-1);
+ }
+ if (strcmp(fs.f_mntonname, "/") != 0) {
+ warnx("tmp mountpoint %s busy", mnt);
+ return (-1);
+ }
+ if (strlcpy(mountpoint, mnt, len) >= len) {
+ warnx("tmp mountpoint %s too long", mnt);
+ return (-1);
+ }
  return (0);
  }
  n = strlcpy(mountpoint, tmp, len);
- if (n >= len)
- errx(1, "tmp mount point too long");
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
  if (mountpoint[n - 1] != '/')
  strlcat(mountpoint, "/", len);
  n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
- if (n >= len)
- errx(1, "tmp mount point too long");
- if (mkdtemp(mountpoint) == NULL)
- err(1, "mkdtemp %s", mountpoint);
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
+ if (mkdtemp(mountpoint) == NULL) {
+ warn("mkdtemp %s", mountpoint);
+ return (-1);
+ }
  return (1);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Avoid leftover temporary mount points when using -P (mfs)

Rafael Neves-2
On Mon, Sep 02, 2019 at 03:59:48PM -0300, Rafael Neves wrote:

> On Mon, Sep 02, 2019 at 07:26:27AM +0200, Otto Moerbeek wrote:
> > On Sun, Sep 01, 2019 at 05:01:55PM -0300, Rafael Neves wrote:
> >
> > > On Wed, Aug 28, 2019 at 03:39:17PM +0200, Otto Moerbeek wrote:
> > > > On Sat, Aug 17, 2019 at 12:13:50PM -0300, Rafael Neves wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > Submitting to tech@ to broader audience.
> > > > >
> > > > > When using -P option in mfs with a directory or a block device that
> > > > > doen't exist, for example when the device roams, newfs(2) leaves
> > > > > leftovers of temporary mount points.
> > > > >
> > > > > With my /etc/fstab:
> > > > > ca7552589896b01e.b none swap sw
> > > > > ca7552589896b01e.a / ffs rw 1 1
> > > > > ca7552589896b01e.k /home ffs rw,nodev,nosuid 1 2
> > > > > #ca7552589896b01e.d /tmp ffs rw,nodev,nosuid 1 2
> > > > > swap /tmp mfs rw,nodev,nosuid,-s=300M 0 0
> > > > > ca7552589896b01e.f /usr ffs rw,nodev 1 2
> > > > > ca7552589896b01e.g /usr/X11R6 ffs rw,nodev 1 2
> > > > > ca7552589896b01e.h /usr/local ffs rw,nodev,wxallowed 1 2
> > > > > ca7552589896b01e.j /usr/build ffs rw,noperm,noauto 1 2
> > > > > swap /usr/obj mfs rw,nodev,nosuid,noauto,-s=4G,-P=/foo/bar  0 0
> > > > > ca7552589896b01e.i /usr/src ffs rw,nodev,nosuid 1 2
> > > > > ca7552589896b01e.e /var ffs rw,nodev,nosuid 1 2
> > > > >
> > > > > Result when trying to mount /usr/obj:
> > > > > root@orus [rneves]# mount /usr/obj
> > > > > mount_mfs: cannot stat /foo/bar: No such file or directory
> > > > > root@orus [rneves]# mount
> > > > > /dev/sd2a on / type ffs (local)
> > > > > /dev/sd2k on /home type ffs (local, nodev, nosuid)
> > > > > mfs:28249 on /tmp type mfs (asynchronous, local, nodev, nosuid, size=614400 512-blocks)
> > > > > /dev/sd2f on /usr type ffs (local, nodev)
> > > > > /dev/sd2g on /usr/X11R6 type ffs (local, nodev)
> > > > > /dev/sd2h on /usr/local type ffs (local, nodev, wxallowed)
> > > > > /dev/sd2i on /usr/src type ffs (local, nodev, nosuid)
> > > > > /dev/sd2e on /var type ffs (local, nodev, nosuid)
> > > > > mfs:44634 on /tmp/mntoMG6WmZTT7 type mfs (asynchronous, local, nodev, nosuid, size=8388608 512-blocks)
> > > > >
> > > > >
> > > > > Tracking down the issue I found that:
> > > > > + When -P is specified, pop != NULL.
> > > > > + After fork, waitformount() is called. It creates the temporary
> > > > >  places to store the data.
> > > > > + copy() is called, and it it fails the following umount() and
> > > > >  rmdir() is not called, leaving the temporary mounts.
> > > > >
> > > > > As copy() can fail in a couple of ways, I thought about the following change:
> > > > > + Make all errors a warning, but after then return to the first
> > > > >           caller indicating an error. Getting the erro the clean up is
> > > > >  done, and exit(1).
> > > > >
> > > > > + Make copy() return an int: -1 in fail, and 0 otherwise.
> > > > > + isdir(), gettmpmnt(): Convert errors to warnings + return(-1).
> > > > >
> > > > > There is a change of messages printing if you don't have a /tmp
> > > > > is read-only, first the error of cannot fstat, and after
> > > > > "Cannot create tmp mountpoint for -P".
> > > > >
> > > > > There still a chance to get a inconsistent state: if there is no
> > > > > /bin/pax, or errors in do_exec(). But erros in do_exec seem very
> > > > > critical, the same with a missing /bin/pax. So I didn't change them.
> > > > >
> > > > > Otto@ said that another alternative is using atexit(3), but we
> > > > > pointed out what it is very difficult to get it right, and almost
> > > > > always has race conditions. Given that and what manpage says I have no
> > > > > hope that I can get it right.
> > > > >
> > > > > What do you think?
> > > > >
> > > > > Note that the check in line 519 (newfs.c) was changed to add the new
> > > > > possible return value. Actually, currently it is not allowed -P with a
> > > > > read-only /tmp, because in this case gettmpmnt() returns 0, and by this
> > > > > early check newfs(2) throws an error. Actually, gettmpmnt() must changed
> > > > > to it work properly. The `created` if uses a <= by symmetry.
> > > > >
> > > > > But this is a differente issue, that I think could be changed in a
> > > > > separated diff.
> > > > >
> > > > > Regards,
> > > > > Rafael Neves
> > > > >
> > > > >
> > > > > Patch:
> > > > >
> > > > >
> > > > > Index: newfs.c
> > > > > ===================================================================
> > > > > RCS file: /cvs/src/sbin/newfs/newfs.c,v
> > > > > retrieving revision 1.112
> > > > > diff -u -p -r1.112 newfs.c
> > > > > --- newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> > > > > +++ newfs.c 17 Aug 2019 14:27:46 -0000
> > > > > @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
> > > > >  static void waitformount(char *, pid_t);
> > > > >  static int do_exec(const char *, const char *, char *const[]);
> > > > >  static int isdir(const char *);
> > > > > -static void copy(char *, char *);
> > > > > +static int copy(char *, char *);
> > > > >  static int gettmpmnt(char *, size_t);
> > > > >  #endif
> > > > >  
> > > > > @@ -179,6 +179,7 @@ main(int argc, char *argv[])
> > > > >  #ifdef MFS
> > > > >   char mountfromname[BUFSIZ];
> > > > >   char *pop = NULL, node[PATH_MAX];
> > > > > + int ret;
> > > >
> > > > Please move this one inside the scope where it is used, see below
> > > >
> > > > >   pid_t pid;
> > > > >   struct stat mountpoint;
> > > > >  #endif
> > > > > @@ -516,7 +517,7 @@ havelabel:
> > > > >   struct mfs_args args;
> > > > >   char tmpnode[PATH_MAX];
> > > >
> > > > here
> > > >
> > > > >  
> > > > > - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> > > > > + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
> > > > >   errx(1, "Cannot create tmp mountpoint for -P");
> > > > >   memset(&args, 0, sizeof(args));
> > > > >   args.base = membase;
> > > > > @@ -537,9 +538,11 @@ havelabel:
> > > > >   default:
> > > > >   if (pop != NULL) {
> > > > >   waitformount(tmpnode, pid);
> > > > > - copy(pop, tmpnode);
> > > > > + ret = copy(pop, tmpnode);
> > > > >   unmount(tmpnode, 0);
> > > > >   rmdir(tmpnode);
> > > > > + if (ret == -1)
> > > > > + exit(1);
> > > >
> > > > >   }
> > > > >   waitformount(node, pid);
> > > >
> > > > It might be an idea to undo the mount an return 1 if ret is != 0.
> > > > For that to work ret should be initialized to 0.
> > > >
> > > > -Otto
> > > > [snip]
> > > >
> > >
> > > Updated patch follows. I understood that return is through exit(3) as
> > > it is pattern used in other parts of the code.
> >
> > I don't code undoing the mount below...
> >
> > -Otto
> > >
> > >
> > > Index: sbin/newfs/newfs.c
> > > ===================================================================
> > > RCS file: /cvs/src/sbin/newfs/newfs.c,v
> > > retrieving revision 1.112
> > > diff -u -p -r1.112 newfs.c
> > > --- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> > > +++ sbin/newfs/newfs.c 1 Sep 2019 18:52:26 -0000
> > > @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
> > >  static void waitformount(char *, pid_t);
> > >  static int do_exec(const char *, const char *, char *const[]);
> > >  static int isdir(const char *);
> > > -static void copy(char *, char *);
> > > +static int copy(char *, char *);
> > >  static int gettmpmnt(char *, size_t);
> > >  #endif
> > >  
> > > @@ -515,8 +515,9 @@ havelabel:
> > >   if (mfs) {
> > >   struct mfs_args args;
> > >   char tmpnode[PATH_MAX];
> > > + int ret = 0;
> > >  
> > > - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> > > + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
> > >   errx(1, "Cannot create tmp mountpoint for -P");
> > >   memset(&args, 0, sizeof(args));
> > >   args.base = membase;
> > > @@ -537,9 +538,11 @@ havelabel:
> > >   default:
> > >   if (pop != NULL) {
> > >   waitformount(tmpnode, pid);
> > > - copy(pop, tmpnode);
> > > + ret = copy(pop, tmpnode);
> > >   unmount(tmpnode, 0);
> > >   rmdir(tmpnode);
> > > + if (ret != 0)
> > > + exit(1);
> > >   }
> > >   waitformount(node, pid);
> >
> > Would expect it here...
> >
> > [snip]
>
> Sure! Updated patch below.
>
>
> Index: sbin/newfs/newfs.c
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/newfs.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 newfs.c
> --- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> +++ sbin/newfs/newfs.c 2 Sep 2019 18:44:04 -0000
> @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
>  static void waitformount(char *, pid_t);
>  static int do_exec(const char *, const char *, char *const[]);
>  static int isdir(const char *);
> -static void copy(char *, char *);
> +static int copy(char *, char *);
>  static int gettmpmnt(char *, size_t);
>  #endif
>  
> @@ -515,8 +515,9 @@ havelabel:
>   if (mfs) {
>   struct mfs_args args;
>   char tmpnode[PATH_MAX];
> + int ret = 0;
>  
> - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) <= 0)
>   errx(1, "Cannot create tmp mountpoint for -P");
>   memset(&args, 0, sizeof(args));
>   args.base = membase;
> @@ -537,11 +538,15 @@ havelabel:
>   default:
>   if (pop != NULL) {
>   waitformount(tmpnode, pid);
> - copy(pop, tmpnode);
> + ret = copy(pop, tmpnode);
>   unmount(tmpnode, 0);
>   rmdir(tmpnode);
>   }
>   waitformount(node, pid);
> + if (ret != 0) {
> + unmount(node, 0);
> + exit(1);
> + }
>   exit(0);
>   /* NOTREACHED */
>   }
> @@ -740,14 +745,18 @@ isdir(const char *path)
>  {
>   struct stat st;
>  
> - if (stat(path, &st) != 0)
> - err(1, "cannot stat %s", path);
> - if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
> - errx(1, "%s: not a dir or a block device", path);
> + if (stat(path, &st) != 0) {
> + warn("cannot stat %s", path);
> + return (-1);
> + }
> + if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
> + warn("%s: not a dir or a block device", path);
> + return  (-1);
> + }
>   return (S_ISDIR(st.st_mode));
>  }
>  
> -static void
> +static int
>  copy(char *src, char *dst)
>  {
>   int ret, dir, created = 0;
> @@ -755,11 +764,16 @@ copy(char *src, char *dst)
>   char mountpoint[MNAMELEN];
>   char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
>  
> - dir = isdir(src);
> + if ((dir = isdir(src)) == -1)
> + return (-1);
> +
>   if (dir)
>   strlcpy(mountpoint, src, sizeof(mountpoint));
>   else {
>   created = gettmpmnt(mountpoint, sizeof(mountpoint));
> + if (created <= 0)
> + return (-1);
> +
>   memset(&mount_args, 0, sizeof(mount_args));
>   mount_args.fspec = src;
>   ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
> @@ -769,7 +783,8 @@ copy(char *src, char *dst)
>   warn("rmdir %s", mountpoint);
>   if (unmount(dst, 0) != 0)
>   warn("unmount %s", dst);
> - errc(1, saved_errno, "mount %s %s", src, mountpoint);
> + warnc(saved_errno, "mount %s %s", src, mountpoint);
> + return (-1);
>   }
>   }
>   ret = do_exec(mountpoint, "/bin/pax", argv);
> @@ -780,8 +795,10 @@ copy(char *src, char *dst)
>   if (ret != 0) {
>   if (unmount(dst, 0) != 0)
>   warn("unmount %s", dst);
> - errx(1, "copy %s to %s failed", mountpoint, dst);
> + warnx("copy %s to %s failed", mountpoint, dst);
> + return (-1);
>   }
> + return (0);
>  }
>  
>  static int
> @@ -792,27 +809,42 @@ gettmpmnt(char *mountpoint, size_t len)
>   struct statfs fs;
>   size_t n;
>  
> - if (statfs(tmp, &fs) != 0)
> - err(1, "statfs %s", tmp);
> + if (statfs(tmp, &fs) != 0) {
> + warn("statfs %s", tmp);
> + return (-1);
> + }
> +
>   if (fs.f_flags & MNT_RDONLY) {
> - if (statfs(mnt, &fs) != 0)
> - err(1, "statfs %s", mnt);
> - if (strcmp(fs.f_mntonname, "/") != 0)
> - errx(1, "tmp mountpoint %s busy", mnt);
> - if (strlcpy(mountpoint, mnt, len) >= len)
> - errx(1, "tmp mountpoint %s too long", mnt);
> + if (statfs(mnt, &fs) != 0) {
> + warn("statfs %s", mnt);
> + return (-1);
> + }
> + if (strcmp(fs.f_mntonname, "/") != 0) {
> + warnx("tmp mountpoint %s busy", mnt);
> + return (-1);
> + }
> + if (strlcpy(mountpoint, mnt, len) >= len) {
> + warnx("tmp mountpoint %s too long", mnt);
> + return (-1);
> + }
>   return (0);
>   }
>   n = strlcpy(mountpoint, tmp, len);
> - if (n >= len)
> - errx(1, "tmp mount point too long");
> + if (n >= len) {
> + warnx("tmp mount point too long");
> + return (-1);
> + }
>   if (mountpoint[n - 1] != '/')
>   strlcat(mountpoint, "/", len);
>   n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
> - if (n >= len)
> - errx(1, "tmp mount point too long");
> - if (mkdtemp(mountpoint) == NULL)
> - err(1, "mkdtemp %s", mountpoint);
> + if (n >= len) {
> + warnx("tmp mount point too long");
> + return (-1);
> + }
> + if (mkdtemp(mountpoint) == NULL) {
> + warn("mkdtemp %s", mountpoint);
> + return (-1);
> + }
>   return (1);
>  }
>  
>


Updated patch: It includes Otto's requests and remove unnecessary
unmount(dst, 0) from copy(), because the caller unmount tmpnode.
While there, I adjusted the return value of gettmpmnt() to return 0
on success, and changed the checks according.

Regards,
Rafael Neves


Index: sbin/newfs/newfs.c
===================================================================
RCS file: /cvs/src/sbin/newfs/newfs.c,v
retrieving revision 1.112
diff -u -p -r1.112 newfs.c
--- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112
+++ sbin/newfs/newfs.c 8 Sep 2019 22:49:11 -0000
@@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
 static void waitformount(char *, pid_t);
 static int do_exec(const char *, const char *, char *const[]);
 static int isdir(const char *);
-static void copy(char *, char *);
+static int copy(char *, char *);
 static int gettmpmnt(char *, size_t);
 #endif
 
@@ -515,8 +515,9 @@ havelabel:
  if (mfs) {
  struct mfs_args args;
  char tmpnode[PATH_MAX];
+ int ret = 0;
 
- if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
+ if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) != 0)
  errx(1, "Cannot create tmp mountpoint for -P");
  memset(&args, 0, sizeof(args));
  args.base = membase;
@@ -537,11 +538,15 @@ havelabel:
  default:
  if (pop != NULL) {
  waitformount(tmpnode, pid);
- copy(pop, tmpnode);
+ ret = copy(pop, tmpnode);
  unmount(tmpnode, 0);
  rmdir(tmpnode);
  }
  waitformount(node, pid);
+ if (ret != 0) {
+ unmount(node, 0);
+ exit(1);
+ }
  exit(0);
  /* NOTREACHED */
  }
@@ -740,48 +745,55 @@ isdir(const char *path)
 {
  struct stat st;
 
- if (stat(path, &st) != 0)
- err(1, "cannot stat %s", path);
- if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
- errx(1, "%s: not a dir or a block device", path);
+ if (stat(path, &st) != 0) {
+ warn("cannot stat %s", path);
+ return (-1);
+ }
+ if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
+ warn("%s: not a dir or a block device", path);
+ return  (-1);
+ }
  return (S_ISDIR(st.st_mode));
 }
 
-static void
+static int
 copy(char *src, char *dst)
 {
- int ret, dir, created = 0;
+ int ret, dir;
  struct ufs_args mount_args;
  char mountpoint[MNAMELEN];
  char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
 
- dir = isdir(src);
+ if ((dir = isdir(src)) == -1)
+ return (-1);
+
  if (dir)
  strlcpy(mountpoint, src, sizeof(mountpoint));
  else {
- created = gettmpmnt(mountpoint, sizeof(mountpoint));
+ if (gettmpmnt(mountpoint, sizeof(mountpoint)) != 0)
+ return (-1);
+
  memset(&mount_args, 0, sizeof(mount_args));
  mount_args.fspec = src;
  ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
  if (ret != 0) {
  int saved_errno = errno;
- if (created && rmdir(mountpoint) != 0)
+ if (rmdir(mountpoint) != 0)
  warn("rmdir %s", mountpoint);
- if (unmount(dst, 0) != 0)
- warn("unmount %s", dst);
- errc(1, saved_errno, "mount %s %s", src, mountpoint);
+ warnc(saved_errno, "mount %s %s", src, mountpoint);
+ return (-1);
  }
  }
  ret = do_exec(mountpoint, "/bin/pax", argv);
  if (!dir && unmount(mountpoint, 0) != 0)
  warn("unmount %s", mountpoint);
- if (created && rmdir(mountpoint) != 0)
+ if (!dir && rmdir(mountpoint) != 0)
  warn("rmdir %s", mountpoint);
  if (ret != 0) {
- if (unmount(dst, 0) != 0)
- warn("unmount %s", dst);
- errx(1, "copy %s to %s failed", mountpoint, dst);
+ warnx("copy %s to %s failed", mountpoint, dst);
+ return (-1);
  }
+ return (0);
 }
 
 static int
@@ -792,28 +804,43 @@ gettmpmnt(char *mountpoint, size_t len)
  struct statfs fs;
  size_t n;
 
- if (statfs(tmp, &fs) != 0)
- err(1, "statfs %s", tmp);
+ if (statfs(tmp, &fs) != 0) {
+ warn("statfs %s", tmp);
+ return (-1);
+ }
+
  if (fs.f_flags & MNT_RDONLY) {
- if (statfs(mnt, &fs) != 0)
- err(1, "statfs %s", mnt);
- if (strcmp(fs.f_mntonname, "/") != 0)
- errx(1, "tmp mountpoint %s busy", mnt);
- if (strlcpy(mountpoint, mnt, len) >= len)
- errx(1, "tmp mountpoint %s too long", mnt);
- return (0);
+ if (statfs(mnt, &fs) != 0) {
+ warn("statfs %s", mnt);
+ return (-1);
+ }
+ if (strcmp(fs.f_mntonname, "/") != 0) {
+ warnx("tmp mountpoint %s busy", mnt);
+ return (-1);
+ }
+ if (strlcpy(mountpoint, mnt, len) >= len) {
+ warnx("tmp mountpoint %s too long", mnt);
+ return (-1);
+ }
+ return (-1);
  }
  n = strlcpy(mountpoint, tmp, len);
- if (n >= len)
- errx(1, "tmp mount point too long");
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
  if (mountpoint[n - 1] != '/')
  strlcat(mountpoint, "/", len);
  n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
- if (n >= len)
- errx(1, "tmp mount point too long");
- if (mkdtemp(mountpoint) == NULL)
- err(1, "mkdtemp %s", mountpoint);
- return (1);
+ if (n >= len) {
+ warnx("tmp mount point too long");
+ return (-1);
+ }
+ if (mkdtemp(mountpoint) == NULL) {
+ warn("mkdtemp %s", mountpoint);
+ return (-1);
+ }
+ return (0);
 }
 
 #endif /* MFS */

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Avoid leftover temporary mount points when using -P (mfs)

Otto Moerbeek
On Sun, Sep 08, 2019 at 10:10:06PM -0300, Rafael Neves wrote:

> Updated patch: It includes Otto's requests and remove unnecessary
> unmount(dst, 0) from copy(), because the caller unmount tmpnode.
> While there, I adjusted the return value of gettmpmnt() to return 0
> on success, and changed the checks according.

If gettmpmnt() always retruns -1 in the MNT_RDONLY case. We already
saw that that case was a problem before, but now it's even more
obvious. We have to decide if we want to allow for the R/O /tmp or not
and adapt the code.  I'd love it it would work. For that three three
cases (error, dir created, dir not created) still should be
distinguished in gettmpmnt() and the callers should be adapted.

Copying from a filestsem (as opposed to a dir) would be a problem,
though, since for that we need two mountpoints.

        -Otto

>
> Regards,
> Rafael Neves
>
>
> Index: sbin/newfs/newfs.c
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/newfs.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 newfs.c
> --- sbin/newfs/newfs.c 28 Jun 2019 13:32:45 -0000 1.112
> +++ sbin/newfs/newfs.c 8 Sep 2019 22:49:11 -0000
> @@ -147,7 +147,7 @@ struct disklabel *getdisklabel(char *, i
>  static void waitformount(char *, pid_t);
>  static int do_exec(const char *, const char *, char *const[]);
>  static int isdir(const char *);
> -static void copy(char *, char *);
> +static int copy(char *, char *);
>  static int gettmpmnt(char *, size_t);
>  #endif
>  
> @@ -515,8 +515,9 @@ havelabel:
>   if (mfs) {
>   struct mfs_args args;
>   char tmpnode[PATH_MAX];
> + int ret = 0;
>  
> - if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) == 0)
> + if (pop != NULL && gettmpmnt(tmpnode, sizeof(tmpnode)) != 0)
>   errx(1, "Cannot create tmp mountpoint for -P");
>   memset(&args, 0, sizeof(args));
>   args.base = membase;
> @@ -537,11 +538,15 @@ havelabel:
>   default:
>   if (pop != NULL) {
>   waitformount(tmpnode, pid);
> - copy(pop, tmpnode);
> + ret = copy(pop, tmpnode);
>   unmount(tmpnode, 0);
>   rmdir(tmpnode);
>   }
>   waitformount(node, pid);
> + if (ret != 0) {
> + unmount(node, 0);
> + exit(1);
> + }
>   exit(0);
>   /* NOTREACHED */
>   }
> @@ -740,48 +745,55 @@ isdir(const char *path)
>  {
>   struct stat st;
>  
> - if (stat(path, &st) != 0)
> - err(1, "cannot stat %s", path);
> - if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
> - errx(1, "%s: not a dir or a block device", path);
> + if (stat(path, &st) != 0) {
> + warn("cannot stat %s", path);
> + return (-1);
> + }
> + if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode)) {
> + warn("%s: not a dir or a block device", path);
> + return  (-1);
> + }
>   return (S_ISDIR(st.st_mode));
>  }
>  
> -static void
> +static int
>  copy(char *src, char *dst)
>  {
> - int ret, dir, created = 0;
> + int ret, dir;
>   struct ufs_args mount_args;
>   char mountpoint[MNAMELEN];
>   char *const argv[] = { "pax", "-rw", "-pe", ".", dst, NULL } ;
>  
> - dir = isdir(src);
> + if ((dir = isdir(src)) == -1)
> + return (-1);
> +
>   if (dir)
>   strlcpy(mountpoint, src, sizeof(mountpoint));
>   else {
> - created = gettmpmnt(mountpoint, sizeof(mountpoint));
> + if (gettmpmnt(mountpoint, sizeof(mountpoint)) != 0)
> + return (-1);
> +
>   memset(&mount_args, 0, sizeof(mount_args));
>   mount_args.fspec = src;
>   ret = mount(MOUNT_FFS, mountpoint, MNT_RDONLY, &mount_args);
>   if (ret != 0) {
>   int saved_errno = errno;
> - if (created && rmdir(mountpoint) != 0)
> + if (rmdir(mountpoint) != 0)
>   warn("rmdir %s", mountpoint);
> - if (unmount(dst, 0) != 0)
> - warn("unmount %s", dst);
> - errc(1, saved_errno, "mount %s %s", src, mountpoint);
> + warnc(saved_errno, "mount %s %s", src, mountpoint);
> + return (-1);
>   }
>   }
>   ret = do_exec(mountpoint, "/bin/pax", argv);
>   if (!dir && unmount(mountpoint, 0) != 0)
>   warn("unmount %s", mountpoint);
> - if (created && rmdir(mountpoint) != 0)
> + if (!dir && rmdir(mountpoint) != 0)
>   warn("rmdir %s", mountpoint);
>   if (ret != 0) {
> - if (unmount(dst, 0) != 0)
> - warn("unmount %s", dst);
> - errx(1, "copy %s to %s failed", mountpoint, dst);
> + warnx("copy %s to %s failed", mountpoint, dst);
> + return (-1);
>   }
> + return (0);
>  }
>  
>  static int
> @@ -792,28 +804,43 @@ gettmpmnt(char *mountpoint, size_t len)
>   struct statfs fs;
>   size_t n;
>  
> - if (statfs(tmp, &fs) != 0)
> - err(1, "statfs %s", tmp);
> + if (statfs(tmp, &fs) != 0) {
> + warn("statfs %s", tmp);
> + return (-1);
> + }
> +
>   if (fs.f_flags & MNT_RDONLY) {
> - if (statfs(mnt, &fs) != 0)
> - err(1, "statfs %s", mnt);
> - if (strcmp(fs.f_mntonname, "/") != 0)
> - errx(1, "tmp mountpoint %s busy", mnt);
> - if (strlcpy(mountpoint, mnt, len) >= len)
> - errx(1, "tmp mountpoint %s too long", mnt);
> - return (0);
> + if (statfs(mnt, &fs) != 0) {
> + warn("statfs %s", mnt);
> + return (-1);
> + }
> + if (strcmp(fs.f_mntonname, "/") != 0) {
> + warnx("tmp mountpoint %s busy", mnt);
> + return (-1);
> + }
> + if (strlcpy(mountpoint, mnt, len) >= len) {
> + warnx("tmp mountpoint %s too long", mnt);
> + return (-1);
> + }
> + return (-1);
>   }
>   n = strlcpy(mountpoint, tmp, len);
> - if (n >= len)
> - errx(1, "tmp mount point too long");
> + if (n >= len) {
> + warnx("tmp mount point too long");
> + return (-1);
> + }
>   if (mountpoint[n - 1] != '/')
>   strlcat(mountpoint, "/", len);
>   n = strlcat(mountpoint, "mntXXXXXXXXXX", len);
> - if (n >= len)
> - errx(1, "tmp mount point too long");
> - if (mkdtemp(mountpoint) == NULL)
> - err(1, "mkdtemp %s", mountpoint);
> - return (1);
> + if (n >= len) {
> + warnx("tmp mount point too long");
> + return (-1);
> + }
> + if (mkdtemp(mountpoint) == NULL) {
> + warn("mkdtemp %s", mountpoint);
> + return (-1);
> + }
> + return (0);
>  }
>  
>  #endif /* MFS */
>