Page MenuHomeFreeBSD

Add option to set umask before starting child process
AcceptedPublic

Authored by darius-dons.net.au on Jul 16 2019, 1:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 19 2024, 11:24 AM
Unknown Object (File)
Jan 11 2024, 11:53 PM
Unknown Object (File)
Dec 29 2023, 2:48 PM
Unknown Object (File)
Dec 10 2023, 8:52 PM
Unknown Object (File)
Dec 3 2023, 2:40 AM
Unknown Object (File)
Dec 1 2023, 7:20 PM
Unknown Object (File)
Oct 11 2023, 5:51 PM
Unknown Object (File)
Oct 11 2023, 9:05 AM

Details

Reviewers
cem
ian
Group Reviewers
manpages
Summary

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.

Test Plan

./daemon -M 002 touch foo

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

rpokala added inline comments.
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?

cem added inline comments.
src/usr.sbin/daemon/daemon.8
121–122 ↗(On Diff #59803)

Might be good to document the expected number format.

src/usr.sbin/daemon/daemon.c
141 ↗(On Diff #59803)

Any reason to use 0 instead of 8? I'm having a hard time imagining hex or decimal modes being useful.

This revision is now accepted and ready to land.Jul 18 2019, 5:20 AM
This revision now requires review to proceed.Jul 18 2019, 6:19 AM
darius-dons.net.au added inline comments.
src/usr.sbin/daemon/daemon.c
104 ↗(On Diff #59803)

Well spotted, I created D20983 for this and put you on it :)

141 ↗(On Diff #59803)

I suppose not, I've updated that just now.

darius-dons.net.au marked an inline comment as done.

Sorry for the noise, I can't drive Git..

This revision is now accepted and ready to land.Jul 18 2019, 6:48 PM

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".

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.

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.

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.

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.

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.

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.

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)