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

Rethinking the interface for fork #586

Open
kamalmarhubi opened this issue Apr 17, 2017 · 11 comments
Open

Rethinking the interface for fork #586

kamalmarhubi opened this issue Apr 17, 2017 · 11 comments

Comments

@kamalmarhubi
Copy link
Member

I was writing a shell as an example for nix, and I realised that #555 makes a really good point: the code that runs in the child after fork(2) must not allocate if there is more than one thread. I don't know if fork should necessarily be made unsafe, but certainly we need to document better what is ok to do afterwards.

This is somewhat related to #90 in that there's an execution context where only limited operations should be performed. Ideally nix would force only correct usage, but at the same time there's a lot of value of staying close to the libc definitions of functions. (#190 seems connected here too.)

@Susurrus Susurrus changed the title Rethinkging he interface for fork Rethinking the interface for fork Apr 17, 2017
@jonas-schievink
Copy link
Contributor

Also see http://www.linuxprogrammingblog.com/threads-and-fork-think-twice-before-using-them

And the man page says:

After a fork() in a multithreaded program, the child can safely
call only async-signal-safe functions (see signal-safety(7)) until
such time as it calls execve(2).

This makes it sound like fork really really should be made unsafe: Spawning threads and calling non-signal-safe function is possible in safe Rust, therefore there must not be a safe way to do so after a fork. Instead, nix could provide safe utility functions that do common patterns like fork + execve in a safe way.

@jonas-schievink
Copy link
Contributor

This has come up in the standard library, too: rust-lang/rust#39575

@asomers
Copy link
Member

asomers commented Jul 19, 2017

You're right that fork is unsafe. But is it unsafe in the Rust sense? I think we should follow whatever decision rust-lang makes in the issue you linked.

@Susurrus
Copy link
Contributor

@asomers I read through some of the discussion there. The argument is that UB should be assumed to include memory unsafety by default. Specifically for fork() there are some more reasonings that should make this more likely.

@asomers
Copy link
Member

asomers commented Jul 20, 2017

Is there any undefined behavior associated with calling async-signal-unsafe functions after forking? I can't find any. fork(2) doesn't mention any undefined behavior. Instead, it suggests not calling async-signal-unsafe functions in order to prevent deadlocks and the like.

bors bot added a commit that referenced this issue Jul 23, 2017
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
@Susurrus
Copy link
Contributor

Susurrus commented Dec 3, 2017

@jonas-schievink @asomers Did this get cleaned resolved with the merging of #695?

@jonas-schievink

This comment has been minimized.

@jonas-schievink
Copy link
Contributor

Update: The conclusion of rust-lang/rust#39575 was that before_exec should have been an unsafe function, and has been deprecated and replaced by an unsafe fn pre_exec. Thus fork should also be considered an unsafe operation.

@kamalmarhubi
Copy link
Member Author

@jonas-schievink thanks for that update!

@interacsion
Copy link

The rustdocs for fork and forkpty forget to mention the restriction is lifted after execve is called, I think this is mildly confusing

@SteveLauC
Copy link
Member

The rustdocs for fork and forkpty forget to mention the restriction is lifted after execve is called, I think this is mildly confusing

PR #2591 will fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants