From owner-freebsd-standards@FreeBSD.ORG  Fri Apr 29 22:30:31 2011
Return-Path: <owner-freebsd-standards@FreeBSD.ORG>
Delivered-To: freebsd-standards@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34])
	by hub.freebsd.org (Postfix) with ESMTP id 54ABA1065672
	for <freebsd-standards@freebsd.org>;
	Fri, 29 Apr 2011 22:30:31 +0000 (UTC) (envelope-from jilles@stack.nl)
Received: from mx1.stack.nl (relay04.stack.nl [IPv6:2001:610:1108:5010::107])
	by mx1.freebsd.org (Postfix) with ESMTP id E6CED8FC17
	for <freebsd-standards@freebsd.org>;
	Fri, 29 Apr 2011 22:30:30 +0000 (UTC)
Received: from turtle.stack.nl (turtle.stack.nl [IPv6:2001:610:1108:5010::132])
	by mx1.stack.nl (Postfix) with ESMTP id 016931DD415;
	Sat, 30 Apr 2011 00:30:29 +0200 (CEST)
Received: by turtle.stack.nl (Postfix, from userid 1677)
	id DDF5C173FE; Sat, 30 Apr 2011 00:30:29 +0200 (CEST)
Date: Sat, 30 Apr 2011 00:30:29 +0200
From: Jilles Tjoelker <jilles@stack.nl>
To: Sergey Kandaurov <pluknet@gmail.com>
Message-ID: <20110429223029.GB10840@stack.nl>
References: <BANLkTi=Uzo1FE9O0pvdr2zGcf_SAz=auMA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BANLkTi=Uzo1FE9O0pvdr2zGcf_SAz=auMA@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: freebsd-standards@freebsd.org
Subject: Re: O_ACCMODE doesn't respect O_EXEC
X-BeenThere: freebsd-standards@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: Standards compliance <freebsd-standards.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-standards>, 
	<mailto:freebsd-standards-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-standards>
List-Post: <mailto:freebsd-standards@freebsd.org>
List-Help: <mailto:freebsd-standards-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-standards>, 
	<mailto:freebsd-standards-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Fri, 29 Apr 2011 22:30:31 -0000

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