On Sun, 03 May 2015 22:07:33 -0700, Philip Guenther wrote:
> The flags passed to open(2) and openat(2) are not a simple bitset.
> Instead, the bits in O_ACCMODE are effectively an enumeration, and the
> other bits are or'ed onto that.
>
> For example, a function that wraps open(2), taking a flag argument that it
> passes through, that wants to verify that it was invoked with either
> O_WRONLY or O_RDWR *cannot* just say
> if ((flags & O_ACCMODE) & ~(O_WRONLY | O_RDWR))
> return (EINVAL);
>
> as that will accept O_RDONLY as well! The correct test is to mask off
> O_ACCMODE and compare the results to the acceptable values:
> if ((flags & O_ACCMODE) != O_WRONLY && (flags & O_ACCMODE) != O_RDWR)
> return (EINVAL);
>
>
> So, in anticipation of future changes, let's make interfaces that wrap
> open() be more precise in the handling of flags for open(), testing
> O_ACCMODE bits separately from others.
>
> While here, document that shm_open() accept O_CLOEXEC and O_NOFOLLOW as
> extensions to POSIX, and use O_CLOEXEC on the temporary fd in
> posix_openpt()
A few comments inline:
> Index: libc/db/db/db.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/db/db/db.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 db.c
> --- libc/db/db/db.c 5 Aug 2005 13:03:00 -0000 1.10
> +++ libc/db/db/db.c 4 May 2015 04:48:11 -0000
> @@ -48,9 +48,10 @@ dbopen(const char *fname, int flags, int
> #define DB_FLAGS (DB_LOCK | DB_SHMEM | DB_TXN)
> #define USE_OPEN_FLAGS
> \
> (O_CREAT | O_EXCL | O_EXLOCK | O_NOFOLLOW | O_NONBLOCK | \
> - O_RDONLY | O_RDWR | O_SHLOCK | O_SYNC | O_TRUNC)
> + O_SHLOCK | O_SYNC | O_TRUNC)
>
> - if ((flags & ~(USE_OPEN_FLAGS | DB_FLAGS)) == 0)
> + if (((flags & O_ACCMODE) == O_RDONLY || (flags & O_ACCMODE) == O_RDWR)
> + && (flags & ~(O_ACCMODE | USE_OPEN_FLAGS | DB_FLAGS)) == 0)
> switch (type) {
> case DB_BTREE:
> return (__bt_open(fname, flags & USE_OPEN_FLAGS,
> Index: libc/gen/shm_open.3
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/shm_open.3,v
> retrieving revision 1.4
> diff -u -p -r1.4 shm_open.3
> --- libc/gen/shm_open.3 8 Jul 2014 00:40:56 -0000 1.4
> +++ libc/gen/shm_open.3 4 May 2015 04:48:11 -0000
> @@ -45,7 +45,7 @@ and must include at least
> or
> .Dv O_RDWR
> and may also include a combination of
> -.Dv O_CREAT , O_EXCL ,
> +.Dv O_CREAT , O_EXCL , O_CLOEXEC , O_NOFOLLOW ,
> or
> .Dv O_TRUNC .
> This implementation forces the
> @@ -81,6 +81,13 @@ and
> .Fn shm_unlink
> appear in
> .St -p1003.1-2001 .
> +Using
> +.Dv O_CLOEXEC
> +or
> +.Dv O_NOFOLLOW
> +with
> +.Fn shm_open
> +is an extension to that standard.
> This implementation deviates from the standard by permitting less sharing.
> .Pp
> .Fn shm_mkstemp
> Index: libc/gen/shm_open.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/shm_open.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 shm_open.c
> --- libc/gen/shm_open.c 12 Nov 2013 06:09:48 -0000 1.4
> +++ libc/gen/shm_open.c 4 May 2015 04:48:11 -0000
> @@ -31,6 +31,9 @@
> /* "/tmp/" + sha256 + ".shm" */
> #define SHM_PATH_SIZE (5 + SHA256_DIGEST_STRING_LENGTH + 4)
>
> +/* O_CLOEXEC and O_NOFOLLOW are extensions to POSIX */
> +#define OK_FLAGS (O_CREAT | O_EXCL | O_TRUNC | O_CLOEXEC | O_NOFOLLOW)
> +
> static void
> makeshmpath(const char *origpath, char *shmpath, size_t len)
> {
> @@ -47,8 +50,8 @@ shm_open(const char *path, int flags, mo
> struct stat sb;
> int fd;
>
> - if (flags & ~(O_RDONLY | O_RDWR |
> - O_CREAT | O_EXCL | O_TRUNC | O_CLOEXEC | O_NOFOLLOW)) {
> + if (((flags & O_ACCMODE) != O_RDONLY && (flags & O_ACCMODE) != O_RDWR)
> + || (flags & ~(O_ACCMODE | OK_FLAGS))) {
> errno = EINVAL;
> return -1;
> }
> Index: libc/stdlib/posix_pty.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/posix_pty.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 posix_pty.c
> --- libc/stdlib/posix_pty.c 3 Dec 2012 20:08:33 -0000 1.1
> +++ libc/stdlib/posix_pty.c 4 May 2015 04:48:11 -0000
> @@ -35,13 +35,14 @@ posix_openpt(int oflag)
> int fd, mfd = -1;
>
> /* User must specify O_RDWR in oflag. */
> - if (!(oflag & O_RDWR)) {
> + if ((oflag & O_ACCMODE) != O_RDWR ||
> + (oflag & ~(O_ACCMODE | O_NOCTTY)) != 0) {
> errno = EINVAL;
> return -1;
> }
I don't see the need to use O_ACCMODE here at all. Why not just:
if (!(oflag & O_RDWR) || (oflag & ~(O_RDWR | O_NOCTTY)) != 0)
or even:
if (oflag != O_RDWR && oflag != (O_RDWR | O_NOCTTY))
> /* Get pty master and slave (this API only uses the master). */
> - fd = open(PATH_PTMDEV, O_RDWR);
> + fd = open(PATH_PTMDEV, O_RDWR | O_CLOEXEC);
> if (fd != -1) {
> if (ioctl(fd, PTMGET, &ptm) != -1) {
> close(ptm.sfd);
> Index: libkvm/kvm.c
> ===================================================================
> RCS file: /cvs/src/lib/libkvm/kvm.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 kvm.c
> --- libkvm/kvm.c 16 Jan 2015 16:48:51 -0000 1.54
> +++ libkvm/kvm.c 4 May 2015 04:48:11 -0000
> @@ -198,7 +198,8 @@ _kvm_open(kvm_t *kd, const char *uf, con
> _kvm_err(kd, kd->program, "exec file name too long");
> goto failed;
> }
> - if (flag & ~O_ACCMODE) {
> + if ((flag & ~O_ACCMODE) || (flag != O_RDONLY && flag != O_WRONLY &&
> + flag != O_RDWR)){
The check against O_ACCMODE seems superfluous here since you added
the explicit flag value checks.
> _kvm_err(kd, kd->program, "bad flags arg");
> goto failed;
> }
>
