O_ACCMODE doesn't respect O_EXEC

[Jilles Tjoelker]

On Fri, Apr 29, 2011 at 04:51:38PM +0400, Sergey Kandaurov wrote:
> As of now, O_ACCMODE is a conjunction of O_{RDONLY,WRONLY,RDWR} flags
> as it was from the beginning. Its definition hasn't changed with
> addition of O_EXEC.

> POSIX states:
> O_ACCMODE - Mask for file access modes.

> O_EXEC falls into this access modes category, so I think O_ACCMODE
> should include O_EXEC.

I agree.

> This may require review of O_ACCMODE usage throughout the kernel, though..

> This simple test demonstrates the problem:

> flags: 0x0
> flags: 0x0
> flags: 0x1
> flags: 0x1
> flags: 0x2
> flags: 0x2
> flags: 0x3ffff
> flags: 0x3
> ^^ decremented due OFLAGS macro that also doesn't respect new access
> mode flag(s)
> ^^ correct version would show 0x40000

Indeed. kern_openat() has a hack that does not apply the FFLAGS macro if
O_EXEC is specified, to work around FFLAGS/OFLAGS not knowing O_EXEC.
Instead, it seems more appropriate to use something like

#define FFLAGS(oflags) (((oflags) & O_EXEC) != 0 ? (oflags) : (oflags) + 1)
#define OFLAGS(fflags) (((fflags) & O_EXEC) != 0 ? (fflags) : (fflags) - 1)

With also the O_ACCMODE change, kern_openat()'s check can be changed to

	switch (flags & O_ACCMODE) {
		case O_RDONLY:
		case O_WRONLY:
		case O_RDWR:
		case O_EXEC:
			break;
		default:
			return (EINVAL);
	}

with an unconditional flags = FFLAGS(flags);

> int
> main(void)
> {
> 
>         test_with(O_RDONLY);
>         test_with(O_WRONLY);
>         test_with(O_RDWR);
>         test_with(O_EXEC);
> 
>         return (0);
> }
> 
> static void
> test_with(int openflag)
> {
>         int fd, flags;
> 
>         if ((fd = open("file", openflag)) < 0)
>                 err(1, "open");
>         if ((flags = fcntl(fd, F_GETFL)) < 0)
>                 err(1, "fcntl(fd, GETFL)");
>         printf("flags: 0x%x\n", flags);
>         printf("flags: 0x%x\n", flags&O_ACCMODE);
>         close(fd);
> }

-- 
Jilles Tjoelker