Page MenuHomeFreeBSD

Remove NFSv4 inheritance flags in setfacl recursive mode
ClosedPublic

Authored by mhorne063_gmail.com on Apr 13 2018, 6:41 PM.

Details

Summary

When modifying ACLs recursively, strip the inheritance flags from each of the ACL entries.

This code was extracted from the patch originally submitted to PR 155163

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

No code changes, just updated from changes made to D15060

emaste added inline comments.Apr 27 2018, 5:28 PM
bin/setfacl/setfacl.c
129 ↗(On Diff #41921)

This should probably have a comment to give more information about what is meant by "sanitize".

153–155 ↗(On Diff #41921)

style(9) puts the operator at the end of the line when wrapping is necessary

emaste added a subscriber: trasz.Apr 27 2018, 6:21 PM

It has been quite a few years since I originally wrote this code. This might be able to carve out some time these next two weeks to refactor it. There are parts of this patch that feel a bit awkward and could likely be improved upon.

Part of me wonders if the inheritance sanitization code should live in libc with the rest of the ACL code. Doing so would allow other projects that deal with ACLs to take advantage of it.

mhorne063_gmail.com updated this revision to Diff 49000.EditedOct 11 2018, 4:43 AM
mhorne063_gmail.com edited the summary of this revision. (Show Details)

Cleaned this up a bit. The logic is the same but should be a bit more obvious now.

Unless I missed something there was no reason to be calling acl_dup() like we did in the last iteration of the patch.

Also made it more obvious that we were ignoring return values by casting them to void

mhorne063_gmail.com marked 2 inline comments as done.Oct 11 2018, 4:45 AM
markj added a reviewer: trasz.Oct 26 2018, 3:23 PM
markj added a subscriber: markj.
markj added inline comments.Oct 26 2018, 3:58 PM
bin/setfacl/setfacl.c
143 ↗(On Diff #49000)

The callers are already checking R_flag, this is redundant.

154 ↗(On Diff #49000)

Don't we also need to call acl_set_flagset_np()? This call is just modifying a stack-local variable.

markj added inline comments.Oct 26 2018, 4:07 PM
bin/setfacl/setfacl.c
131 ↗(On Diff #49000)

This should probably be documented in the man page.

149 ↗(On Diff #49000)

acl_flagset will be left uninitialized if an error occurs. This may not be possible in practice, but we should handle it anyway.

markj added inline comments.Oct 26 2018, 5:30 PM
bin/setfacl/setfacl.c
154 ↗(On Diff #49000)

Ignore this, I misunderstood the API: acl_flagset_t is a uint16_t *, not a uint16_t.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 26 2018, 9:17 PM
This revision was automatically updated to reflect the committed changes.
mhorne063_gmail.com added a comment.EditedOct 26 2018, 9:39 PM

Ahh I see, I misunderstood the purpose of duplicating the ACL. Much better now.

Thanks Mark for being so quick to fix this up and commit.