Skip to content

Commit

Permalink
Only wipe the whole sandbox base during the first build in a server.
Browse files Browse the repository at this point in the history
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 #7527.

RELNOTES: None.
PiperOrigin-RevId: 243635557
  • Loading branch information
jmmv authored and copybara-github committed Apr 15, 2019
1 parent 460843f commit b93e61b
Show file tree
Hide file tree
Showing 8 changed files with 210 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
* <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 {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -288,4 +288,14 @@ protected ImmutableSet<Path> 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();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -321,4 +321,18 @@ private void validateBindMounts(SortedMap<Path, Path> 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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,18 @@
import java.io.File;
import java.io.IOException;
import java.time.Duration;
import java.util.HashSet;
import java.util.Set;
import javax.annotation.Nullable;

/**
* This module provides the Sandbox spawn strategy.
*/
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;

Expand All @@ -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.
*
* <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.
Expand Down Expand Up @@ -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()
Expand All @@ -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;
Expand Down Expand Up @@ -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));
}
Expand All @@ -238,6 +256,7 @@ private void setup(CommandEnvironment cmdEnv, ExecutorBuilder builder)
defaultImage,
timeoutKillDelay,
useCustomizedImages));
spawnRunners.add(spawnRunner);
builder.addActionContext(
new DockerSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner));
}
Expand All @@ -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));
}

Expand All @@ -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));
}

Expand Down Expand Up @@ -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);
}
}

/**
Expand Down Expand Up @@ -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;

Expand Down
13 changes: 0 additions & 13 deletions src/test/shell/bazel/bazel_sandboxing_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions src/test/shell/integration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
115 changes: 115 additions & 0 deletions src/test/shell/integration/sandboxing_test.sh
Original file line number Diff line number Diff line change
@@ -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"

0 comments on commit b93e61b

Please sign in to comment.