Add recursive flags to setfacl(1)

[mhorne063_gmail.com]

Summary

Add a -R option to setfacl to operate recursively on directories, along with the accompanying flags -H, -L, and -P (whose behaviour mimics chmod).

A patch for this feature was previously submitted in D9096 which used nftw to traverse a file hierarchy, but as was discussed there, fts is slightly more suitable so this is a fresh implementation using that.

[mhorne063_gmail.com]

fts_open() requires an array of strings as an argument, so I had to modify the way input is handled from stdin. It now adds the filename inputs to an array which reallocs when necessary -- an approach that doesn't seem particularly elegant to me, but I thought was preferable to calling fts_open() on each input file individually. If anyone wants to weigh in on this choice I'd gladly hear it.

setfacl.c could also benefit from having its 350+ line main function broken up a bit. I can do this, I'm just unsure if it would be better to include it as part of this patch or a follow-up. I want to keep the changes readable.

[emaste]

bin/setfacl/setfacl.c

220 ↗

(On Diff #40993)

bytes or array entries?

[mhorne063_gmail.com]

bin/setfacl/setfacl.c

220 ↗

(On Diff #40993)

Entries. Is FL_BASE_ENTRIES better?

[emaste]

bin/setfacl/setfacl.c

220 ↗

(On Diff #40993)

but zmalloc() takes bytes

[jilles]

I like the idea. Apparently, the idea of the designers of setfacl was to pipe in a list of filenames from find, but this cannot handle filenames containing newlines and race conditions (such as where an untrusted user replaces a file with a symlink to a different location).

In D14934#314043, @mhorne063_gmail.com wrote:

fts_open() requires an array of strings as an argument, so I had to modify the way input is handled from stdin. It now adds the filename inputs to an array which reallocs when necessary -- an approach that doesn't seem particularly elegant to me, but I thought was preferable to calling fts_open() on each input file individually. If anyone wants to weigh in on this choice I'd gladly hear it.

It created a data structure containing all specified filenames up front and still does so (perhaps a better data structure, even). Therefore, this is not a regression and need not be changed for commit.

setfacl.c could also benefit from having its 350+ line main function broken up a bit. I can do this, I'm just unsure if it would be better to include it as part of this patch or a follow-up. I want to keep the changes readable.

This can be done separately. The now more complicated logic whether to follow symlinks or not can be extracted out in this patch.

bin/setfacl/setfacl.c

292 ↗

(On Diff #40993)

In -RH mode, symlinks should be followed for files with file->fts_level == FTS_ROOTLEVEL. See rS282208.

As a more stylistic issue, logical operations on booleans should be && and || unless there is a pressing reason to use & and |.

[emaste]

In D14934#314043, @mhorne063_gmail.com wrote:

setfacl.c could also benefit from having its 350+ line main function broken up a bit. I can do this, I'm just unsure if it would be better to include it as part of this patch or a follow-up. I want to keep the changes readable.

It should be a separate change. All else being equal I'd generally have a preference for doing non-functional changes first (if this work wasn't already done).

[mhorne063_gmail.com]

Fixed issue with reallocation size

Adjust symlink logic for -H case

[jilles]

bin/setfacl/setfacl.c

224–226 ↗

(On Diff #41047)

Hmm, do we want to check for overflow here? Note that this may not matter since address space limitations are likely to cause it to fail before overflow happens.

267–273 ↗

(On Diff #41047)

This allows setting default ACL on files that are neither regular files nor directories.

Looking further, it seems like none of the code cares about any stat information other than whether the file is a directory or not. Therefore, you could use FTS_NOSTAT and get a minor performance boost of -R mode.

333–335 ↗

(On Diff #41047)

The name here is passed to be printed in error messages, not to be passed as pathname to system calls (in which case some form of follow_symlink check would have been necessary). Therefore, it seems like this should be file->fts_path.

The same applies to various more calls below to the functions defined in mask.c, merge.c and remove.c.

[mhorne063_gmail.com]

bin/setfacl/setfacl.c

224–226 ↗

(On Diff #41047)

Seems incredibly unlikely that it could ever reach enough filenames to overflow, but the cost of adding the check is small so I might as well.

[jilles]

I found two minor issues. It looks good otherwise.

bin/setfacl/setfacl.c

408 ↗

(On Diff #41302)

set_acl_mask() uses the filename for error messages, so this should be file->fts_path. The other instances of file->fts_accpath and file->fts_path seem correct.

224–226 ↗

(On Diff #41047)

OK. Now that UINT_MAX is visible here, it seems clearer that the types of the variables should be changed to size_t and UINT_MAX to SIZE_MAX, even though it is unlikely that more than 500 million pathnames will be in the file (on 64-bit systems). Note that fts_open uses size_t for counting as well.

[mhorne063_gmail.com]

Change to fix last review comments:

• Use size_t instead of u_int
• Provide proper path to set_acl_mask()

[emaste]

Minor style(9) comment nits to be addressed upon commit

bin/setfacl/setfacl.1

29 ↗

(On Diff #41353)

.Dd to be updated upon commit

bin/setfacl/setfacl.c

214 ↗

(On Diff #41353)

Comments should generally be full sentences, with a . at the end.