kern/181155: [libc] [patch] *access*(2) does not handle invalid amodes properly

[Bruce Evans]

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