Some fixes to process-wrapper / linux-sandbox.

- Refactoring to share more code between the two programs.
- Remove setuid() call in linux-sandbox. It was added due to a wrong understanding of what process-wrapper did in the beginning and unless someone installed linux-sandbox as a setuid binary, it was a no-op.
- Switch to a new process group in linux-sandbox to avoid accidentally killing our parent.

RELNOTES: None.
PiperOrigin-RevId: 156332503

12 files changed, 100 insertions, 137 deletions

diff --git a/src/main/tools/BUILD b/src/main/tools/BUILD
index a29b71f03f..72aff6301f 100644
--- a/src/main/tools/BUILD
+++ b/src/main/tools/BUILD
@@ -1,11 +1,16 @@
 package(default_visibility = ["//src:__subpackages__"])
 
 cc_library(
+    name = "logging",
+    srcs = ["logging.cc"],
+    hdrs = ["logging.h"],
+)
+
+cc_library(
     name = "process-tools",
-    srcs = [
-        "process-tools.cc",
-        "process-tools.h",
-    ],
+    srcs = ["process-tools.cc"],
+    hdrs = ["process-tools.h"],
+    deps = [":logging"],
 )
 
 cc_binary(
@@ -21,6 +26,7 @@ cc_binary(
         "//src:windows_msvc": [],
         "//conditions:default": [
             ":process-tools",
+            ":logging",
         ],
     }),
 )
@@ -49,10 +55,21 @@ cc_binary(
             "linux-sandbox-options.h",
             "linux-sandbox-pid1.cc",
             "linux-sandbox-pid1.h",
-            "linux-sandbox-utils.h",
         ],
     }),
     linkopts = ["-lm"],
+    deps = select({
+        "//src:darwin": [],
+        "//src:darwin_x86_64": [],
+        "//src:freebsd": [],
+        "//src:windows": [],
+        "//src:windows_msys": [],
+        "//src:windows_msvc": [],
+        "//conditions:default": [
+            ":logging",
+            ":process-tools",
+        ],
+    }),
 )
 
 filegroup(
diff --git a/src/main/tools/linux-sandbox-options.cc b/src/main/tools/linux-sandbox-options.cc
index c646cece0f..ce4a624da2 100644
--- a/src/main/tools/linux-sandbox-options.cc
+++ b/src/main/tools/linux-sandbox-options.cc
@@ -12,14 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#define DIE(args...)                                     \
-  {                                                      \
-    fprintf(stderr, __FILE__ ":" S__LINE__ ": \"" args); \
-    fprintf(stderr, "\": ");                             \
-    perror(nullptr);                                     \
-    exit(EXIT_FAILURE);                                  \
-  }
-
 #include "src/main/tools/linux-sandbox-options.h"
 
 #include <errno.h>
@@ -37,7 +29,8 @@
 #include <string>
 #include <vector>
 
-#include "src/main/tools/linux-sandbox-utils.h"
+#include "src/main/tools/logging.h"
+#include "src/main/tools/process-tools.h"
 
 using std::ifstream;
 using std::unique_ptr;
@@ -207,8 +200,8 @@ static void ParseCommandLine(unique_ptr<vector<char *>> args) {
 
 // Expands a single argument, expanding options @filename to read in the content
 // of the file and add it to the list of processed arguments.
-unique_ptr<vector<char *>> ExpandArgument(unique_ptr<vector<char *>> expanded,
-                                          char *arg) {
+static unique_ptr<vector<char *>> ExpandArgument(
+    unique_ptr<vector<char *>> expanded, char *arg) {
   if (arg[0] == '@') {
     const char *filename = arg + 1;  // strip off the '@'.
     ifstream f(filename);
@@ -236,7 +229,7 @@ unique_ptr<vector<char *>> ExpandArgument(unique_ptr<vector<char *>> expanded,
 // Pre-processes an argument list, expanding options @filename to read in the
 // content of the file and add it to the list of arguments. Stops expanding
 // arguments once it encounters "--".
-unique_ptr<vector<char *>> ExpandArguments(const vector<char *> &args) {
+static unique_ptr<vector<char *>> ExpandArguments(const vector<char *> &args) {
   unique_ptr<vector<char *>> expanded(new vector<char *>());
   expanded->reserve(args.size());
   for (auto arg = args.begin(); arg != args.end(); ++arg) {
@@ -250,7 +243,6 @@ unique_ptr<vector<char *>> ExpandArguments(const vector<char *> &args) {
   return expanded;
 }
 
-// Handles parsing all command line flags and populates the global opt struct.
 void ParseOptions(int argc, char *argv[]) {
   vector<char *> args(argv, argv + argc);
   ParseCommandLine(ExpandArguments(args));
diff --git a/src/main/tools/linux-sandbox-options.h b/src/main/tools/linux-sandbox-options.h
index d67a14baa2..c190372d5c 100644
--- a/src/main/tools/linux-sandbox-options.h
+++ b/src/main/tools/linux-sandbox-options.h
@@ -12,8 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifndef LINUX_SANDBOX_OPTIONS_H__
-#define LINUX_SANDBOX_OPTIONS_H__
+#ifndef SRC_MAIN_TOOLS_LINUX_SANDBOX_OPTIONS_H_
+#define SRC_MAIN_TOOLS_LINUX_SANDBOX_OPTIONS_H_
 
 #include <stdbool.h>
 #include <stddef.h>
@@ -56,6 +56,7 @@ struct Options {
 
 extern struct Options opt;
 
+// Handles parsing all command line flags and populates the global opt struct.
 void ParseOptions(int argc, char *argv[]);
 
 #endif
diff --git a/src/main/tools/linux-sandbox-pid1.cc b/src/main/tools/linux-sandbox-pid1.cc
index 740c8de242..5c923e9522 100644
--- a/src/main/tools/linux-sandbox-pid1.cc
+++ b/src/main/tools/linux-sandbox-pid1.cc
@@ -17,20 +17,7 @@
  * mount, UTS, IPC and PID namespace.
  */
 
-#include "src/main/tools/linux-sandbox-options.h"
-#include "src/main/tools/linux-sandbox-utils.h"
-#include "src/main/tools/linux-sandbox.h"
-
-// Note that we define DIE() here and not in a shared header, because we want to
-// use _exit() in the
-// pid1 child, but exit() in the parent.
-#define DIE(args...)                                     \
-  {                                                      \
-    fprintf(stderr, __FILE__ ":" S__LINE__ ": \"" args); \
-    fprintf(stderr, "\": ");                             \
-    perror(nullptr);                                     \
-    _exit(EXIT_FAILURE);                                 \
-  }
+#include "src/main/tools/linux-sandbox-pid1.h"
 
 #include <errno.h>
 #include <fcntl.h>
@@ -53,9 +40,13 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
-
 #include <string>
 
+#include "src/main/tools/linux-sandbox-options.h"
+#include "src/main/tools/linux-sandbox.h"
+#include "src/main/tools/logging.h"
+#include "src/main/tools/process-tools.h"
+
 static int global_child_pid;
 
 static void SetupSelfDestruction(int *sync_pipe) {
@@ -68,6 +59,13 @@ static void SetupSelfDestruction(int *sync_pipe) {
     DIE("prctl");
   }
 
+  // Switch to a new process group, otherwise our process group will still refer
+  // to the outer PID namespace. We might then accidentally kill our parent by a
+  // call to e.g. `kill(0, sig)`.
+  if (setpgid(0, 0) < 0) {
+    DIE("setpgid");
+  }
+
   // Verify that the parent still lives.
   char buf = 0;
   if (close(sync_pipe[0]) < 0) {
@@ -301,8 +299,7 @@ static void SetupNetworking() {
       DIE("socket");
     }
 
-    struct ifreq ifr;
-    memset(&ifr, 0, sizeof(ifr));
+    struct ifreq ifr = {};
     strncpy(ifr.ifr_name, "lo", IF_NAMESIZE);
 
     // Verify that name is valid.
@@ -329,8 +326,7 @@ static void EnterSandbox() {
 }
 
 static void InstallSignalHandler(int signum, void (*handler)(int)) {
-  struct sigaction sa;
-  memset(&sa, 0, sizeof(sa));
+  struct sigaction sa = {};
   sa.sa_handler = handler;
   if (handler == SIG_IGN || handler == SIG_DFL) {
     // No point in blocking signals when using the default handler or ignoring
@@ -366,8 +362,7 @@ static void RestoreSignalHandlersAndMask() {
   }
 
   // Set the default signal handler for all signals.
-  struct sigaction sa;
-  memset(&sa, 0, sizeof(sa));
+  struct sigaction sa = {};
   if (sigemptyset(&sa.sa_mask) < 0) {
     DIE("sigemptyset");
   }
diff --git a/src/main/tools/linux-sandbox-pid1.h b/src/main/tools/linux-sandbox-pid1.h
index 507739a08e..a8260dbdba 100644
--- a/src/main/tools/linux-sandbox-pid1.h
+++ b/src/main/tools/linux-sandbox-pid1.h
@@ -12,8 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifndef LINUX_SANDBOX_PID1_H__
-#define LINUX_SANDBOX_PID1_H__
+#ifndef SRC_MAIN_TOOLS_LINUX_SANDBOX_PID1_H_
+#define SRC_MAIN_TOOLS_LINUX_SANDBOX_PID1_H_
 
 int Pid1Main(void *sync_pipe_param);
 
diff --git a/src/main/tools/linux-sandbox.cc b/src/main/tools/linux-sandbox.cc
index b58905e6af..0326d24dd6 100644
--- a/src/main/tools/linux-sandbox.cc
+++ b/src/main/tools/linux-sandbox.cc
@@ -37,13 +37,7 @@
  *    system are invisible.
  */
 
-#define DIE(args...)                                     \
-  {                                                      \
-    fprintf(stderr, __FILE__ ":" S__LINE__ ": \"" args); \
-    fprintf(stderr, "\": ");                             \
-    perror(nullptr);                                     \
-    exit(EXIT_FAILURE);                                  \
-  }
+#include "src/main/tools/linux-sandbox.h"
 
 #include <ctype.h>
 #include <dirent.h>
@@ -67,7 +61,8 @@
 
 #include "src/main/tools/linux-sandbox-options.h"
 #include "src/main/tools/linux-sandbox-pid1.h"
-#include "src/main/tools/linux-sandbox-utils.h"
+#include "src/main/tools/logging.h"
+#include "src/main/tools/process-tools.h"
 
 int global_outer_uid;
 int global_outer_gid;
@@ -80,6 +75,8 @@ static volatile sig_atomic_t global_next_timeout_signal = SIGTERM;
 // The signal that caused us to kill the child (e.g. on timeout).
 static volatile sig_atomic_t global_signal;
 
+// Make sure the child process does not inherit any accidentally left open file
+// handles from our parent.
 static void CloseFds() {
   DIR *fds = opendir("/proc/self/fd");
   if (fds == nullptr) {
@@ -117,18 +114,6 @@ static void CloseFds() {
   }
 }
 
-static void HandleSignal(int signum, void (*handler)(int)) {
-  struct sigaction sa;
-  memset(&sa, 0, sizeof(sa));
-  sa.sa_handler = handler;
-  if (sigemptyset(&sa.sa_mask) < 0) {
-    DIE("sigemptyset");
-  }
-  if (sigaction(signum, &sa, nullptr) < 0) {
-    DIE("sigaction");
-  }
-}
-
 static void OnTimeout(int sig) {
   global_signal = sig;
   kill(global_child_pid, global_next_timeout_signal);
@@ -210,31 +195,6 @@ static int WaitForPid1() {
   }
 }
 
-static void Redirect(const std::string &target_path, int fd) {
-  if (!target_path.empty() && target_path != "-") {
-    const int flags = O_WRONLY | O_CREAT | O_TRUNC | O_APPEND;
-    int fd_out = open(target_path.c_str(), flags, 0666);
-    if (fd_out < 0) {
-      DIE("open(%s)", target_path.c_str());
-    }
-    // If we were launched with less than 3 fds (stdin, stdout, stderr) open,
-    // but redirection is still requested via a command-line flag, something is
-    // wacky and the following code would not do what we intend to do, so let's
-    // bail.
-    if (fd_out < 3) {
-      DIE("open(%s) returned a handle that is reserved for stdin / stdout / "
-          "stderr",
-          target_path.c_str());
-    }
-    if (dup2(fd_out, fd) < 0) {
-      DIE("dup2()");
-    }
-    if (close(fd_out) < 0) {
-      DIE("close()");
-    }
-  }
-}
-
 int main(int argc, char *argv[]) {
   // Ask the kernel to kill us with SIGKILL if our parent dies.
   if (prctl(PR_SET_PDEATHSIG, SIGKILL) < 0) {
@@ -242,26 +202,19 @@ int main(int argc, char *argv[]) {
   }
 
   ParseOptions(argc, argv);
+  global_debug = opt.debug;
 
   Redirect(opt.stdout_path, STDOUT_FILENO);
   Redirect(opt.stderr_path, STDERR_FILENO);
 
-  // This should never be called as a setuid binary, drop privileges just in
-  // case. We don't need to be root, because we use user namespaces anyway.
-  if (setuid(getuid()) < 0) {
-    DIE("setuid");
-  }
-
   global_outer_uid = getuid();
   global_outer_gid = getgid();
 
-  // Make sure the sandboxed process does not inherit any accidentally left open
-  // file handles from our parent.
   CloseFds();
 
-  HandleSignal(SIGALRM, OnTimeout);
   if (opt.timeout_secs > 0) {
-    alarm(opt.timeout_secs);
+    HandleSignal(SIGALRM, OnTimeout);
+    SetTimeout(opt.timeout_secs);
   }
 
   SpawnPid1();
diff --git a/src/main/tools/linux-sandbox.h b/src/main/tools/linux-sandbox.h
index df07dee39f..18a744c612 100644
--- a/src/main/tools/linux-sandbox.h
+++ b/src/main/tools/linux-sandbox.h
@@ -12,8 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifndef LINUX_SANDBOX_H__
-#define LINUX_SANDBOX_H__
+#ifndef SRC_MAIN_TOOLS_LINUX_SANDBOX_H_
+#define SRC_MAIN_TOOLS_LINUX_SANDBOX_H_
 
 extern int global_outer_uid;
 extern int global_outer_gid;
diff --git a/src/main/tools/logging.cc b/src/main/tools/logging.cc
new file mode 100644
index 0000000000..d2a1ca8bc2
--- /dev/null
+++ b/src/main/tools/logging.cc
@@ -0,0 +1,17 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "src/main/tools/logging.h"
+
+bool global_debug = false;
diff --git a/src/main/tools/linux-sandbox-utils.h b/src/main/tools/logging.h
index 3a4a1bff6c..be646a7109 100644
--- a/src/main/tools/linux-sandbox-utils.h
+++ b/src/main/tools/logging.h
@@ -1,4 +1,4 @@
-// Copyright 2016 The Bazel Authors. All rights reserved.
+// Copyright 2017 The Bazel Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -12,19 +12,32 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifndef LINUX_SANDBOX_UTILS_H__
-#define LINUX_SANDBOX_UTILS_H__
+#ifndef SRC_MAIN_TOOLS_LOGGING_H_
+#define SRC_MAIN_TOOLS_LOGGING_H_
 
+// see
+// http://stackoverflow.com/questions/5641427/how-to-make-preprocessor-generate-a-string-for-line-keyword
 #define S(x) #x
 #define S_(x) S(x)
 #define S__LINE__ S_(__LINE__)
 
+#define DIE(...)                                                \
+  {                                                             \
+    fprintf(stderr, __FILE__ ":" S__LINE__ ": \"" __VA_ARGS__); \
+    fprintf(stderr, "\": ");                                    \
+    perror(nullptr);                                            \
+    exit(EXIT_FAILURE);                                         \
+  }
+
 #define PRINT_DEBUG(...)                                        \
   do {                                                          \
-    if (opt.debug) {                                            \
+    if (global_debug) {                                         \
       fprintf(stderr, __FILE__ ":" S__LINE__ ": " __VA_ARGS__); \
       fprintf(stderr, "\n");                                    \
     }                                                           \
   } while (0)
 
-#endif
+// Set to `true` to let PRINT_DEBUG() print messages.
+extern bool global_debug;
+
+#endif  // SRC_MAIN_TOOLS_LOGGING_H_
diff --git a/src/main/tools/process-tools.cc b/src/main/tools/process-tools.cc
index a25dbeeb1d..0c77611e25 100644
--- a/src/main/tools/process-tools.cc
+++ b/src/main/tools/process-tools.cc
@@ -27,7 +27,7 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
-extern bool global_debug;
+#include "src/main/tools/logging.h"
 
 int SwitchToEuid() {
   int uid = getuid();
@@ -143,10 +143,6 @@ void ClearSignalMask() {
 }
 
 void SetTimeout(double timeout_secs) {
-  if (timeout_secs <= 0) {
-    return;
-  }
-
   double int_val, fraction_val;
   fraction_val = modf(timeout_secs, &int_val);
 
diff --git a/src/main/tools/process-tools.h b/src/main/tools/process-tools.h
index 7a8554d4ea..ef8f3fbbcd 100644
--- a/src/main/tools/process-tools.h
+++ b/src/main/tools/process-tools.h
@@ -12,35 +12,13 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifndef PROCESS_TOOLS_H__
-#define PROCESS_TOOLS_H__
+#ifndef SRC_MAIN_TOOLS_PROCESS_TOOLS_H_
+#define SRC_MAIN_TOOLS_PROCESS_TOOLS_H_
 
 #include <stdbool.h>
 #include <sys/types.h>
 #include <string>
 
-// see
-// http://stackoverflow.com/questions/5641427/how-to-make-preprocessor-generate-a-string-for-line-keyword
-#define S(x) #x
-#define S_(x) S(x)
-#define S__LINE__ S_(__LINE__)
-
-#define DIE(...)                                                \
-  {                                                             \
-    fprintf(stderr, __FILE__ ":" S__LINE__ ": \"" __VA_ARGS__); \
-    fprintf(stderr, "\": ");                                    \
-    perror(nullptr);                                            \
-    exit(EXIT_FAILURE);                                         \
-  }
-
-#define PRINT_DEBUG(...)                                        \
-  do {                                                          \
-    if (global_debug) {                                         \
-      fprintf(stderr, __FILE__ ":" S__LINE__ ": " __VA_ARGS__); \
-      fprintf(stderr, "\n");                                    \
-    }                                                           \
-  } while (0)
-
 // Switch completely to the effective uid.
 // Some programs (notably, bash) ignore the euid and just use the uid. This
 // limits the ability for us to use process-wrapper as a setuid binary for
diff --git a/src/main/tools/process-wrapper.cc b/src/main/tools/process-wrapper.cc
index 6140eba713..c2eeee2b89 100644
--- a/src/main/tools/process-wrapper.cc
+++ b/src/main/tools/process-wrapper.cc
@@ -36,10 +36,9 @@
 #include <string>
 #include <vector>
 
+#include "src/main/tools/logging.h"
 #include "src/main/tools/process-tools.h"
 
-bool global_debug = false;
-
 static double global_kill_delay;
 static pid_t global_child_pid;
 static volatile sig_atomic_t global_signal;
@@ -136,7 +135,9 @@ static void SpawnCommand(const std::vector<char *> &args, double timeout_secs) {
     HandleSignal(SIGALRM, OnSignal);
     HandleSignal(SIGTERM, OnSignal);
     HandleSignal(SIGINT, OnSignal);
-    SetTimeout(timeout_secs);
+    if (timeout_secs > 0) {
+      SetTimeout(timeout_secs);
+    }
 
     int status = WaitChild(global_child_pid);