sandbox: Remove the flag --sandbox_block_path.

It is in the way of optimizing the performance of the sandbox, because it requires us to create two helper files (an unreadable file and an unreadable directory) which are bind-mounted on top of paths specified via this flag. These two helper files were created on a tmpfs mounted by the sandbox until now, which ensured that they were automatically deleted on exit. However, mounting tmpfs on /dev/shm or /tmp causes issues like #2686 or #1882. By removing this flag, we can get rid of the two helper files, which means we can also remove the reliance on a "sandbox temp directory" completely in the next change. -- PiperOrigin-RevId: 151107496 MOS_MIGRATED_REVID=151107496
bazelbuild · Mar 24, 2017 · 3e2329a · 3e2329a
1 parent 2879971
commit 3e2329a
Show file tree

Hide file tree

Showing 11 changed files with 11 additions and 123 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxRunner.java
@@ -44,21 +44,18 @@ final class DarwinSandboxRunner extends SandboxRunner {
   private final Path sandboxExecRoot;
   private final Path argumentsFilePath;
   private final Set<Path> writableDirs;
-  private final Set<Path> inaccessiblePaths;
   private final Path runUnderPath;
 
   DarwinSandboxRunner(
       Path sandboxPath,
       Path sandboxExecRoot,
       Set<Path> writableDirs,
-      Set<Path> inaccessiblePaths,
       Path runUnderPath,
       boolean verboseFailures) {
     super(verboseFailures);
     this.sandboxExecRoot = sandboxExecRoot;
     this.argumentsFilePath = sandboxPath.getRelative("sandbox.sb");
     this.writableDirs = writableDirs;
-    this.inaccessiblePaths = inaccessiblePaths;
     this.runUnderPath = runUnderPath;
   }
 
@@ -141,9 +138,6 @@ private void writeConfig(boolean allowNetwork) throws IOException {
       out.println("(allow network* (local ip \"localhost:*\"))");
       out.println("(allow network* (remote ip \"localhost:*\"))");
 
-      for (Path inaccessiblePath : inaccessiblePaths) {
-        out.println("(deny file-read* (subpath \"" + inaccessiblePath + "\"))");
-      }
       if (runUnderPath != null) {
         out.println("(allow file-read* (subpath \"" + runUnderPath + "\"))");
       }

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
@@ -50,7 +50,9 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.SearchPath;
 import com.google.devtools.build.lib.vfs.Symlinks;
+import java.io.BufferedWriter;
 import java.io.IOException;
+import java.io.OutputStreamWriter;
 import java.io.PrintWriter;
 import java.util.HashMap;
 import java.util.List;
@@ -184,10 +186,14 @@ private void actuallyExec(
     Executor executor = actionExecutionContext.getExecutor();
     SandboxHelpers.reportSubcommand(executor, spawn);
 
-    PrintWriter errWriter =
-        sandboxDebug
-            ? new PrintWriter(actionExecutionContext.getFileOutErr().getErrorStream())
-            : null;
+    PrintWriter errWriter = null;
+    if (sandboxDebug) {
+      errWriter =
+          new PrintWriter(
+              new BufferedWriter(
+                  new OutputStreamWriter(
+                      actionExecutionContext.getFileOutErr().getErrorStream(), UTF_8)));
+    }
 
     // Each invocation of "exec" gets its own sandbox.
     Path sandboxPath = SandboxHelpers.getSandboxRoot(blazeDirs, productName, uuid, execCounter);
@@ -225,7 +231,6 @@ private void actuallyExec(
             sandboxPath,
             sandboxExecRoot,
             getWritableDirs(sandboxExecRoot, spawnEnvironment),
-            getInaccessiblePaths(),
             runUnderPath,
             verboseFailures);
     try {
@@ -284,15 +289,6 @@ protected ImmutableSet<Path> getWritableDirs(Path sandboxExecRoot, Map<String, S
     return writableDirs.build();
   }
 
-  @Override
-  protected ImmutableSet<Path> getInaccessiblePaths() {
-    ImmutableSet.Builder<Path> inaccessiblePaths = ImmutableSet.builder();
-    inaccessiblePaths.addAll(super.getInaccessiblePaths());
-    inaccessiblePaths.add(blazeDirs.getWorkspace());
-    inaccessiblePaths.add(execRoot);
-    return inaccessiblePaths.build();
-  }
-
   @Override
   public Map<PathFragment, Path> getMounts(Spawn spawn, ActionExecutionContext executionContext)
       throws ExecException {

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxRunner.java
@@ -43,7 +43,6 @@ final class LinuxSandboxRunner extends SandboxRunner {
   private final Path sandboxTempDir;
   private final Path argumentsFilePath;
   private final Set<Path> writableDirs;
-  private final Set<Path> inaccessiblePaths;
   private final Set<Path> tmpfsPaths;
   // a <target, source> mapping of paths to bind mount
   private final Map<Path, Path> bindMounts;
@@ -55,7 +54,6 @@ final class LinuxSandboxRunner extends SandboxRunner {
       Path sandboxExecRoot,
       Path sandboxTempDir,
       Set<Path> writableDirs,
-      Set<Path> inaccessiblePaths,
       Set<Path> tmpfsPaths,
       Map<Path, Path> bindMounts,
       boolean verboseFailures,
@@ -66,7 +64,6 @@ final class LinuxSandboxRunner extends SandboxRunner {
     this.sandboxTempDir = sandboxTempDir;
     this.argumentsFilePath = sandboxPath.getRelative("linux-sandbox.params");
     this.writableDirs = writableDirs;
-    this.inaccessiblePaths = inaccessiblePaths;
     this.tmpfsPaths = tmpfsPaths;
     this.bindMounts = bindMounts;
     this.sandboxDebug = sandboxDebug;
@@ -151,11 +148,6 @@ private void writeConfig(int timeout, boolean allowNetwork, boolean useFakeHostn
       fileArgs.add(writablePath.getPathString());
     }
 
-    for (Path inaccessiblePath : inaccessiblePaths) {
-      fileArgs.add("-i");
-      fileArgs.add(inaccessiblePath.getPathString());
-    }
-
     for (Path tmpfsPath : tmpfsPaths) {
       fileArgs.add("-e");
       fileArgs.add(tmpfsPath.getPathString());

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
@@ -180,7 +180,6 @@ private SandboxRunner getSandboxRunner(
           sandboxExecRoot,
           sandboxTempDir,
           getWritableDirs(sandboxExecRoot, spawn.getEnvironment()),
-          getInaccessiblePaths(),
           getTmpfsPaths(),
           getReadOnlyBindMounts(blazeDirs, sandboxExecRoot),
           verboseFailures,

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
@@ -93,15 +93,6 @@ public String getTypeDescription() {
   )
   public boolean sandboxFakeHostname;
 
-  @Option(
-    name = "sandbox_block_path",
-    allowMultiple = true,
-    defaultValue = "",
-    category = "config",
-    help = "For sandboxed actions, disallow access to this path."
-  )
-  public List<String> sandboxBlockPath;
-
   @Option(
       name = "sandbox_tmpfs_path",
       allowMultiple = true,

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java
@@ -47,7 +47,6 @@
 abstract class SandboxStrategy implements SandboxedSpawnActionContext {
 
   private final BuildRequest buildRequest;
-  private final BlazeDirectories blazeDirs;
   private final Path execRoot;
   private final boolean verboseFailures;
   private final SandboxOptions sandboxOptions;
@@ -59,7 +58,6 @@ public SandboxStrategy(
       boolean verboseFailures,
       SandboxOptions sandboxOptions) {
     this.buildRequest = buildRequest;
-    this.blazeDirs = blazeDirs;
     this.execRoot = blazeDirs.getExecRoot();
     this.verboseFailures = verboseFailures;
     this.sandboxOptions = sandboxOptions;
@@ -131,14 +129,6 @@ protected ImmutableSet<Path> getWritableDirs(Path sandboxExecRoot, Map<String, S
     return writableDirs.build();
   }
 
-  protected ImmutableSet<Path> getInaccessiblePaths() {
-    ImmutableSet.Builder<Path> inaccessiblePaths = ImmutableSet.builder();
-    for (String path : sandboxOptions.sandboxBlockPath) {
-      inaccessiblePaths.add(blazeDirs.getFileSystem().getPath(path));
-    }
-    return inaccessiblePaths.build();
-  }
-
   @Override
   public String toString() {
     return "sandboxed";

diff --git a/src/main/tools/linux-sandbox-options.cc b/src/main/tools/linux-sandbox-options.cc
@@ -66,8 +66,6 @@ static void Usage(char *program_name, const char *fmt, ...) {
           "  -L <file>  redirect stderr to a file\n"
           "  -w <file>  make a file or directory writable for the sandboxed "
           "process\n"
-          "  -i <file>  make a file or directory inaccessible for the "
-          "sandboxed process\n"
           "  -e <dir>  mount an empty tmpfs on a directory\n"
           "  -M/-m <source/target>  directory to mount inside the sandbox\n"
           "    Multiple directories can be specified and each of them will be "
@@ -126,7 +124,7 @@ static void ParseCommandLine(unique_ptr<vector<char *>> args) {
   bool source_specified;
 
   while ((c = getopt(args->size(), args->data(),
-                     ":CS:W:T:t:l:L:w:i:e:M:m:HNRD")) != -1) {
+                     ":CS:W:T:t:l:L:w:e:M:m:HNRD")) != -1) {
     if (c != 'M' && c != 'm') source_specified = false;
     switch (c) {
       case 'C':
@@ -183,10 +181,6 @@ static void ParseCommandLine(unique_ptr<vector<char *>> args) {
         ValidateIsAbsolutePath(optarg, args->front(), static_cast<char>(c));
         opt.writable_files.push_back(strdup(optarg));
         break;
-      case 'i':
-        ValidateIsAbsolutePath(optarg, args->front(), static_cast<char>(c));
-        opt.inaccessible_files.push_back(strdup(optarg));
-        break;
       case 'e':
         ValidateIsAbsolutePath(optarg, args->front(), static_cast<char>(c));
         opt.tmpfs_dirs.push_back(strdup(optarg));

diff --git a/src/main/tools/linux-sandbox-options.h b/src/main/tools/linux-sandbox-options.h
@@ -36,8 +36,6 @@ struct Options {
   const char *stderr_path;
   // Files or directories to make writable for the sandboxed process (-w)
   std::vector<const char *> writable_files;
-  // Files or directories to make inaccessible for the sandboxed process (-i)
-  std::vector<const char *> inaccessible_files;
   // Directories where to mount an empty tmpfs (-e)
   std::vector<const char *> tmpfs_dirs;
   // Source of files or directories to explicitly bind mount in the sandbox (-M)

diff --git a/src/main/tools/linux-sandbox-pid1.cc b/src/main/tools/linux-sandbox-pid1.cc
@@ -55,8 +55,6 @@
 #include <unistd.h>
 
 static int global_child_pid;
-static char global_inaccessible_directory[] = "tmp/empty.XXXXXX";
-static char global_inaccessible_file[] = "tmp/empty.XXXXXX";
 
 static void SetupSelfDestruction(int *sync_pipe) {
   // We could also poll() on the pipe fd to find out when the parent goes away,
@@ -147,26 +145,6 @@ static void SetupUtsNamespace() {
   }
 }
 
-static void SetupHelperFiles() {
-  if (mkdtemp(global_inaccessible_directory) == NULL) {
-    DIE("mkdtemp(%s)", global_inaccessible_directory);
-  }
-  if (chmod(global_inaccessible_directory, 0) < 0) {
-    DIE("chmod(%s, 0)", global_inaccessible_directory);
-  }
-
-  int handle = mkstemp(global_inaccessible_file);
-  if (handle < 0) {
-    DIE("mkstemp(%s)", global_inaccessible_file);
-  }
-  if (fchmod(handle, 0)) {
-    DIE("fchmod(%s, 0)", global_inaccessible_file);
-  }
-  if (close(handle) < 0) {
-    DIE("close(%s)", global_inaccessible_file);
-  }
-}
-
 // Recursively creates the file or directory specified in "path" and its parent
 // directories.
 static int CreateTarget(const char *path, bool is_directory) {
@@ -265,31 +243,6 @@ static void MountFilesystems() {
           writable_file + 1);
     }
   }
-
-  SetupHelperFiles();
-
-  for (const char *inaccessible_file : opt.inaccessible_files) {
-    struct stat sb;
-    if (stat(inaccessible_file, &sb) < 0) {
-      DIE("stat(%s)", inaccessible_file);
-    }
-
-    if (S_ISDIR(sb.st_mode)) {
-      PRINT_DEBUG("inaccessible dir: %s", inaccessible_file);
-      if (mount(global_inaccessible_directory, inaccessible_file + 1, NULL,
-                MS_BIND, NULL) < 0) {
-        DIE("mount(%s, %s, NULL, MS_BIND, NULL)", global_inaccessible_directory,
-            inaccessible_file + 1);
-      }
-    } else {
-      PRINT_DEBUG("inaccessible file: %s", inaccessible_file);
-      if (mount(global_inaccessible_file, inaccessible_file + 1, NULL, MS_BIND,
-                NULL) < 0) {
-        DIE("mount(%s, %s, NULL, MS_BIND, NULL", global_inaccessible_file,
-            inaccessible_file + 1);
-      }
-    }
-  }
 }
 
 // We later remount everything read-only, except the paths for which this method

diff --git a/src/main/tools/linux-sandbox.cc b/src/main/tools/linux-sandbox.cc
@@ -20,8 +20,6 @@
  *  - The working directory (-W) will be made read-write, though.
  *  - Individual files or directories can be made writable (but not deletable)
  *    (-w).
- *  - Individual files or directories can be made inaccessible / unreadable
- *    (-i).
  *  - tmpfs will be mounted on /tmp.
  *  - tmpfs can be mounted on top of existing directories (-e), too.
  *  - If the process takes longer than the timeout (-T), it will be killed with

diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -298,23 +298,6 @@ function test_sandbox_undeclared_deps_skylark_with_local_tag() {
     || fail "Action did not produce output: examples/genrule:skylark_breaks1_works_with_local_tag"
 }
 
-function test_sandbox_block_filesystem() {
-  output_file="${BAZEL_GENFILES_DIR}/examples/genrule/breaks2.txt"
-
-  bazel build --sandbox_block_path=/var/log examples/genrule:breaks2 &> $TEST_log \
-    && fail "Non-hermetic genrule succeeded: examples/genrule:breaks2" || true
-
-  [ -f "$output_file" ] ||
-    fail "Action did not produce output: $output_file"
-
-  if [ $(wc -l $output_file) -gt 1 ]; then
-    fail "Output contained more than one line: $output_file"
-  fi
-
-  fgrep "Permission denied" $output_file ||
-    fail "Output did not contain expected error message: $output_file"
-}
-
 function test_sandbox_cyclic_symlink_in_inputs() {
   bazel build examples/genrule:breaks3 &> $TEST_log \
     && fail "Genrule with cyclic symlinks succeeded: examples/genrule:breaks3" || true