Only wipe the whole sandbox base during the first build in a server.
During the lifetime of a Bazel server, assign unique identifiers to
each sandboxed action so that their symlink trees are guaranteed to be
disjoint as they are keyed on that identifier. This was in theory
already happening... but it actually wasn't and there was no test to
validate this assumption.
With that done, there is no need to ensure that the sandbox base is clean
before a build -- unless we are the very first build of a server, in which
case we must ensure we don't clash with possible leftovers from a past
server.
Note that we already clean up each action's tree as soon as the action
completes, so the only thing we are trying to clean up here are stale
files that may be left if those individual deletions didn't work (e.g.
because there were still stray processes writing to the directories)
or if --sandbox_debug was in use.
This is a prerequisite before making deletions asynchronous for two
reasons: first, we don't want to delay build starts if old deletions are
still ongoing; and, second, we don't want to schedule too broad
deletions that could step over subsequent builds (i.e. we only want to
delete the contents of the *-sandbox directories, which contain one
subdirectory per action, and not the whole tree).
Lastly, add a test for this expected behavior (which is what actually
triggered the fix) and for the fact that we expect the identifiers to
be always different.
Partially addresses https://github.com/bazelbuild/bazel/issues/7527.
RELNOTES: None.
PiperOrigin-RevId: 243635557
diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
index 0504f1b..d3aaa3c 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
@@ -54,9 +54,17 @@
/** Abstract common ancestor for spawn strategies implementing the common parts. */
public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionContext {
+
+ /**
+ * Last unique identifier assigned to a spawn by this strategy.
+ *
+ * <p>These identifiers must be unique per strategy within the context of a Bazel server instance
+ * to avoid cross-contamination across actions in case we perform asynchronous deletions.
+ */
+ private static final AtomicInteger execCount = new AtomicInteger();
+
private final SpawnInputExpander spawnInputExpander;
private final SpawnRunner spawnRunner;
- private final AtomicInteger execCount = new AtomicInteger();
public AbstractSpawnStrategy(Path execRoot, SpawnRunner spawnRunner) {
this.spawnInputExpander = new SpawnInputExpander(execRoot, false);
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java
index c3aee0f..08dccc5 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java
@@ -23,6 +23,7 @@
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
import com.google.devtools.build.lib.util.io.FileOutErr;
+import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.time.Duration;
@@ -241,6 +242,18 @@
/** Returns whether this SpawnRunner supports executing the given Spawn. */
boolean canExec(Spawn spawn);
- /* Name of the SpawnRunner. */
+ /** Returns the name of the SpawnRunner. */
String getName();
+
+ /**
+ * Removes any files or directories that this spawn runner may have put in the sandbox base.
+ *
+ * <p>It is important that this function only removes entries that may have been generated by this
+ * build, not any possible entries that a future build may generate.
+ *
+ * @param sandboxBase path to the base of the sandbox tree where the spawn runner may have created
+ * entries
+ * @throws IOException if there are problems deleting the entries
+ */
+ default void cleanupSandboxBase(Path sandboxBase) throws IOException {}
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
index 6bccf16..2829b64 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
@@ -67,7 +67,7 @@
@Override
public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
- throws ExecException, InterruptedException {
+ throws ExecException, IOException, InterruptedException {
ActionExecutionMetadata owner = spawn.getResourceOwner();
context.report(ProgressStatus.SCHEDULING, getName());
try (ResourceHandle ignored =
@@ -288,4 +288,14 @@
protected SandboxOptions getSandboxOptions() {
return sandboxOptions;
}
+
+ @Override
+ public void cleanupSandboxBase(Path sandboxBase) throws IOException {
+ Path root = sandboxBase.getChild(getName());
+ if (root.exists()) {
+ for (Path child : root.getDirectoryEntries()) {
+ child.deleteTree();
+ }
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
index d9309be..37672d6 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
@@ -321,4 +321,18 @@
}
}
}
+
+ @Override
+ public void cleanupSandboxBase(Path sandboxBase) throws IOException {
+ if (inaccessibleHelperDir.exists()) {
+ inaccessibleHelperDir.chmod(0700);
+ inaccessibleHelperDir.deleteTree();
+ }
+ if (inaccessibleHelperFile.exists()) {
+ inaccessibleHelperFile.chmod(0600);
+ inaccessibleHelperFile.delete();
+ }
+
+ super.cleanupSandboxBase(sandboxBase);
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
index f2e17f9..22e3079 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
@@ -52,6 +52,8 @@
import java.io.File;
import java.io.IOException;
import java.time.Duration;
+import java.util.HashSet;
+import java.util.Set;
import javax.annotation.Nullable;
/**
@@ -59,6 +61,9 @@
*/
public final class SandboxModule extends BlazeModule {
+ /** Tracks whether we are issuing the very first build within this Bazel server instance. */
+ private static boolean firstBuild = true;
+
/** Environment for the running command. */
private @Nullable CommandEnvironment env;
@@ -69,6 +74,15 @@
private @Nullable SandboxfsProcess sandboxfsProcess;
/**
+ * Collection of spawn runner instantiated during the executor setup.
+ *
+ * <p>We need this information to clean up the heavy subdirectories of the sandbox base on build
+ * completion but to avoid wiping the whole sandbox base itself, which could be problematic across
+ * builds.
+ */
+ private final Set<SpawnRunner> spawnRunners = new HashSet<>();
+
+ /**
* Whether to remove the sandbox worker directories after a build or not. Useful for debugging
* to inspect the state of files on failures.
*/
@@ -163,9 +177,6 @@
Path mountPoint = sandboxBase.getRelative("sandboxfs");
- // Ensure that each build starts with a clean sandbox base directory. Otherwise using the `id`
- // that is provided by SpawnExecutionPolicy#getId to compute a base directory for a sandbox
- // might result in an already existing directory.
if (sandboxfsProcess != null) {
if (options.sandboxDebug) {
env.getReporter()
@@ -178,9 +189,15 @@
sandboxfsProcess.destroy();
sandboxfsProcess = null;
}
- if (sandboxBase.exists()) {
+ // SpawnExecutionPolicy#getId returns unique base directories for each sandboxed action during
+ // the life of a Bazel server instance so we don't need to worry about stale directories from
+ // previous builds. However, on the very first build of an instance of the server, we must
+ // wipe old contents to avoid reusing stale directories.
+ if (firstBuild && sandboxBase.exists()) {
+ cmdEnv.getReporter().handle(Event.info("Deleting stale sandbox base " + sandboxBase));
sandboxBase.deleteTree();
}
+ firstBuild = false;
PathFragment sandboxfsPath = PathFragment.create(options.sandboxfsPath);
boolean useSandboxfs;
@@ -215,6 +232,7 @@
cmdEnv,
new ProcessWrapperSandboxedSpawnRunner(
cmdEnv, sandboxBase, cmdEnv.getRuntime().getProductName(), timeoutKillDelay));
+ spawnRunners.add(spawnRunner);
builder.addActionContext(
new ProcessWrapperSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner));
}
@@ -238,6 +256,7 @@
defaultImage,
timeoutKillDelay,
useCustomizedImages));
+ spawnRunners.add(spawnRunner);
builder.addActionContext(
new DockerSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner));
}
@@ -258,6 +277,7 @@
timeoutKillDelay,
sandboxfsProcess,
options.sandboxfsMapSymlinkTargets));
+ spawnRunners.add(spawnRunner);
builder.addActionContext(new LinuxSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner));
}
@@ -272,6 +292,7 @@
timeoutKillDelay,
sandboxfsProcess,
options.sandboxfsMapSymlinkTargets));
+ spawnRunners.add(spawnRunner);
builder.addActionContext(new DarwinSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner));
}
@@ -362,6 +383,12 @@
public boolean canExec(Spawn spawn) {
return sandboxSpawnRunner.canExec(spawn) || fallbackSpawnRunner.canExec(spawn);
}
+
+ @Override
+ public void cleanupSandboxBase(Path sandboxBase) throws IOException {
+ sandboxSpawnRunner.cleanupSandboxBase(sandboxBase);
+ fallbackSpawnRunner.cleanupSandboxBase(sandboxBase);
+ }
}
/**
@@ -405,10 +432,13 @@
if (shouldCleanupSandboxBase) {
try {
- sandboxBase.deleteTree();
+ checkNotNull(sandboxBase, "shouldCleanupSandboxBase implies sandboxBase has been set");
+ for (SpawnRunner spawnRunner : spawnRunners) {
+ spawnRunner.cleanupSandboxBase(sandboxBase);
+ }
} catch (IOException e) {
- env.getReporter().handle(Event.warn("Failed to delete sandbox base " + sandboxBase
- + ": " + e));
+ env.getReporter()
+ .handle(Event.warn("Failed to delete contents of sandbox " + sandboxBase + ": " + e));
}
shouldCleanupSandboxBase = false;
diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh
index 72a5b45..d2fa83d 100755
--- a/src/test/shell/bazel/bazel_sandboxing_test.sh
+++ b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -225,19 +225,6 @@
|| fail "Genrule did not produce output: examples/genrule:tools_work"
}
-# Make sure that sandboxed execution doesn't accumulate files in the
-# sandbox directory.
-function test_sandbox_cleanup() {
- bazel --batch clean &> $TEST_log \
- || fail "bazel clean failed"
- bazel build examples/genrule:tools_work &> $TEST_log \
- || fail "Hermetic genrule failed: examples/genrule:tools_work"
- bazel shutdown &> $TEST_log || fail "bazel shutdown failed"
- if [[ -n "$(ls -A "$(bazel info output_base)/sandbox")" ]]; then
- fail "Build left files around afterwards"
- fi
-}
-
# Test for #400: Linux sandboxing and relative symbolic links.
#
# let A = examples/genrule/symlinks/a/b/x.txt -> ../x.txt
diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD
index 36e1a57..1d9dd96 100644
--- a/src/test/shell/integration/BUILD
+++ b/src/test/shell/integration/BUILD
@@ -549,6 +549,16 @@
tags = ["no_windows"],
)
+sh_test(
+ name = "sandboxing_test",
+ size = "large",
+ srcs = ["sandboxing_test.sh"],
+ data = [
+ ":test-deps",
+ ],
+ tags = ["no_windows"],
+)
+
package_group(
name = "spend_cpu_time_users",
packages = [
diff --git a/src/test/shell/integration/sandboxing_test.sh b/src/test/shell/integration/sandboxing_test.sh
new file mode 100755
index 0000000..0542cc5
--- /dev/null
+++ b/src/test/shell/integration/sandboxing_test.sh
@@ -0,0 +1,115 @@
+#!/bin/bash
+#
+# Copyright 2019 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.
+#
+# Test sandboxing spawn strategy
+#
+
+set -euo pipefail
+
+# Load the test setup defined in the parent directory
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "${CURRENT_DIR}/../integration_test_setup.sh" \
+ || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+function tear_down() {
+ bazel clean --expunge
+ bazel shutdown
+ rm -rf pkg
+}
+
+function test_sandbox_base_contents_wiped_only_on_startup() {
+ mkdir pkg
+ cat >pkg/BUILD <<EOF
+genrule(name = "pkg", outs = ["pkg.out"], cmd = "echo >\$@")
+EOF
+
+ local output_base="$(bazel info output_base)"
+
+ do_build() {
+ bazel build --genrule_strategy=sandboxed //pkg
+ }
+
+ do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
+ find "${output_base}" >>"${TEST_log}" 2>&1 || true
+
+ local sandbox_dir="$(echo "${output_base}/sandbox"/*-sandbox)"
+ [[ -d "${sandbox_dir}" ]] \
+ || fail "${sandbox_dir} is missing; prematurely deleted?"
+
+ local garbage="${output_base}/sandbox/garbage"
+ mkdir -p "${garbage}/some/nested/contents"
+ do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
+ expect_not_log "Deleting stale sandbox"
+ [[ -d "${garbage}" ]] \
+ || fail "Spurious contents deleted from sandbox base too early"
+
+ bazel shutdown
+ do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
+ expect_log "Deleting stale sandbox"
+ [[ ! -d "${garbage}" ]] \
+ || fail "sandbox base was not cleaned on restart"
+}
+
+function test_sandbox_base_can_be_rm_rfed() {
+ mkdir pkg
+ cat >pkg/BUILD <<EOF
+genrule(name = "pkg", outs = ["pkg.out"], cmd = "echo >\$@")
+EOF
+
+ local output_base="$(bazel info output_base)"
+
+ do_build() {
+ bazel build --genrule_strategy=sandboxed //pkg
+ }
+
+ do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
+ find "${output_base}" >>"${TEST_log}" 2>&1 || true
+
+ local sandbox_base="${output_base}/sandbox"
+ [[ -d "${sandbox_base}" ]] \
+ || fail "${sandbox_base} is missing; build did not use sandboxing?"
+
+ # Ensure the sandbox base does not contain protected files that would prevent
+ # a simple "rm -rf" from working under an unprivileged user.
+ rm -rf "${sandbox_base}" || fail "Cannot clean sandbox base"
+
+ # And now ensure Bazel reconstructs the sandbox base on a second build.
+ do_build >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
+}
+
+function test_sandbox_old_contents_not_reused_in_consecutive_builds() {
+ mkdir pkg
+ cat >pkg/BUILD <<EOF
+genrule(
+ name = "pkg",
+ srcs = ["pkg.in"],
+ outs = ["pkg.out"],
+ cmd = "cp \$(location :pkg.in) \$@",
+)
+EOF
+ touch pkg/pkg.in
+
+ for i in $(seq 5); do
+ # Ensure that, even if we don't clean up the sandbox at all (with
+ # --sandbox_debug), consecutive builds don't step on each other by trying to
+ # reuse previous spawn identifiers.
+ bazel build --genrule_strategy=sandboxed --sandbox_debug //pkg \
+ >"${TEST_log}" 2>&1 || fail "Expected build to succeed"
+ echo foo >>pkg/pkg.in
+ done
+}
+
+run_suite "sandboxing"
