Date: Sat, 19 Apr 2014 06:37:27 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <yaneurabeya@gmail.com> Cc: freebsd-bugs@freebsd.org Subject: Re: kern/181155: [libc] [patch] *access*(2) does not handle invalid amodes properly Message-ID: <20140419052844.O3643@besplex.bde.org> In-Reply-To: <201404181730.s3IHU1GB027144@freefall.freebsd.org> References: <201404181730.s3IHU1GB027144@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <empty> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140419052844.O3643>