Support setting signal masks for child processes on Unix #100737

sunshowers · 2022-08-19T00:59:58Z

This commit implements basic support for setting signal masks during process spawning.

With the posix_spawn code path, this means setting posix_spawnattr_setsigmask.
With the fork/exec path, this means calling pthread_sigmask in the child before executing it.

This PR retains the default behavior (empty signal mask). I've also put up #101077 as a dependent PR to change the default behavior.

API change proposal:

Add a way to set the signal mask of a child process libs-team#88

Unresolved questions:

Is the possible UB with masking SIGSEGV etc (documented in the POSIX spec) enough reason to mark this unsafe?

rust-highfive · 2022-08-19T01:00:01Z

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

sunshowers · 2022-08-19T01:00:44Z

Ah I'm not sure if there should be a reviewer at the moment, this is meant to be a draft PR :)

jyn514 · 2022-08-19T04:11:45Z

@sunshowers I've unassigned a reviewer for now, but please do run r? rust-lang/libs-api once this is ready for review so it doesn't get lost :)

jyn514 · 2022-08-19T04:14:07Z

library/std/src/sys/unix/process/process_common.rs

@@ -128,6 +169,47 @@ pub enum Stdio {
    Fd(FileDesc),
 }

+pub struct SignalSet {


Shouldn't this be private?

It follows the pattern of the other items in the module which are all declared pub (though they actually resolve to pub(crate)).

library/std/src/sys/unix/process/process_common.rs

sunshowers · 2022-08-19T18:57:39Z

Updates:

Addressed review feedback from @joshtriplett
Added documentation and examples

I think this is ready for review now.

rustbot · 2022-08-19T18:58:05Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

sunshowers · 2022-08-19T18:58:47Z

r? rust-lang/libs-api

sunshowers · 2022-08-19T19:00:38Z

(I still need to write up the change proposal I guess!)

edit: done: rust-lang/libs-team#88

thomcc · 2022-08-19T20:13:02Z

Is the possible UB with masking SIGSEGV etc (documented in the POSIX spec) enough reason to mark this unsafe?

I guess the problem would be that it changes the documented contract of the pre_exec hook. I don't think there's a way to raise SIGBUS, SIGFPE, or SIGSEGV from safe code (well, a way that would violate the UB), but SIGILL is also listed as one of the signals that is UB to trigger when it's blocked, and we commonly use that for an "abort immediately" function (e.g. core::intrinsics::abort()). (Often this is used for "we need to abort and are inside libcore or liballoc rather than libstd")

That said, pre_exec is already unsafe and you have to be careful anyway, so I'm not 100% sure the extent to which this matters. The easy way of triggering sigill from safe code is a double panic, but inside pre_exec, the first panic will lead to sys::abort_internal(), e.g. process::abort() (and not intrinsics::abort()), which shouldn't raise SIGILL. (I guess a small caveat here is the panic_immediate_abort feature, which does use intrinsics::abort()).

I'm not sure if this is worth marking as unsafe, it seems unfortunate if so, but it's probably worth considering carefully if we aren't going to, to avoid a footgun. That said, there's probably enough time to get this right prior to stabilizing, so I don't know that this is a blocker.

sunshowers · 2022-08-19T20:26:45Z

That said, pre_exec is already unsafe and you have to be careful anyway, so I'm not 100% sure the extent to which this matters.

Yeah, that makes sense. Note that existing code won't be affected though -- only users of both signal_mask and pre_exec are potentially affected. So at least in principle, this could be documented as an additional constraint on pre_exec, and signal_mask can be left safe.

thomcc · 2022-08-19T20:27:41Z

Yeah, so I suppose we could adjust the safety contract of pre_exec at the same time. I'm not sure if this is something we generally want to do, but pre_exec is rare and highly dodgy already, and it's pretty unlikely anybody is triggering sigill inside their pre_exec, so maybe it's fine.

sunshowers · 2022-08-19T20:29:41Z

(whoops, closed by accident)

The other issue with safety I was thinking of is: what happens if the child itself has a segfault unrelated to the parent, and the parent has masked SIGSEGV? This isn't UB within Rust, but it is UB across the system. I don't know if unsafe extends to that kind of UB.

thomcc · 2022-08-19T20:32:28Z

I don't know if unsafe extends to that kind of UB.

In general it doesn't. It's totally safe to spawn processes that crash or do other UB (std::process::Command::spawn is safe after all). We have no way of preventing it, it's kind of outside Rust's ability to defend against in general.

That said, fork/exec stuff is ofc where the line gets blurry here (for example, technically pre_exec is run inside the child process, but it's still unsafe).

We're going to need this for upcoming signal mask support.

sunshowers · 2022-08-27T05:48:30Z

I've put up #101077 as a dependent PR on top of this one.

This commit implements basic support for setting signal masks during process spawning. * With the `posix_spawn` code path, this means setting `posix_spawnattr_setsigmask`. * With the `fork/exec` path, this means calling `pthread_sigmask` in the child before executing it. This also fixes a bug: previously, we were always setting the signal mask to the empty set, breaking tools like `nohup`. With this change, we inherit the signal mask from the parent by default.

thomcc

The impl looks fine although I'll probably want to review again after the situation with the other PR is resolved.

thomcc · 2022-08-28T15:01:56Z

library/std/src/sys/unix/process/process_common.rs

@@ -296,6 +396,10 @@ impl Command {
    pub fn get_pgroup(&self) -> Option<pid_t> {
        self.pgroup
    }
+    #[allow(dead_code)]
+    pub fn get_signal_mask(&self) -> io::Result<&SignalSet> {
+        self.signal_mask.get_or_try_init(SignalSet::empty)


We'll want to remove the OnceLock from this if the other PR's FCP doesn't get accepted.

thomcc · 2022-08-28T15:03:04Z

I'm going to mark this as blocked on the other PR, as it's unlikely that we want this impl without that (and the resolution there may change how we approach this API).

bors · 2022-08-29T11:34:02Z

☔ The latest upstream changes (presumably #100786) made this pull request unmergeable. Please resolve the merge conflicts.

jsgf · 2022-09-01T07:42:22Z

library/std/src/os/unix/process.rs

+    /// success and has no effect.
+    ///
+    /// Blocking some signals like `SIGSEGV` can lead to undefined behavior in
+    /// the child. See the [`pthread_sigmask`] man page for more information.


sigprocmask is probably the more canonical reference.

Also, while the manpage may say that the effect of blocking synchronous traps (ie, SEGV/BUS/FPE/ILL raised while executing an instruction) is undefined, I wouldn't read that as being the same as UB at the Rust/llvm level.

In practice I think it would either deliver the signal as SIG_DFL (ie, Linux), or endlessly re-trap on the same instruction (what x86 macOS seems to do) - either way the program has effectively terminated and won't enter any UB states as far as the program semantics are concerned.

Heh it's actually the same man page for both pthread_sigmask and sigprocmask. I can change the reference in the docs to be sigprocmask though.

Huh - on Fedora they're separate, but pthread_sigmask has almost no content and refers to sigprocmask for discussion of the detailed semantics.

(I was referring to the man pages from the official POSIX spec here, as linked in the comment)

Align android `sigaddset` impl with the reference impl from Bionic In rust-lang/rust#100737 I noticed we were treating the sigset_t as an array of bytes, while referencing code from android (https://github.com/aosp-mirror/platform_bionic/blob/ad8dcd6023294b646e5a8288c0ed431b0845da49/libc/include/android/legacy_signal_inlines.h) which treats it as an array of unsigned long. That said, the behavior difference is so subtle here that it's not hard to see why nobody noticed. This fixes the implementation to be equivalent to the one in bionic.

thomcc · 2022-11-10T01:59:55Z

I think this is no longer blocked.

sunshowers · 2022-11-10T02:06:26Z

Yep, planning to get to it soon. Meanwhile I've worked around it on beta Rust (1.66) by calling pthread_sigmask before and after process spawning, which appears to work.

thomcc · 2022-11-10T02:08:04Z

No rush, just got a ping on it from someone.

JohnCSimon · 2023-01-29T06:03:57Z

@sunshowers
ping from triage - can you post your status on this PR? This has sat unmoved for a while now. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Dylan-DPC · 2023-02-23T06:46:44Z

Closing this due to inactivity. Feel free to make a new pr if you wish to continue this.

sunshowers · 2023-02-26T03:13:04Z

Apologies, I started a new full-time job since I originally wrote this and haven't had the time to follow up on it. After #101077, I also figured out a way to do this without needing to add new API surface to std:

Temporarily use pthread_sigmask to set a new sigmask.
Spawn the process.
Unset the sigmask.

(and use a scope guard to ensure that the sigmask is unset in case of panic.)

This means that I'm probably not going to pursue this further. However, someone else is welcome to.

rust-highfive assigned joshtriplett Aug 19, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2022

sunshowers force-pushed the signal-mask branch from 2513ed2 to 7e08f95 Compare August 19, 2022 01:12

jyn514 unassigned joshtriplett Aug 19, 2022

jyn514 reviewed Aug 19, 2022

View reviewed changes

joshtriplett reviewed Aug 19, 2022

View reviewed changes

library/std/src/sys/unix/process/process_common.rs Outdated Show resolved Hide resolved

joshtriplett reviewed Aug 19, 2022

View reviewed changes

library/std/src/sys/unix/process/process_common.rs Outdated Show resolved Hide resolved

sunshowers force-pushed the signal-mask branch from 7e08f95 to 8e6e778 Compare August 19, 2022 18:56

sunshowers marked this pull request as ready for review August 19, 2022 18:58

sunshowers mentioned this pull request Aug 19, 2022

Add a way to set the signal mask of a child process rust-lang/libs-team#88

Closed

sunshowers changed the title ~~[WIP] implement signal mask support~~ Support setting signal masks for child processes on Unix Aug 19, 2022

sunshowers closed this Aug 19, 2022

sunshowers reopened this Aug 19, 2022

Implement sigdelset and sigismember for Android

7b9651f

We're going to need this for upcoming signal mask support.

sunshowers mentioned this pull request Aug 27, 2022

Change process spawning to inherit the parent's signal mask by default #101077

Merged

sunshowers force-pushed the signal-mask branch from da45dc3 to aa251cf Compare August 27, 2022 05:49

sunshowers requested a review from thomcc August 27, 2022 05:50

sunshowers force-pushed the signal-mask branch from aa251cf to ac8fac2 Compare August 27, 2022 06:06

thomcc reviewed Aug 28, 2022

View reviewed changes

thomcc added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Aug 28, 2022

jsgf reviewed Sep 1, 2022

View reviewed changes

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2022

Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2022

thomcc added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Nov 10, 2022

cocoon mentioned this pull request Feb 17, 2023

[Signals] Options/Params to disable signal handling rustdesk/rustdesk-server#199

Open

Dylan-DPC closed this Feb 23, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting signal masks for child processes on Unix #100737

Support setting signal masks for child processes on Unix #100737

sunshowers commented Aug 19, 2022 •

edited

Loading

rust-highfive commented Aug 19, 2022

sunshowers commented Aug 19, 2022

jyn514 commented Aug 19, 2022

jyn514 Aug 19, 2022

sunshowers Aug 19, 2022

sunshowers commented Aug 19, 2022

rustbot commented Aug 19, 2022

sunshowers commented Aug 19, 2022

sunshowers commented Aug 19, 2022 •

edited

Loading

thomcc commented Aug 19, 2022

sunshowers commented Aug 19, 2022 •

edited

Loading

thomcc commented Aug 19, 2022 •

edited

Loading

sunshowers commented Aug 19, 2022

thomcc commented Aug 19, 2022 •

edited

Loading

sunshowers commented Aug 27, 2022

thomcc left a comment

thomcc Aug 28, 2022

thomcc commented Aug 28, 2022

bors commented Aug 29, 2022

jsgf Sep 1, 2022

sunshowers Sep 1, 2022

jsgf Sep 1, 2022

sunshowers Sep 1, 2022

thomcc commented Nov 10, 2022

sunshowers commented Nov 10, 2022 •

edited

Loading

thomcc commented Nov 10, 2022

JohnCSimon commented Jan 29, 2023

Dylan-DPC commented Feb 23, 2023

sunshowers commented Feb 26, 2023 •

edited

Loading

Support setting signal masks for child processes on Unix #100737

Support setting signal masks for child processes on Unix #100737

Conversation

sunshowers commented Aug 19, 2022 • edited Loading

rust-highfive commented Aug 19, 2022

sunshowers commented Aug 19, 2022

jyn514 commented Aug 19, 2022

jyn514 Aug 19, 2022

Choose a reason for hiding this comment

sunshowers Aug 19, 2022

Choose a reason for hiding this comment

sunshowers commented Aug 19, 2022

rustbot commented Aug 19, 2022

sunshowers commented Aug 19, 2022

sunshowers commented Aug 19, 2022 • edited Loading

thomcc commented Aug 19, 2022

sunshowers commented Aug 19, 2022 • edited Loading

thomcc commented Aug 19, 2022 • edited Loading

sunshowers commented Aug 19, 2022

thomcc commented Aug 19, 2022 • edited Loading

sunshowers commented Aug 27, 2022

thomcc left a comment

Choose a reason for hiding this comment

thomcc Aug 28, 2022

Choose a reason for hiding this comment

thomcc commented Aug 28, 2022

bors commented Aug 29, 2022

jsgf Sep 1, 2022

Choose a reason for hiding this comment

sunshowers Sep 1, 2022

Choose a reason for hiding this comment

jsgf Sep 1, 2022

Choose a reason for hiding this comment

sunshowers Sep 1, 2022

Choose a reason for hiding this comment

thomcc commented Nov 10, 2022

sunshowers commented Nov 10, 2022 • edited Loading

thomcc commented Nov 10, 2022

JohnCSimon commented Jan 29, 2023

Dylan-DPC commented Feb 23, 2023

sunshowers commented Feb 26, 2023 • edited Loading

sunshowers commented Aug 19, 2022 •

edited

Loading

sunshowers commented Aug 19, 2022 •

edited

Loading

sunshowers commented Aug 19, 2022 •

edited

Loading

thomcc commented Aug 19, 2022 •

edited

Loading

thomcc commented Aug 19, 2022 •

edited

Loading

sunshowers commented Nov 10, 2022 •

edited

Loading

sunshowers commented Feb 26, 2023 •

edited

Loading