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
bazelbuild · May 19, 2017 · 67cea8f · 67cea8f
1 parent d922e65
commit 67cea8f
Show file tree

Hide file tree

Showing 12 changed files with 100 additions and 137 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,58 +195,26 @@ 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) {
     DIE("prctl");
   }
 
   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
@@ -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
@@ -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;