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/libutil/util.cc | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

(limited to 'third_party/nix/src/libutil/util.cc')

diff --git a/third_party/nix/src/libutil/util.cc b/third_party/nix/src/libutil/util.cc
index bb6a17e03047..b97624e58dbb 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(
-- 
cgit 1.4.1