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 0504f1b1521979..d3aaa3c2b0aa5e 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. + * + *

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 c3aee0fc55b789..08dccc57e0d727 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 @@ SpawnResult exec(Spawn spawn, SpawnExecutionContext context) /** 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. + * + *

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 6bccf16400f619..2829b64b0c10fc 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 @@ public AbstractSandboxSpawnRunner(CommandEnvironment cmdEnv) { @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 ImmutableSet getInaccessiblePaths() { 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 d9309be92a3158..37672d68479a84 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 @@ private void validateBindMounts(SortedMap bindMounts) throws UserExe } } } + + @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 f2e17f98baee07..22e3079a956d91 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; @@ -68,6 +73,15 @@ public final class SandboxModule extends BlazeModule { /** Instance of the sandboxfs process in use, if enabled. */ private @Nullable SandboxfsProcess sandboxfsProcess; + /** + * Collection of spawn runner instantiated during the executor setup. + * + *

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 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 @@ private void setup(CommandEnvironment cmdEnv, ExecutorBuilder builder) 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 @@ private void setup(CommandEnvironment cmdEnv, ExecutorBuilder builder) 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 @@ private void setup(CommandEnvironment cmdEnv, ExecutorBuilder builder) cmdEnv, new ProcessWrapperSandboxedSpawnRunner( cmdEnv, sandboxBase, cmdEnv.getRuntime().getProductName(), timeoutKillDelay)); + spawnRunners.add(spawnRunner); builder.addActionContext( new ProcessWrapperSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner)); } @@ -238,6 +256,7 @@ private void setup(CommandEnvironment cmdEnv, ExecutorBuilder builder) defaultImage, timeoutKillDelay, useCustomizedImages)); + spawnRunners.add(spawnRunner); builder.addActionContext( new DockerSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner)); } @@ -258,6 +277,7 @@ private void setup(CommandEnvironment cmdEnv, ExecutorBuilder builder) timeoutKillDelay, sandboxfsProcess, options.sandboxfsMapSymlinkTargets)); + spawnRunners.add(spawnRunner); builder.addActionContext(new LinuxSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner)); } @@ -272,6 +292,7 @@ private void setup(CommandEnvironment cmdEnv, ExecutorBuilder builder) timeoutKillDelay, sandboxfsProcess, options.sandboxfsMapSymlinkTargets)); + spawnRunners.add(spawnRunner); builder.addActionContext(new DarwinSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner)); } @@ -362,6 +383,12 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) 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 @@ public void afterCommand() { 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 72a5b45f7ec668..d2fa83dddf6403 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh @@ -225,19 +225,6 @@ function test_sandboxed_genrule_with_tools() { || 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 36e1a5799840c5..1d9dd9620f1984 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD @@ -549,6 +549,16 @@ sh_test( 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 00000000000000..0542cc56591666 --- /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 <"${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 <"${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 <"${TEST_log}" 2>&1 || fail "Expected build to succeed" + echo foo >>pkg/pkg.in + done +} + +run_suite "sandboxing"