preserve inherited umask on autodaemonize#384
Open
tomfitzhenry wants to merge 1 commit into
Open
Conversation
When shpool autodaemonizes, it uses the `daemonize` crate to detach
the daemon process. By default, the `daemonize` crate overrides the
process umask to `0o027`.
This causes the daemon (and all shells it subsequently spawns) to run
with umask `0027`, even if the user launched `shpool attach` with
another umask (e.g. `0022`). I noticed this when I saw some
tools/workflows break only in shpool.
This change fixes the issue by inherting the umask. The parent is
the correct place to decide on umask.
Steps to reproduce:
```
$ umask
0022
$ killall shpool; shpool --socket $(mktemp) attach --cmd "bash -c 'umask; sleep 1s'" foo
0027
```
With this commit:
```
$ umask
0022
$ cargo run -- --socket $(mktemp) attach --cmd "bash -c 'umask; sleep 1s'" foo
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s
Running `target/debug/shpool --socket /tmp/tmp.POFOUoQDt2 attach --cmd 'bash -c '\''umask; sleep 1s'\''' foo`
0022
```
Contributor
|
Oh good find, thanks! I'm actually abuot to rip out the daemonize crate (#372) so let's not merge this as is, but let's make sure that the direct impl does the right thing here. One thing you can do right now (which I'll want before merging anyway) is add a regression test for this in shpool/tests/regression.rs that fails without you patch applies. We can then make sure that the new daemonization approach does the right thing. |
Contributor
|
Oh, also as a workaround for now you should be able to just run with systemd. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI Policy Ack
Please ack that you have read the AI Policy
and explain your use of AI to generate this PR.
Ack. An LLM diagnosed this issue when I prompted it to find why the umask was 027 only in shpool. It found it, and generated a fix. Its initial attempt used
unsafe, but when prompted, it switched to using thenixcrate. I readdaemonize's docs re default mode. I tidied up the code (removed verbose comments, improved variable name).This PR was:
Description
When shpool autodaemonizes, it uses the
daemonizecrate to detach the daemon process. By default, thedaemonizecrate overrides the process umask to0o027.This causes the daemon (and all shells it subsequently spawns) to run with umask
0027, even if the user launchedshpool attachwith another umask (e.g.0022). I noticed this when I saw some tools/workflows break only in shpool.This change fixes the issue by inherting the umask. The parent is the correct place to decide on umask.
Steps to reproduce:
With this commit: