Skip to content

Commit

Permalink
Preserve paths under hermetic /tmp in the sandbox
Browse files Browse the repository at this point in the history 
The bind mounting scheme used with the Linux sandbox' hermetic `/tmp` feature is modified to preserve all paths as they are outside the sandbox, which removes the need to rewrite paths when staging inputs into and, crucially, moving outputs out of the sandbox.

Source roots and output base paths under `/tmp` are now treated just like any user-specified bind mount under `/tmp`: They are mounted under the hermetic tmp directory with their path relativized against `/tmp` before the hermetic tmp directory is mounted as `/tmp` as the final step.

There is one caveat compared to user-specified mounts: Source roots, which may themselves not lie under `/tmp`, can be symlinks to directories under `/tmp` (e.g., when they arise from a `local_repository`). To handle this situation in the common case, all parent directories of package path entries (up to direct children of `/tmp`) are mounted into the sandbox. If users use `local_repository`s with fixed target paths under `/tmp`, they will need to specify `--sandbox_add_mount_pair`.

Overlayfs has been considered as an alternative to this approach, but ultimately doesn't seem to work for this use case since its `lowerpath`, which would be `/tmp`, is not allowed to have child mounts from a different user namespace (see https://unix.stackexchange.com/questions/776030/mounting-overlayfs-in-a-user-namespace-with-child-mounts). However, this is exactly the situation created by a Bazel-in-Bazel test and can also arise if the user has existing mounts under `/tmp` when using Bazel (e.g. the JetBrains toolbox on Linux uses such mounts).

This replaces and mostly reverts the following commits, but keeps their tests:
* bf6ebe9
* fb6658c
* bc1d9d3
* 1829883
* 70691f2
* a556969
* 8e32f44 (had its test lost in an incorrect merge conflict resolution, this PR adds it back)

Fixes #20533
Work towards #20753
Fixes #21215
Fixes #22117
Fixes #22226
Fixes #22290

RELNOTES: Paths in the Linux sandbox are now again identical to those outside the sandbox, even with `--incompatible_sandbox_hermetic_tmp`.

Closes #22001.

PiperOrigin-RevId: 634381503
Change-Id: I9f7f3948c705be120c55c9b0c51204e5bea45f61
  • Loading branch information
@fmeum
fmeum authored and Copybara-Service committed May 16, 2024
1 parent dd2464a commit 3fddc7f
Show file tree
Hide file tree
Showing 25 changed files with 548 additions and 793 deletions.
47 changes: 7 additions & 40 deletions src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,19 +170,11 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) {
/**
* Optional materialization path.
*
* <p>If present, this artifact is a copy of another artifact whose contents live at this path.
* This can happen when it is declared as a file and not as an unresolved symlink but the action
* that creates it materializes it in the filesystem as a symlink to another output artifact. This
* information is useful in two situations:
*
* <ol>
* <li>When the symlink target is a remotely stored artifact, we can avoid downloading it
* multiple times when building without the bytes (see AbstractActionInputPrefetcher).
* <li>When the symlink target is inaccessible from the sandboxed environment an action runs
* under, we can rewrite it accordingly (see SandboxHelpers).
* </ol>
*
* @see com.google.devtools.build.lib.skyframe.TreeArtifactValue#getMaterializationExecPath().
* <p>If present, this artifact is a copy of another artifact. It is still tracked as a
* non-symlink by Bazel, but materialized in the local filesystem as a symlink to the original
* artifact, whose contents live at this location. This is used by {@link
* com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher} to implement zero-cost
* copies of remotely stored artifacts.
*/
public Optional<PathFragment> getMaterializationExecPath() {
return Optional.empty();
Expand Down Expand Up @@ -223,12 +215,6 @@ public static FileArtifactValue createForSourceArtifact(
xattrProvider);
}

public static FileArtifactValue createForResolvedSymlink(
PathFragment realPath, FileArtifactValue metadata, @Nullable byte[] digest) {
return new ResolvedSymlinkFileArtifactValue(
realPath, digest, metadata.getContentsProxy(), metadata.getSize());
}

public static FileArtifactValue createFromInjectedDigest(
FileArtifactValue metadata, @Nullable byte[] digest) {
return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize());
Expand Down Expand Up @@ -457,25 +443,7 @@ public String toString() {
}
}

private static final class ResolvedSymlinkFileArtifactValue extends RegularFileArtifactValue {
private final PathFragment realPath;

private ResolvedSymlinkFileArtifactValue(
PathFragment realPath,
@Nullable byte[] digest,
@Nullable FileContentsProxy proxy,
long size) {
super(digest, proxy, size);
this.realPath = realPath;
}

@Override
public Optional<PathFragment> getMaterializationExecPath() {
return Optional.of(realPath);
}
}

private static class RegularFileArtifactValue extends FileArtifactValue {
private static final class RegularFileArtifactValue extends FileArtifactValue {
private final byte[] digest;
@Nullable private final FileContentsProxy proxy;
private final long size;
Expand All @@ -497,8 +465,7 @@ public boolean equals(Object o) {
}
return Arrays.equals(digest, that.digest)
&& Objects.equals(proxy, that.proxy)
&& size == that.size
&& Objects.equals(getMaterializationExecPath(), that.getMaterializationExecPath());
&& size == that.size;
}

@Override
Expand Down
5 changes: 2 additions & 3 deletions ...main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import java.io.IOException;
import java.util.LinkedHashSet;
import java.util.Set;
Expand Down Expand Up @@ -163,9 +162,9 @@ void createInputs(Iterable<PathFragment> inputsToCreate, SandboxInputs inputs)
}
Path key = sandboxExecRoot.getRelative(fragment);
if (inputs.getFiles().containsKey(fragment)) {
RootedPath fileDest = inputs.getFiles().get(fragment);
Path fileDest = inputs.getFiles().get(fragment);
if (fileDest != null) {
copyFile(fileDest.asPath(), key);
copyFile(fileDest, key);
} else {
FileSystemUtils.createEmptyFile(key);
}
Expand Down
8 changes: 3 additions & 5 deletions src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,21 +360,19 @@ private boolean wasTimeout(Duration timeout, Duration wallTime) {
/**
* Gets the list of directories that the spawn will assume to be writable.
*
* @param sandboxExecRoot the exec root of the sandbox from the point of view of the Bazel process
* @param withinSandboxExecRoot the exec root from the point of view of the sandboxed processes
* @param sandboxExecRoot the exec root of the sandbox
* @param env the environment of the sandboxed processes
* @throws IOException because we might resolve symlinks, which throws {@link IOException}.
*/
protected ImmutableSet<Path> getWritableDirs(
Path sandboxExecRoot, Path withinSandboxExecRoot, Map<String, String> env)
protected ImmutableSet<Path> getWritableDirs(Path sandboxExecRoot, Map<String, String> env)
throws IOException {
// We have to make the TEST_TMPDIR directory writable if it is specified.
ImmutableSet.Builder<Path> writablePaths = ImmutableSet.builder();

// On Windows, sandboxExecRoot is actually the main execroot. We will specify
// exactly which output path is writable.
if (OS.getCurrent() != OS.WINDOWS) {
writablePaths.add(withinSandboxExecRoot);
writablePaths.add(execRoot);
}

String testTmpdir = env.get("TEST_TMPDIR");
Expand Down
12 changes: 2 additions & 10 deletions src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import java.io.BufferedWriter;
import java.io.File;
import java.io.IOException;
Expand Down Expand Up @@ -103,7 +102,6 @@ private static boolean computeIsSupported() throws InterruptedException {

private final SandboxHelpers helpers;
private final Path execRoot;
private final ImmutableList<Root> packageRoots;
private final boolean allowNetwork;
private final ProcessWrapper processWrapper;
private final Path sandboxBase;
Expand Down Expand Up @@ -134,7 +132,6 @@ private static boolean computeIsSupported() throws InterruptedException {
super(cmdEnv);
this.helpers = helpers;
this.execRoot = cmdEnv.getExecRoot();
this.packageRoots = cmdEnv.getPackageLocator().getPathEntries();
this.allowNetwork = helpers.shouldAllowNetwork(cmdEnv.getOptions());
this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem());
this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv);
Expand Down Expand Up @@ -221,18 +218,13 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), binTools, "/tmp");

final HashSet<Path> writableDirs = new HashSet<>(alwaysWritableDirs);
ImmutableSet<Path> extraWritableDirs =
getWritableDirs(sandboxExecRoot, sandboxExecRoot, environment);
ImmutableSet<Path> extraWritableDirs = getWritableDirs(sandboxExecRoot, environment);
writableDirs.addAll(extraWritableDirs);

SandboxInputs inputs =
helpers.processInputFiles(
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
context.getInputMetadataProvider(),
execRoot,
execRoot,
packageRoots,
null);
execRoot);
SandboxOutputs outputs = helpers.getOutputs(spawn);

final Path sandboxConfigPath = sandboxPath.getRelative("sandbox.sb");
Expand Down
9 changes: 1 addition & 8 deletions src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import com.google.devtools.build.lib.util.ProcessUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand Down Expand Up @@ -143,7 +142,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)

private final SandboxHelpers helpers;
private final Path execRoot;
private final ImmutableList<Root> packageRoots;
private final boolean allowNetwork;
private final Path dockerClient;
private final ProcessWrapper processWrapper;
Expand Down Expand Up @@ -181,7 +179,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)
super(cmdEnv);
this.helpers = helpers;
this.execRoot = cmdEnv.getExecRoot();
this.packageRoots = cmdEnv.getPackageLocator().getPathEntries();
this.allowNetwork = helpers.shouldAllowNetwork(cmdEnv.getOptions());
this.dockerClient = dockerClient;
this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv);
Expand Down Expand Up @@ -226,11 +223,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
SandboxInputs inputs =
helpers.processInputFiles(
context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
context.getInputMetadataProvider(),
execRoot,
execRoot,
packageRoots,
null);
execRoot);
SandboxOutputs outputs = helpers.getOutputs(spawn);

Duration timeout = context.getTimeout();
Expand Down
29 changes: 8 additions & 21 deletions src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxCommandLineBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NETNS;
import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NETNS_WITH_LOOPBACK;

import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.vfs.Path;
Expand All @@ -36,20 +36,6 @@
* linux-sandbox} tool.
*/
public class LinuxSandboxCommandLineBuilder {
/** A bind mount that needs to be present when the sandboxed command runs. */
@AutoValue
public abstract static class BindMount {
public static BindMount of(Path mountPoint, Path source) {
return new AutoValue_LinuxSandboxCommandLineBuilder_BindMount(mountPoint, source);
}

/** "target" in mount(2) */
public abstract Path getMountPoint();

/** "source" in mount(2) */
public abstract Path getContent();
}

private final Path linuxSandboxPath;
private Path hermeticSandboxPath;
private Path workingDirectory;
Expand All @@ -60,7 +46,7 @@ public static BindMount of(Path mountPoint, Path source) {
private Path stderrPath;
private Set<Path> writableFilesAndDirectories = ImmutableSet.of();
private ImmutableSet<PathFragment> tmpfsDirectories = ImmutableSet.of();
private List<BindMount> bindMounts = ImmutableList.of();
private Map<Path, Path> bindMounts = ImmutableMap.of();
private Path statisticsPath;
private boolean useFakeHostname = false;
private NetworkNamespace createNetworkNamespace = NetworkNamespace.NO_NETNS;
Expand Down Expand Up @@ -164,7 +150,7 @@ public LinuxSandboxCommandLineBuilder setTmpfsDirectories(
* if any.
*/
@CanIgnoreReturnValue
public LinuxSandboxCommandLineBuilder setBindMounts(List<BindMount> bindMounts) {
public LinuxSandboxCommandLineBuilder setBindMounts(Map<Path, Path> bindMounts) {
this.bindMounts = bindMounts;
return this;
}
Expand Down Expand Up @@ -273,11 +259,12 @@ public ImmutableList<String> buildForCommand(List<String> commandArguments) {
for (PathFragment tmpfsPath : tmpfsDirectories) {
commandLineBuilder.add("-e", tmpfsPath.getPathString());
}
for (BindMount bindMount : bindMounts) {
commandLineBuilder.add("-M", bindMount.getContent().getPathString());
for (Path bindMountTarget : bindMounts.keySet()) {
Path bindMountSource = bindMounts.get(bindMountTarget);
commandLineBuilder.add("-M", bindMountSource.getPathString());
// The file is mounted in a custom location inside the sandbox.
if (!bindMount.getContent().equals(bindMount.getMountPoint())) {
commandLineBuilder.add("-m", bindMount.getMountPoint().getPathString());
if (!bindMountSource.equals(bindMountTarget)) {
commandLineBuilder.add("-m", bindMountTarget.getPathString());
}
}
if (statisticsPath != null) {
Expand Down
Loading

0 comments on commit 3fddc7f

Please sign in to comment.