Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process_unix::do_exec should reset the signal mask to its value on startup, not clear it #39185

Closed
zackw opened this issue Jan 19, 2017 · 5 comments
Labels
A-process Area: std::process and std::env C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@zackw
Copy link
Contributor

zackw commented Jan 19, 2017

The Unix implementation of process::spawn (specifically, do_exec in sys/unix/process/process_unix.rs) contains logic to restore the signal mask to the default in child processes...

            // Reset signal handling so the child process starts in a
            // standardized state. libstd ignores SIGPIPE, and signal-handling
            // libraries often set a mask. Child processes inherit ignored
            // signals and the signal mask from their parent, but most
            // UNIX programs do not reset these things on their own, so we
            // need to clean things up now to avoid confusing the program
            // we're about to run.
            let mut set: libc::sigset_t = mem::uninitialized();
            t!(cvt(libc::sigemptyset(&mut set)));
            t!(cvt(libc::pthread_sigmask(libc::SIG_SETMASK, &set,
                                         ptr::null_mut())));
            let ret = sys::signal(libc::SIGPIPE, libc::SIG_DFL);
            if ret == libc::SIG_ERR {
                return io::Error::last_os_error()
            }

Completely clearing the signal mask here is subtly wrong. It should instead be restored to whatever it was when the parent process started up. Certain standard shell utilities — the best-known is nohup — deliberately start a process with some signals already masked, and expect that those settings will be inherited by any further subprocesses.

For the same reason, the handler for SIGPIPE should be reset not to SIG_DFL, but to whichever of SIG_DFL or SIG_IGN it was before libstd ignored it.

(In order to make it possible to implement such utilities in Rust, the ideal resolution here would involve adding mask_signals and ignore_signals knobs to unix::process::CommandExt, and document that the defaults for these are whatever was observed on process startup. But I don't see a clean way to do that without a lot more support for signals in general in libstd, maybe more than is properly in scope.)

@alexcrichton
Copy link
Member

Sounds reasonable to me to reset to whatever we had on startup! I think it's also fine to store away what the signal was for SIGPIPE at the start and reset to that here rather than always SIG_DFL. Having unix-specific knobs also sounds good.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. C-bug Category: This is a bug. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Jul 26, 2017
@Enselic
Copy link
Member

Enselic commented Jul 9, 2023

This issue can be closed. It was fixed by #101077.

@workingjubilee workingjubilee added the A-process Area: std::process and std::env label Jul 22, 2023
@Enselic
Copy link
Member

Enselic commented Jul 23, 2023

@workingjubilee Sorry for direct ping, but I suspect you did not notice that this issue can be closed since you just added a label to it? Maybe you can help me close it? Thank you :)

@workingjubilee
Copy link
Contributor

Not a problem! I often don't examine the contents of issues before labeling them if I can determine the label is appropriate from the title. Closing, thank you!

@zopsicle
Copy link
Contributor

zopsicle commented Apr 13, 2024

To avoid confusing future visitors: it should be noted that #101077 fixes this in a way that is different than was originally proposed in this issue (the fix removes the mask reset altogether rather than restoring the mask to what it was pre-main).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-process Area: std::process and std::env C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants