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
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 index 6c42370..a297ebe 100644 --- 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 @@ 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 @@ 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 index 926e23a..6a892da 100644 --- 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 @@ 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 @@ sandboxPath, sandboxExecRoot, getWritableDirs(sandboxExecRoot, spawnEnvironment), - getInaccessiblePaths(), runUnderPath, verboseFailures); try { @@ -285,15 +290,6 @@ } @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 { try {
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 index 2144c44..122bd76 100644 --- 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 @@ 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 @@ Path sandboxExecRoot, Path sandboxTempDir, Set<Path> writableDirs, - Set<Path> inaccessiblePaths, Set<Path> tmpfsPaths, Map<Path, Path> bindMounts, boolean verboseFailures, @@ -66,7 +64,6 @@ 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 @@ 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 index e7d742c..7ab7900 100644 --- 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 @@ 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 index 0618c85..303ec0f 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
@@ -94,15 +94,6 @@ 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, defaultValue = "",
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 index fa33ec2..ffa0ff4 100644 --- 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 @@ boolean verboseFailures, SandboxOptions sandboxOptions) { this.buildRequest = buildRequest; - this.blazeDirs = blazeDirs; this.execRoot = blazeDirs.getExecRoot(); this.verboseFailures = verboseFailures; this.sandboxOptions = sandboxOptions; @@ -131,14 +129,6 @@ 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 index 7db09f0..3bd9a7e 100644 --- a/src/main/tools/linux-sandbox-options.cc +++ b/src/main/tools/linux-sandbox-options.cc
@@ -66,8 +66,6 @@ " -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 @@ 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 @@ 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 index 342ed12..6f57eba 100644 --- a/src/main/tools/linux-sandbox-options.h +++ b/src/main/tools/linux-sandbox-options.h
@@ -36,8 +36,6 @@ 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 index 2aba127..0be62d9 100644 --- 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 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 @@ 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 index e450ccd..799ece4 100644 --- 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 index 8e393d8..f85d33a 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -298,23 +298,6 @@ || 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