On Fri, 19 Sep 2014 00:14:47 -0700 Doug Hogan <[email protected]> wrote:
> On Thu, Sep 18, 2014 at 09:28:41AM +0100, [email protected] > wrote: > > Revised diffbelow: > > You are inheriting some bugs in here by reusing newfs code. :) > > I agree with the comments from others. Additional comments: > > > #include <util.h> > > +#include <paths.h> > ... > > + args.ta_root_mode = modeset ? mode : sb.st_mode; > > + > > + int newflags = mntflags; > ... > > + if(template) newflags &= ~MNT_RDONLY; > ... > > + if(template) { > > KNF > > > +#include "pathnames.h" > > This was too much copy/paste from newfs. You aren't using any of > the definitions in here. Sorted. > > > + if (execv(cmd, argv) != 0) > > + err(1, "%s", cmd); > > execv() does not return when successful. On error, it returns -1. Hence the need for the code. The check appears to be more of a semantics thin to me. Yes, it will not return if successful, but if it is not successful, there should still be some code to handle this case. At least here, it looks like a typical error handling "if" statement rather than just "Do this, but act like there's an error whether or not we are successful." > > > + strlcpy(mountpoint, src, sizeof(mountpoint)); > > The strlcpy call should check for truncation. There's no guarantee > that src is less than sizeof(mountpoint) since src = template = > optarg. According to man strlcpy(3): "strlcpy() copies up to dstsize - 1 characters from the string src to dst, NUL-terminating the result if dstsize is not 0." Thus, such a check here would be redundant. > > + tmp = getenv("TMPDIR"); > > + if (tmp == NULL || *tmp == '\0') > > + tmp = _PATH_TMP; > > It's being used with mkdtemp(), but I think you should still check > issetugid() before trusting getenv(). Looking into this now. > > + if (mntflags & MNT_RDONLY) { > + mntflags |= MNT_UPDATE; > + if (mount(MOUNT_TMPFS, canon_dir, mntflags, > &args) < 0) { > > This doesn't look right. /usr/src/sys/tmpfs/tmpfs_vfsops.c doesn't > support MNT_UPDATE in tmpfs_mount(). I have added some code to allow updating of MNT_RDONLY, and it also responds to MNT_WANTRDWR by unsetting MNT_RDONLY. As I see from tmpfs_vnops.c as well, the MNT_RDONLY flag is used here to prevent writing to files. There also appears to be a bug here: it appears to be possible to delete files from a "read-only" tmpfs file system. > > I don't see a reason for canon_dev to exist. It gets written to but > isn't used anywhere else. I am going to fix this in another diff since Otto suggested parsing changes should be kept separate from functionality.
