fix(3p/nix): do not call vfork
The use of vfork() in Nix is entirely illegal. Quote: If the process created by vfork() returns from the function in which vfork() was called, or calls any other function before successfully calling _exit() or one of the exec*() family of functions, the behavior is undefined. -- Linux man-pages, release 5.05 Add a TODO to use the higher-performance variants of clone() on Linux when it is available. Change-Id: I42370e1568ad6e2d00d70d0b66c8aded8f1288bb Reviewed-on: https://cl.tvl.fyi/c/depot/+/1418 Tested-by: BuildkiteCI Reviewed-by: tazjin <mail@tazj.in> Reviewed-by: Alyssa Ross <hi@alyssa.is>
This commit is contained in:
parent
388b5f1abe
commit
bd77090703
4 changed files with 12 additions and 21 deletions
3
third_party/nix/src/libstore/build.cc
vendored
3
third_party/nix/src/libstore/build.cc
vendored
|
@ -2342,8 +2342,6 @@ void DerivationGoal::startBuilder() {
|
||||||
|
|
||||||
userNamespaceSync.create();
|
userNamespaceSync.create();
|
||||||
|
|
||||||
options.allowVfork = false;
|
|
||||||
|
|
||||||
Pid helper = startProcess(
|
Pid helper = startProcess(
|
||||||
[&]() {
|
[&]() {
|
||||||
/* Drop additional groups here because we can't do it
|
/* Drop additional groups here because we can't do it
|
||||||
|
@ -2443,7 +2441,6 @@ void DerivationGoal::startBuilder() {
|
||||||
#endif
|
#endif
|
||||||
{
|
{
|
||||||
fallback:
|
fallback:
|
||||||
options.allowVfork = !buildUser && !drv->isBuiltin();
|
|
||||||
pid = startProcess([&]() { runChild(); }, options);
|
pid = startProcess([&]() { runChild(); }, options);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
28
third_party/nix/src/libutil/util.cc
vendored
28
third_party/nix/src/libutil/util.cc
vendored
|
@ -859,7 +859,6 @@ void killUser(uid_t uid) {
|
||||||
fork a process, switch to uid, and send a mass kill. */
|
fork a process, switch to uid, and send a mass kill. */
|
||||||
|
|
||||||
ProcessOptions options;
|
ProcessOptions options;
|
||||||
options.allowVfork = false;
|
|
||||||
|
|
||||||
Pid pid = startProcess(
|
Pid pid = startProcess(
|
||||||
[&]() {
|
[&]() {
|
||||||
|
@ -897,16 +896,19 @@ void killUser(uid_t uid) {
|
||||||
|
|
||||||
//////////////////////////////////////////////////////////////////////
|
//////////////////////////////////////////////////////////////////////
|
||||||
|
|
||||||
/* Wrapper around vfork to prevent the child process from clobbering
|
/*
|
||||||
the caller's stack frame in the parent. */
|
* Please note that it is not legal for this function to call vfork(). If the
|
||||||
static pid_t doFork(bool allowVfork, const std::function<void()>& fun)
|
* process created by vfork() returns from the function in which vfork() was
|
||||||
__attribute__((noinline));
|
* called, or calls any other function before successfully calling _exit() or
|
||||||
static pid_t doFork(bool allowVfork, const std::function<void()>& fun) {
|
* one of the exec*() family of functions, the behavior is undefined.
|
||||||
|
*/
|
||||||
|
static pid_t doFork(const std::function<void()>& fun) __attribute__((noinline));
|
||||||
|
static pid_t doFork(const std::function<void()>& fun) {
|
||||||
#ifdef __linux__
|
#ifdef __linux__
|
||||||
pid_t pid = allowVfork ? vfork() : fork();
|
// TODO(kanepyork): call clone() instead for faster forking
|
||||||
#else
|
|
||||||
pid_t pid = fork();
|
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
|
pid_t pid = fork();
|
||||||
if (pid != 0) {
|
if (pid != 0) {
|
||||||
return pid;
|
return pid;
|
||||||
}
|
}
|
||||||
|
@ -938,7 +940,7 @@ pid_t startProcess(std::function<void()> fun, const ProcessOptions& options) {
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
pid_t pid = doFork(options.allowVfork, wrapper);
|
pid_t pid = doFork(wrapper);
|
||||||
if (pid == -1) {
|
if (pid == -1) {
|
||||||
throw SysError("unable to fork");
|
throw SysError("unable to fork");
|
||||||
}
|
}
|
||||||
|
@ -1012,12 +1014,6 @@ void runProgram2(const RunOptions& options) {
|
||||||
}
|
}
|
||||||
|
|
||||||
ProcessOptions processOptions;
|
ProcessOptions processOptions;
|
||||||
// vfork implies that the environment of the main process and the fork will
|
|
||||||
// be shared (technically this is undefined, but in practice that's the
|
|
||||||
// case), so we can't use it if we alter the environment
|
|
||||||
if (options.environment) {
|
|
||||||
processOptions.allowVfork = false;
|
|
||||||
}
|
|
||||||
|
|
||||||
/* Fork. */
|
/* Fork. */
|
||||||
Pid pid = startProcess(
|
Pid pid = startProcess(
|
||||||
|
|
1
third_party/nix/src/libutil/util.hh
vendored
1
third_party/nix/src/libutil/util.hh
vendored
|
@ -232,7 +232,6 @@ struct ProcessOptions {
|
||||||
std::string errorPrefix = "error: ";
|
std::string errorPrefix = "error: ";
|
||||||
bool dieWithParent = true;
|
bool dieWithParent = true;
|
||||||
bool runExitHandlers = false;
|
bool runExitHandlers = false;
|
||||||
bool allowVfork = true;
|
|
||||||
};
|
};
|
||||||
|
|
||||||
pid_t startProcess(std::function<void()> fun,
|
pid_t startProcess(std::function<void()> fun,
|
||||||
|
|
1
third_party/nix/src/nix-daemon/nix-daemon.cc
vendored
1
third_party/nix/src/nix-daemon/nix-daemon.cc
vendored
|
@ -1051,7 +1051,6 @@ static void daemonLoop(char** argv) {
|
||||||
options.errorPrefix = "unexpected Nix daemon error: ";
|
options.errorPrefix = "unexpected Nix daemon error: ";
|
||||||
options.dieWithParent = false;
|
options.dieWithParent = false;
|
||||||
options.runExitHandlers = true;
|
options.runExitHandlers = true;
|
||||||
options.allowVfork = false;
|
|
||||||
startProcess(
|
startProcess(
|
||||||
[&]() {
|
[&]() {
|
||||||
fdSocket = -1;
|
fdSocket = -1;
|
||||||
|
|
Loading…
Reference in a new issue