Add a way to set the signal mask of a child process

[sunshowers]

Proposal

Currently, when Rust spawns a process on Unix, it always sets the signal mask to the empty set.

• posix_spawn: https://github.com/rust-lang/rust/blob/7fe2c4b00dfbc33643e1af4b293eb057306a8339/library/std/src/sys/unix/process/process_unix.rs#L532-L539
• fork/exec: https://github.com/rust-lang/rust/blob/7fe2c4b00dfbc33643e1af4b293eb057306a8339/library/std/src/sys/unix/process/process_unix.rs#L340

This proposal adds a way to set the signal mask for a process.

Motivation, use-cases

There are a variety of use cases for signal masks, but my own use case is to block SIGTSTP and other signals, to work around a flaw in the definition of posix_spawn. This is currently not possible to do with the Rust std::process::Command APIs.

Solution sketches

Proposed API, using the suggestion by @zackw in this comment:

pub trait CommandExt: Sealed {
    // <...snip: existing methods here...>

    fn block_signal(&mut self, signal: i32) -> io::Result<&mut std::process::Command>;
    fn unblock_signal(&mut self, signal: i32) -> io::Result<&mut std::process::Command>;
    fn will_block_signal(&self, signal: i32) -> io::Result<&mut std::process::Command>;
}

I put up a PR with this API:

• Support setting signal masks for child processes on Unix rust#100737

Questions:

• Should the io::Errors returned by these methods be deferred until spawn()? I think doing it immediately is better to indicate errors (for invalid signals) immediately.
• Should CommandExt::signal_mask be marked unsafe? See discussion here.

Future directions:

• This can be extended to support setsigdefault as well. Currently we always set SIGPIPE.

Links and related work

Post in the internals group: https://internals.rust-lang.org/t/add-a-way-to-set-the-signal-mask-of-a-child-process/17220

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

[thomcc]

I wrote a concern about the safety of this here rust-lang/rust#100737 (comment). I don't think its a big issue, but I do want to make a note of it here.

It's definitely something we should be sure about prior to whenever we stabilize this, but I can't see a way for it to be unsound at the moment (but I have not thought about this for very long), so I don't have any objections to it being safe.

[thomcc]

Because it's not written out here, the API surface being proposed in the PR is:

// Inside `std::os::unix::process`

pub trait CommandExt: Sealed {
    // <...snip: existing methods here...>

    fn signal_mask(&mut self, mask: impl Into<Option<SignalSet>>) -> &mut std::process::Command;
}

pub struct SignalSet { .. }

impl SignalSet {
    pub fn empty() -> std::io::Result<Self>;
    pub fn filled() -> std::io::Result<Self>;
    pub fn insert(&mut self, signal: i32) -> std::io::Result<&mut Self>;
    pub fn remove(&mut self, signal: i32) -> std::io::Result<&mut Self>;
    pub fn contains(&self, signal: i32) -> std::io::Result<bool>;
}

I think SignalSet probably deserves some additional trait implementations (at least Debug and Clone, for example, since those should be doable).

[sunshowers]

I've updated the API based on a suggestion by @zackw.

[joshtriplett]

will_block_signal has the wrong type here; it should return bool.

[the8472]

We discussed this in today's meeting. We're ok with adding signalmasking for the child process but the proposed API needs some polish.

• will_block_signal should be removed since we generally don't support inspection methods on Command
• unblock_signal seems questionable since child processes start out with all signals unblocked. So this would only make sense to undo something previously set with block_signal

Should the io::Errors returned by these methods be deferred until spawn()? I think doing it immediately is better to indicate errors (for invalid signals) immediately.

We are in favor of deferring until spawn.

Should CommandExt::signal_mask be marked unsafe? See discussion rust-lang/rust#100737 (comment).

We haven't reached a decision on that and it should be listed as open question in the tracking issue.
It might be reasonable to keep this a safe method because it doesn't affect the safety of the current process and spawning processes in "questionable" ways falls into the same category as doing Comman::new("rm").args(["-rf", "/"]).

[zackw]

unblock_signal seems questionable since child processes start out with all signals unblocked. So this would only make sense to undo something previously set with block_signal

This is only mostly true, and the exceptions matter.

In the C world, if you don't do anything special, child processes start out with a copy of the parent's signal mask, whatever it is. There are shell utilities (e.g. nohup) that intentionally mask some signals on behalf of the child processes they start. The convention these utilities expect is that each child process will pass that same mask down to its own children unless it has a positive reason to do otherwise (e.g. you nest a tool that masks SIGINT inside nohup).

But this means that a program may start out with some signals masked, and that it might have a positive reason to unmask some of those signals for child processes, and therefore unblock_signal is a necessary API.

[note: nohup from coreutils 8.32, which is the only implementation I can conveniently test right now, actually works by setting the signal disposition for SIGHUP, but I'm pretty sure there do, or did, exist implementations that worked by manipulating the signal mask.]