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.

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
Lint
Lint Skipped
Unit
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 accepted this revision.Jul 18 2019, 5:20 AM
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

Changed umask parsing to octal.

darius-dons.net.au marked an inline comment as done.Jul 18 2019, 6:23 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..

cem accepted this revision.Jul 18 2019, 6:48 PM
This revision is now accepted and ready to land.Jul 18 2019, 6:48 PM
jilles added a subscriber: jilles.Jul 18 2019, 9:00 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.