I wanted to use something like this for syncthing since it can't set its own umask and my patch to add such a feature was rejected by the maintainers.
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Since this review touches more than the man-page, you may want to talk to a few commiters about getting it reviewed by them. The easiest way to find people is to look at who touched these files last (ie. via svn/git blame). :)
src/usr.sbin/daemon/daemon.c | ||
---|---|---|
104 ↗ | (On Diff #59803) | Pre-existing, but it looks like l: is there twice. Maybe do a followup change which sorts and de-dupes this? |
Perhaps it will be more generically useful to add a ${name}_umask variable to rc.subr instead.
Ideally this would go via a login class but the ${name}_login_class variable is already used for "rlimits from this login class".
Definitely a generic way of achieving this would be nice, and it looks like you can set umask with a login class - to be honest if I had known about the "${name}_login_class" before now I would have used that rather than writing this patch.
You can set umask with a login class but not all ways to apply a login class set it; logins and init's daemon login class do but rc.subr's ${name}_login_class currently does not.
So I don't like ${name}_umask since that may block off fixing ${name}_login_class to apply the umask. However, you may consider using the functions from login_class(3) in daemon.
OK, I see that is because limits does not call setusercontext(... | LOGIN_UMASK) right?
So I don't like ${name}_umask since that may block off fixing ${name}_login_class to apply the umask. However, you may consider using the functions from login_class(3) in daemon.
Are you suggesting I should instead modify limits to add a flag so it will call setusercontext?
If so I am not sure what the scope would be, obviously just adding setusercontext(.. | LOGIN_UMASK) would be pretty straight forward but that seems incomplete.
I wrote two versions of a patch for limits. The first adds a '-M' option which when specified will set the umask on the command line or read the login class value (if -C is specified and no argument was used for -M). The second adds no option but sets the umask is -C is used.
I wrote the first one first but then realised it doesn't actually help my problem unless I modify rc.subr to pass -M to limits.
Anyone have a preference?
I think the second one is probably right but it's arguably a POLA violation.
Unfortunately in either case it is quite useless because various things afterward re-set the umask so it's a NOP.
I haven't quite tracked down what does it except that it's an sh and csh process (but hard to know why they did it)