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 */
>
