From owner-freebsd-bugs@FreeBSD.ORG Fri Apr 18 20:37:36 2014 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id DA71A4EB for ; Fri, 18 Apr 2014 20:37:36 +0000 (UTC) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 6C6F61C4B for ; Fri, 18 Apr 2014 20:37:35 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 2E01642684D; Sat, 19 Apr 2014 06:37:28 +1000 (EST) Date: Sat, 19 Apr 2014 06:37:27 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Garrett Cooper Subject: Re: kern/181155: [libc] [patch] *access*(2) does not handle invalid amodes properly In-Reply-To: <201404181730.s3IHU1GB027144@freefall.freebsd.org> Message-ID: <20140419052844.O3643@besplex.bde.org> References: <201404181730.s3IHU1GB027144@freefall.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=QIpRGG7L c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=mgrFXIbrn8sA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=uZvujYp8AAAA:8 a=pzTw19-mZjC3powR0A4A:9 a=CjuIK1q_8ugA:10 a=LNDHkyYRo8IA:10 Cc: freebsd-bugs@freebsd.org X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Apr 2014 20:37:37 -0000 On Fri, 18 Apr 2014, Garrett Cooper wrote: > And one more thing: fixing this will make FreeBSD more POSIX > compliant with this system call ( > http://pubs.opengroup.org/onlinepubs/009695299/functions/access.html > ), will match the behavior on Linux and NetBSD at least: > > The access() function may fail if: > > [EINVAL]The value of the amode argument is invalid. FreeBSD's access() is already conformant. It is applications that don't conform if they pass an invalid arg. I think the behaviour for such args is undefined in both POSIX and FreeBSD, and the above clause doesn't change this, so the fuzzy ``may'' in it is more useless than usually. Both POSIX and FreeBSD say that the amode arg _is_ either the OR of 3 flags or the possibly-non-flag F_OK. When it isn't one of these, the behaviour is implicitly undefined and I think it takes explicit wording to change this. Such wording would not be useful. The errno clause is even less useful without out it. The undefined behaviour already includes failing with errno EINVAL, but conforming applications cannot assume anything. The patch in the PR is apparently too simple to detect all invalid args. It allows F_OK ORed with the other flags. However, F_OK isn't a flag. It is the special value 0. Thus ORing F_OK has no effect, and the check works accidentally. This is confusing. The code that uses F_OK is even more confusing, except a comment makes it less confusing. F_OK is _never_ used explictly in vfs_syscalls.c. It is spelled as 0 in the comment and as in an obfuscated flags test: % /* Flags == 0 means only check for existence. */ This magic 0 is the non-flag F_OK. % error = 0; % if (user_flags) { This if clause covers the non-F_OK case. In the F_OK case, we will just return 0 (existence is checked elsewhere). Both cases are obfuscated by not returning early for the easy case. The amode check probably belongs here (don't bother optimizing away namei stuff for invalid amode). The name 'user_flags' is bad. Related variables are named *mode everywhere else, and we don't even have flags until reaching this point, since F_OK is not a flag. `user_flags' is also verbose. This bad name dates from before 4.4BSD-Lite, where the arg was named 'mode' in the man page but 'flags' in the kernel for the syscall and the local variable was named 'flags' to match the syscall in the kernel. FreeBSD eventually switched to the POSIX name 'amode' for the syscall in the kernel. However, the man page has not been updated, so it is inconsistent in a different way. % accmode = 0; % if (user_flags & R_OK) % accmode |= VREAD; % if (user_flags & W_OK) % accmode |= VWRITE; % if (user_flags & X_OK) % accmode |= VEXEC; % #ifdef MAC % error = mac_vnode_check_access(cred, vp, accmode); % if (error != 0) % return (error); % #endif % if ((accmode & VWRITE) == 0 || (error = vn_writechk(vp)) == 0) % error = VOP_ACCESS(vp, accmode, cred, td); % } % return (error); Removing the obfuscations (except the name 'user_flags') gives something like: %%% /* No longer any comment or pre-initializtion of 'error' here. */ if (user_flags == F_OK) return (0); /* The existence check is elsewhere. */ if ((user_flags & ~(R_OK | W_OK | X_OK)) != 0) return (EINVAL); accmode = 0; if (user_flags & R_OK) accmode |= VREAD; if (user_flags & W_OK) accmode |= VWRITE; if (user_flags & X_OK) accmode |= VEXEC; #ifdef MAC error = mac_vnode_check_access(cred, vp, accmode); if (error != 0) return (error); #endif if ((accmode & VWRITE) == 0 || (error = vn_writechk(vp)) == 0) error = VOP_ACCESS(vp, accmode, cred, td); return (error); %%% Bruce