Page MenuHomeFreeBSD

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

Authored by ngie on Aug 24 2014, 7:44 AM.

Details

Reviewers
rpaulo
jmmv
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

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

ngie updated this revision to Diff 1241.Aug 24 2014, 7:44 AM
ngie retitled this revision from to Validate the mode argument in access, eaccess, and faccessat for POSIX compliance.
ngie updated this object.
ngie added reviewers: jmmv, rpaulo.
ngie added subscribers: jilles, wollman.

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.

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 added a comment.Aug 25 2014, 5:32 AM
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 retitled this revision from Validate the mode argument in access, eaccess, and faccessat for POSIX compliance to Validate the mode argument in access, eaccess, and faccessat for optional POSIX compliance and to improve compatibility with Linux and NetBSD.Aug 25 2014, 5:35 AM
ngie updated this object.

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

jmmv accepted this revision.Sep 8 2014, 2:40 PM
jmmv edited edge metadata.

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.

This revision is now accepted and ready to land.Sep 8 2014, 2:40 PM
ngie added a comment.Sep 11 2014, 10:27 PM
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 added a comment.Sep 11 2014, 10:29 PM
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 added a comment.Sep 15 2014, 7:03 PM

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 added a comment.Sep 15 2014, 7:04 PM
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 closed this revision.Sep 16 2014, 1:07 AM

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 :)!