Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve paths under hermetic /tmp in the sandbox #22001

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
e772a12
Revert "Make tree artifacts that are symlinks to absolute paths work …
fmeum Apr 15, 2024
5014297
Revert "Fix two issues with `--incompatible_sandbox_hermetic_tmp` tha…
fmeum Apr 15, 2024
86e40c6
Revert "Rewrite paths of writable directories that are under the exec…
fmeum Apr 15, 2024
19296b1
Revert "Add support for bind mounts under `/tmp` with hermetic tmp"
fmeum Apr 15, 2024
d9f1cdd
Revert "Add support for tmpfs mounts under `/tmp` with hermetic tmp"
fmeum Apr 15, 2024
73521a9
Revert "Mount user-specified bind mounts before Bazel's own magic."
fmeum Apr 15, 2024
cc0a4f1
WIP
fmeum Apr 12, 2024
e5d571a
Manually revert bind mounting magic
fmeum Apr 15, 2024
4d2a494
Revert "Make all sandboxed strategies work with Filesets on their inp…
fmeum Apr 15, 2024
8560be2
Revert "Make the Linux sandbox work with ActionInputs with absolute "…
fmeum Apr 15, 2024
896e4eb
Revert "Replace Path with RootedPath in SandboxHelpers and maintain a…
fmeum Apr 15, 2024
161ac55
Fix Java tests
fmeum Apr 15, 2024
3b84d1e
fix mount tests
fmeum Apr 16, 2024
7844af9
Add back package path test
fmeum Apr 16, 2024
17b2404
Add standalone test
fmeum May 4, 2024
72d3623
Improve test
fmeum May 7, 2024
1d6fe72
Switch to new bind mounting scheme
fmeum May 8, 2024
47bef06
Working mount logic for non-execroot
fmeum May 8, 2024
47808a2
Fix output base under tmp case
fmeum May 8, 2024
b1cf872
Add comment
fmeum May 8, 2024
0a7c217
Add import
fmeum May 8, 2024
2410782
Revert caffeine update
fmeum May 8, 2024
c2f8768
Fix more tests and add back RootedPath
fmeum May 8, 2024
583e2c3
Clean up bind mounting logic
fmeum May 8, 2024
561ba78
Reduce diff
fmeum May 8, 2024
1a477e1
Fix SandboxHelpersTest
fmeum May 8, 2024
af8c8dc
Fix one more test
fmeum May 8, 2024
78b78d1
Expand symlink test to cover external repos and symlinks
fmeum May 11, 2024
b6888d4
Modify sandboxing test to move workspace into subdirectory
fmeum May 14, 2024
612da61
WIp
fmeum May 14, 2024
9c0c738
Simplify sandbox setup and fix integration test
fmeum May 14, 2024
9ebdcc2
Re-revert the switch to `RootedPath`
fmeum May 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -360,21 +360,20 @@ 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 env the environment 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)
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
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/sandbox/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ java_library(
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
Expand Down
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
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
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
Loading