Validate the mode argument in access, eaccess, and faccessat for optional POSIX compliance and to improve compatibility with Linux and NetBSD

[ngie]

Summary

Validate the mode argument in access, eaccess, and faccessat for optional POSIX compliance and to improve compatibility with Linux and NetBSD

The issue was identified with lib/libc/sys/t_access:access_inval from NetBSD

Phabric: D678
PR: 181155
Sponsored by: EMC / Isilon Storage Division

[jilles]

Although this change makes sense, it is not required for POSIX compliance. Note that the [EINVAL] error for an invalid amode is in the "may fail" section. One reason for this is that implementation extensions or new versions of POSIX may add new amode values.

Be aware that this might break non-compliant applications that pass invalid amode values.

[wollman]

As Jilles says, there's no requirement to have this check. But since access() is not called very frequently, adding one branch seems unlikely to hurt.

[ngie]

In D678#4, @jilles wrote:

Although this change makes sense, it is not required for POSIX compliance. Note that the [EINVAL] error for an invalid amode is in the "may fail" section. One reason for this is that implementation extensions or new versions of POSIX may add new amode values.

Good point. I'll adjust the text.

Be aware that this might break non-compliant applications that pass invalid amode values.

Does this deserve a ports -exp run? Most of the code written to work with Linux and NetBSD should have been fixed to not pass in invalid values.

[ngie]

This also requires a manpage update... I missed that.

[jmmv]

The change looks good to me and the other more-expert reviewers in the topic don't disagree to this, but I wonder how this has been tested. Accepting on the grounds that proper testing has been done.

[ngie]

In D678#10, @jmmv wrote:

The change looks good to me and the other more-expert reviewers in the topic don't disagree to this, but I wonder how this has been tested. Accepting on the grounds that proper testing has been done.

Yup. I've tested out different variations of this patch; the test command was t_access from NetBSD -- which isn't available in FreeBSD yet, but hopefully will be in the next couple months once I do the cat herding required for consolidating tools/regression/lib/libc and src/tests/lib/libc from NetBSD.

[ngie]

In D678#13, @ngie wrote:

In D678#10, @jmmv wrote:

The change looks good to me and the other more-expert reviewers in the topic don't disagree to this, but I wonder how this has been tested. Accepting on the grounds that proper testing has been done.

Yup. I've tested out different variations of this patch; the test command was t_access from NetBSD -- which isn't available in FreeBSD yet, but hopefully will be in the next couple months once I do the cat herding required for consolidating tools/regression/lib/libc and src/tests/lib/libc from NetBSD.

Also, the first attempt I made at fixing this a year and a half ago ended pretty horribly, but was also quite humorous (I inverted the conditional). Needless to say.. the handoff from PID = 0 to PID = 1 (init) didn't go too well :).

[ngie]

Ok. I've tested out the changes in one of my project branches. I'll commit the code and the manpage diffs to head after lunch :).

[ngie]

In D678#15, @ngie wrote:

Ok. I've tested out the changes in one of my project branches. I'll commit the code and the manpage diffs to head after lunch :).

"tested out" -> "tested out the commit operations" :). I need to update sys/sys/param.h as well.

[ngie]

I committed the code change along with the manpage content change in r271655. I committed the other bookkeeping items (bumping .Dd, __FreeBSD_version) in r271656 and r271657, respectively to ease MFCing [without conflicts]. Thank you very much for the review :)!