From bd770907034609dcdb927c3a75b6a98eff4f23e7 Mon Sep 17 00:00:00 2001
From: Kane York <kanepyork@gmail.com>
Date: Thu, 23 Jul 2020 19:10:51 -0700
Subject: 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>
---
 third_party/nix/src/libstore/build.cc        |  3 ---
 third_party/nix/src/libutil/util.cc          | 28 ++++++++++++----------------
 third_party/nix/src/libutil/util.hh          |  1 -
 third_party/nix/src/nix-daemon/nix-daemon.cc |  1 -
 4 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/third_party/nix/src/libstore/build.cc b/third_party/nix/src/libstore/build.cc
index da35388d85..74db67e862 100644
--- a/third_party/nix/src/libstore/build.cc
+++ b/third_party/nix/src/libstore/build.cc
@@ -2342,8 +2342,6 @@ void DerivationGoal::startBuilder() {
 
     userNamespaceSync.create();
 
-    options.allowVfork = false;
-
     Pid helper = startProcess(
         [&]() {
           /* Drop additional groups here because we can't do it
@@ -2443,7 +2441,6 @@ void DerivationGoal::startBuilder() {
 #endif
   {
   fallback:
-    options.allowVfork = !buildUser && !drv->isBuiltin();
     pid = startProcess([&]() { runChild(); }, options);
   }
 
diff --git a/third_party/nix/src/libutil/util.cc b/third_party/nix/src/libutil/util.cc
index bb6a17e030..b97624e58d 100644
--- a/third_party/nix/src/libutil/util.cc
+++ b/third_party/nix/src/libutil/util.cc
@@ -859,7 +859,6 @@ void killUser(uid_t uid) {
      fork a process, switch to uid, and send a mass kill. */
 
   ProcessOptions options;
-  options.allowVfork = false;
 
   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. */
-static pid_t doFork(bool allowVfork, const std::function<void()>& fun)
-    __attribute__((noinline));
-static pid_t doFork(bool allowVfork, const std::function<void()>& fun) {
+/*
+ * Please note that it is not legal for this function to call vfork().  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.
+ */
+static pid_t doFork(const std::function<void()>& fun) __attribute__((noinline));
+static pid_t doFork(const std::function<void()>& fun) {
 #ifdef __linux__
-  pid_t pid = allowVfork ? vfork() : fork();
-#else
-  pid_t pid = fork();
+  // TODO(kanepyork): call clone() instead for faster forking
 #endif
+
+  pid_t pid = fork();
   if (pid != 0) {
     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) {
     throw SysError("unable to fork");
   }
@@ -1012,12 +1014,6 @@ void runProgram2(const RunOptions& options) {
   }
 
   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. */
   Pid pid = startProcess(
diff --git a/third_party/nix/src/libutil/util.hh b/third_party/nix/src/libutil/util.hh
index ede83164de..0fa15f745c 100644
--- a/third_party/nix/src/libutil/util.hh
+++ b/third_party/nix/src/libutil/util.hh
@@ -232,7 +232,6 @@ struct ProcessOptions {
   std::string errorPrefix = "error: ";
   bool dieWithParent = true;
   bool runExitHandlers = false;
-  bool allowVfork = true;
 };
 
 pid_t startProcess(std::function<void()> fun,
diff --git a/third_party/nix/src/nix-daemon/nix-daemon.cc b/third_party/nix/src/nix-daemon/nix-daemon.cc
index 1f49788ae5..2570179d42 100644
--- a/third_party/nix/src/nix-daemon/nix-daemon.cc
+++ b/third_party/nix/src/nix-daemon/nix-daemon.cc
@@ -1051,7 +1051,6 @@ static void daemonLoop(char** argv) {
       options.errorPrefix = "unexpected Nix daemon error: ";
       options.dieWithParent = false;
       options.runExitHandlers = true;
-      options.allowVfork = false;
       startProcess(
           [&]() {
             fdSocket = -1;
-- 
cgit 1.4.1