-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
std.os.ChildProcess should use clone instead of fork on linux #1620
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
Comments
Interesting notes from the man page:
|
I don't think that can work. With vfork() you're only allowed to call execve() or _exit(). The setup that spawnPosix() does, like changing uid/gid and the cwd, is incompatible with vfork() - it would be visible to other threads in the parent process. |
My understanding is that changes to memory would affect the parent process, but the actual pid is a new child pid. So things like current directory, uid/gid, would be independent from the parent process. I'm doing some experiments now. Currently I'm getting a confusing segfault with the following code which I am troubleshooting: const std = @import("std");
const os = std.os;
const linux = os.linux;
pub fn main() void {
var x: i32 = 1234;
const rc = linux.vfork();
if (rc == 0) {
// we are the child
x += 1;
linux.exit(0);
}
std.debug.warn("x={}\n", x);
}
gdb has no stack trace, and the program prints |
Figured it out. vfork is a function, and when the child does a /// This must be inline, and inline call the syscall function, because if the
/// child does a return it will clobber the parent's stack.
pub inline fn vfork() usize {
return @inlineCall(syscall0, SYS_vfork);
} However it's still problematic, because the example code now does this in debug builds:
But does this in release builds:
This is because the optimizer sees There's a better way to do this, which is to use |
Sorry, I thought posix_setregid() and posix_setreuid() did the "signal other threads" dance that glibc and musl do. I see now that they only update the calling thread. changeCurDir() and posixExecve() on the other hand allocate memory. That will be visible to other threads because the fork shares the address space with the parent. Apropos signals, signal handlers are one reason I never used vfork() in libuv. Signals delivered to the fork will be observable in the parent when the signal handler writes to memory. And signal handlers exist for their side effects so that's probably an all too common pitfall. |
Agreed, this issue is no longer to use With |
Why wouldn't you just use |
tldr;
(eventual) future without a bazillion flags to benches aspawn vs alternatives Benchmark Time CPU Iterations
---------------------------------------------------------------------
BM_aspawn_no_reuse 18009 ns 17942 ns 38943
BM_aspawn/threads:1 14500 ns 14446 ns 48339
BM_vfork_with_shared_stack 46545 ns 16554 ns 44027
BM_fork 54583 ns 54527 ns 12810
BM_posix_spawn 125061 ns 29091 ns 24483 |
This is surprising
|
posix_spawn is too limited in user modification of the startup code. Take as example a lib that wants to create multiple pipes without the user having to worry about, but still providing the user with the open pipe handle for reading/writing. Standard option: child uses dup() and close() the unused file handles such that writer.writerAll works as expected and doesn't need a "Read until x" to prevent infinite wait on data by the consumer. And using explicit numbers for file desriptor does only work by chance xor forces user code to use explicit file descriptors. |
The standard option is to just do the
This is outlined in the man page for |
Yes. The problem however is that you dont know which file descriptor you will get back from calling We indeed know the "original pipe ends" before calling Offering the user "where+how to store system resources" sounds like the best option here to me, but I dont know best practice to "store stuff before exec and access it after exec" for this use case. |
You tell |
The parent process might use the file descriptor already for something different and expect the child not to overwrite/bambuzzle it. |
The |
Yes, though thats not the point. You cant just willy-nilly use file descriptors, because they might be already used for something else (and intentionally by user not automatically closed with close-on-exec via If you want to get one fresh from Kernel via For stdin,stdout and stderr we dont need that, because they must be always available and have a fixed numbering schema. |
Because that's not actually a syscall. That's a libc function. Not to mention, it's trash. |
I would argue that the API (on our side) should require the caller to explicitly specify which handles/file descriptors (or however the OS it runs on names it) the child should inherit. Everything else is racy (as in, the child could receive unwanted handles when multiple threads try to create a child). On Unix-likes this can be done on Zig's side by always using CLO_EXEC and then removing it from the specified fds (be it via dup{,2,3}, fcntl etc. Windows has an option for that in its CreateProcess API (see https://learn.microsoft.com/en-us/windows/win32/procthread/inheritance#inheriting-handles, https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute and PROC_THREAD_ATTRIBUTE_HANDLE_LIST). And I know from some research OSes where one must explicitly specify it anyway. So, being explicit would be less-racy/more secure, can already be implemented, and be more "future-proof". Additionally I would argue that it would be a quite clear API. Sure, one could circumvent it on some platforms, but not on all. |
vfork pauses the parent process until the child process either calls execve or exit. This prevents Copy On Write pages, which makes spawning child processes avoid double the parent resident memory requirements when overcommit is off.
The text was updated successfully, but these errors were encountered: