Document safety of `fork()` and fix tests by jonas-schievink · Pull Request #695 · nix-rust/nix

jonas-schievink · 2017-07-20T17:56:42Z

Some tests were invoking non-async-signal-safe functions from the child
process after a fork. Since they might be invoked in parallel, this
could lead to problems.

cc #586

Susurrus

I think this makes sense, setting fork to be unsafe. Just a few minor nits here to clean up. Also @asomers will need to take a look at this.

Susurrus · 2017-07-20T18:05:41Z

+
+    // Safe: Child only calls `_exit`, which is signal-safe
+    let pid = unsafe { fork() };
    match pid {


Let's get rid of the pid variable completely here for consistency with other tests and since it isn't used anywhere else.

Susurrus · 2017-07-20T18:07:23Z

+///
+/// # Safety
+///
+/// In a multithreaded program, only [async-signal-safe] functions may be called


Your comments talk about libc::exit and libc::pause being safe to use here. Let's explicitly list those here. We should also state that std::process:exit is not safe.

libc::exit actually isn't safe, only _exit is

Sorry, that's what I meant. We should document a few of the safe and unsafe things for reference. The big thing about this is that how would the user figure out what is safe or not. I don't know if we have any guidance on that end, however.

asomers

I like all of the changes that you've made to the tests. But I still think it's premature to mark fork as unsafe. It's not that I think fork is safe; it's that I think consistency between nix and libstd is more important.

Nobody has yet come up with a POC that violates Rust's memory safety rules by forking. Until that happens, I don't think libstd is going to change.

asomers · 2017-07-20T18:14:04Z

        let (reader, writer) = pipe().unwrap();

-        match fork().unwrap() {
+        // Safe: Child calls `exit`, `dup` and the provided `exec*` family function.


If you're going to list them all, you should add close

jonas-schievink · 2017-07-20T19:53:27Z

Nobody has yet come up with a POC that violates Rust's memory safety rules by forking. Until that happens, I don't think libstd is going to change.

And this is a very bad thing IMO. Maybe it's hard to come up with a POC on the currently supported configurations, but what about other allocator implementations, other libcs, other operating systems, other architectures where this is a more serious issue? Even if those aren't supported by Rust today, even if those configurations don't even exist today - the fact that POSIX says "don't do this it's unsafe" should be more than enough of a reason to proceed with extreme caution.

A language that aims to guarantee memory safety shouldn't do things like this.

asomers · 2017-07-20T20:26:22Z

POSIX says it's "unsafe", but the libstd guys say it's not unsafe. unsafe means something very particular in Rust. You can easily do things that are "unsafe" without using any unsafe code, like leak memory, trigger deadlocks, etc. In order to convince libstd to change, we'll probably have to come up with a POC that violates one of Rust's narrow memory safety rules, for example by overrunning an array or using after free.

jonas-schievink · 2017-07-20T21:02:29Z

Yeah, seems like POSIX leaves the meaning of "safe" completely undefined. Depending on that, fork might indeed by safe according to Rust's definition of safety. However, common sense makes me doubt that POSIX, which defines a C API, would call something "unsafe" that Rust would deem safe.

jonas-schievink · 2017-07-20T21:05:39Z

As an aside, from the docs of pthread_atfork:

If a fork() call in a multi-threaded process leads to a child fork handler calling any function that is not async-signal-safe, the behavior is undefined.

I would be very surprised if "the behaviour is undefined" means something different than UB in C in this context, and the child handler is basically just code that is executed by the child after a fork, so there isn't much of a difference to what we're dealing with. (UB is strictly worse than memory unsafety, after all)

asomers · 2017-07-21T00:30:07Z

I don't want to argue with you about the safety of fork, because I don't completely disagree with you. But nix needs to maintain consistency with libc, and since libc is far more important than nix, we need to be having the conversation over there. For now, please revert the changes to fork and let's just merge the changes to the tests and to fork's documentation.

Susurrus · 2017-07-21T03:24:49Z

I'd like to echo what @asomers says here. Rust has a pretty narrow definition of "unsafe" and whatever libc goes with we'll stick with for convention. That said, it is worth bringing up the invariants for these functions so that users can be aware.

asomers · 2017-07-22T03:38:13Z

PR #566 just added a new fork call in a test. So when you rebase, please replace its sys::process::exit with _exit

Some tests were invoking non-async-signal-safe functions from the child process after a `fork`. Since they might be invoked in parallel, this could lead to problems.

Note that ptrace isn't documented as signal-safe, but it's supposed to just be a light syscall wrapper, so it should be fine.

asomers · 2017-07-23T22:14:05Z

Thanks for the PR @jonas-schievink . Looks like we can sneak this one into 0.9.0

bors r+

695: Document safety of `fork()` and fix tests r=asomers Some tests were invoking non-async-signal-safe functions from the child process after a `fork`. Since they might be invoked in parallel, this could lead to problems. cc #586

bors · 2017-07-23T22:46:21Z

Build succeeded

Susurrus suggested changes Jul 20, 2017

View reviewed changes

asomers requested changes Jul 20, 2017

View reviewed changes

jonas-schievink added 3 commits July 22, 2017 18:02

Document invariants of fork and fix tests

7128f15

Some tests were invoking non-async-signal-safe functions from the child process after a `fork`. Since they might be invoked in parallel, this could lead to problems.

Remove unneeded local

f793d5f

Replace remaining process::exit with libc::_exit

91fa117

Note that ptrace isn't documented as signal-safe, but it's supposed to just be a light syscall wrapper, so it should be fine.

jonas-schievink changed the title ~~Make fork() unsafe and fix tests~~ Document safety of fork() and fix tests Jul 22, 2017

bors Bot merged commit 91fa117 into nix-rust:master Jul 23, 2017

jonas-schievink deleted the fork branch July 23, 2017 22:51

Susurrus mentioned this pull request Dec 3, 2017

Rethinking the interface for fork #586

Open

vi mentioned this pull request Feb 14, 2019

Shall safety status of unistd::fork be the same as std's Process::pre_exec? #1030

Closed

Conversation

jonas-schievink commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Susurrus left a comment

Choose a reason for hiding this comment

Uh oh!

Susurrus Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

Susurrus Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

jonas-schievink Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

Susurrus Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

asomers Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

jonas-schievink commented Jul 20, 2017

Uh oh!

asomers commented Jul 20, 2017

Uh oh!

jonas-schievink commented Jul 20, 2017

Uh oh!

jonas-schievink commented Jul 20, 2017

Uh oh!

asomers commented Jul 21, 2017

Uh oh!

Susurrus commented Jul 21, 2017

Uh oh!

asomers commented Jul 22, 2017

Uh oh!

asomers commented Jul 23, 2017

Uh oh!

bors Bot commented Jul 23, 2017

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonas-schievink commented Jul 20, 2017 •

edited

Loading