Map symlinks into sandboxfs as they are and propagate their targets. We expect rules to map all the input files they need, including the targets of those symlinks. Unfortunately, not all rules currently abide by this principle, which means that using the stricter sandboxfs sandboxing causes these rules to fail. In order to mitigate this problem and to have an escape hatch for using sandboxfs in the presence of these bugs, compute what the targets of the symlinks are and map them automatically within the sandbox unless they were explicitly mapped. But because we treat this condition as a bug in the rules (and because doing this computation is not trivial), this behavior is optional and is disabled by default. Users encountering this problem in the rules they use (or in their own rules) can temporarily enable the looser sandboxing by setting --experimental_sandboxfs_map_symlink_targets. RELNOTES: None. PiperOrigin-RevId: 239066555
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java index 22a35c3..21eed10 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
@@ -112,6 +112,7 @@ private final Path sandboxBase; private final Duration timeoutKillDelay; private final @Nullable SandboxfsProcess sandboxfsProcess; + private final boolean sandboxfsMapSymlinkTargets; /** * The set of directories that always should be writable, independent of the Spawn itself. @@ -130,12 +131,14 @@ * @param timeoutKillDelay additional grace period before killing timing out commands * @param sandboxfsProcess instance of the sandboxfs process to use; may be null for none, in * which case the runner uses a symlinked sandbox + * @param sandboxfsMapSymlinkTargets map the targets of symlinks within the sandbox if true */ DarwinSandboxedSpawnRunner( CommandEnvironment cmdEnv, Path sandboxBase, Duration timeoutKillDelay, - @Nullable SandboxfsProcess sandboxfsProcess) + @Nullable SandboxfsProcess sandboxfsProcess, + boolean sandboxfsMapSymlinkTargets) throws IOException { super(cmdEnv); this.execRoot = cmdEnv.getExecRoot(); @@ -146,6 +149,7 @@ this.sandboxBase = sandboxBase; this.timeoutKillDelay = timeoutKillDelay; this.sandboxfsProcess = sandboxfsProcess; + this.sandboxfsMapSymlinkTargets = sandboxfsMapSymlinkTargets; } private static void addPathToSetIfExists(FileSystem fs, Set<Path> paths, String path) @@ -277,7 +281,8 @@ environment, inputs, outputs, - ImmutableSet.of()) { + ImmutableSet.of(), + sandboxfsMapSymlinkTargets) { @Override public void createFileSystem() throws IOException { super.createFileSystem();
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 c0d3cec..d9309be 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
@@ -96,6 +96,7 @@ private final LocalEnvProvider localEnvProvider; private final Duration timeoutKillDelay; private final @Nullable SandboxfsProcess sandboxfsProcess; + private final boolean sandboxfsMapSymlinkTargets; /** * Creates a sandboxed spawn runner that uses the {@code linux-sandbox} tool. @@ -107,6 +108,7 @@ * @param timeoutKillDelay an additional grace period before killing timing out commands * @param sandboxfsProcess instance of the sandboxfs process to use; may be null for none, in * which case the runner uses a symlinked sandbox + * @param sandboxfsMapSymlinkTargets map the targets of symlinks within the sandbox if true */ LinuxSandboxedSpawnRunner( CommandEnvironment cmdEnv, @@ -114,7 +116,8 @@ Path inaccessibleHelperFile, Path inaccessibleHelperDir, Duration timeoutKillDelay, - @Nullable SandboxfsProcess sandboxfsProcess) { + @Nullable SandboxfsProcess sandboxfsProcess, + boolean sandboxfsMapSymlinkTargets) { super(cmdEnv); this.fileSystem = cmdEnv.getRuntime().getFileSystem(); this.blazeDirs = cmdEnv.getDirectories(); @@ -126,6 +129,7 @@ this.inaccessibleHelperDir = inaccessibleHelperDir; this.timeoutKillDelay = timeoutKillDelay; this.sandboxfsProcess = sandboxfsProcess; + this.sandboxfsMapSymlinkTargets = sandboxfsMapSymlinkTargets; this.localEnvProvider = new PosixLocalEnvProvider(cmdEnv.getClientEnv()); } @@ -196,7 +200,8 @@ execRoot, getSandboxOptions().symlinkedSandboxExpandsTreeArtifactsInRunfilesTree), outputs, - ImmutableSet.of()); + ImmutableSet.of(), + sandboxfsMapSymlinkTargets); } else { sandbox = new SymlinkedSandboxedSpawn(
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java index 154ac79..4d3d85d 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
@@ -49,10 +49,15 @@ * @param timeoutKillDelay additional grace period before killing timing out commands * @param sandboxfsProcess instance of the sandboxfs process to use; may be null for none, in * which case the runner uses a symlinked sandbox + * @param sandboxfsMapSymlinkTargets map the targets of symlinks within the sandbox if true */ static LinuxSandboxedSpawnRunner create( - CommandEnvironment cmdEnv, Path sandboxBase, Duration timeoutKillDelay, - @Nullable SandboxfsProcess sandboxfsProcess) throws IOException { + CommandEnvironment cmdEnv, + Path sandboxBase, + Duration timeoutKillDelay, + @Nullable SandboxfsProcess sandboxfsProcess, + boolean sandboxfsMapSymlinkTargets) + throws IOException { Path inaccessibleHelperFile = sandboxBase.getRelative("inaccessibleHelperFile"); FileSystemUtils.touchFile(inaccessibleHelperFile); inaccessibleHelperFile.setReadable(false); @@ -71,6 +76,7 @@ inaccessibleHelperFile, inaccessibleHelperDir, timeoutKillDelay, - sandboxfsProcess); + sandboxfsProcess, + sandboxfsMapSymlinkTargets); } }
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 7f48615..5777c44 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
@@ -216,7 +216,11 @@ withFallback( cmdEnv, LinuxSandboxedStrategy.create( - cmdEnv, sandboxBase, timeoutKillDelay, sandboxfsProcess)); + cmdEnv, + sandboxBase, + timeoutKillDelay, + sandboxfsProcess, + options.sandboxfsMapSymlinkTargets)); builder.addActionContext(new LinuxSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner)); } @@ -226,7 +230,11 @@ withFallback( cmdEnv, new DarwinSandboxedSpawnRunner( - cmdEnv, sandboxBase, timeoutKillDelay, sandboxfsProcess)); + cmdEnv, + sandboxBase, + timeoutKillDelay, + sandboxfsProcess, + options.sandboxfsMapSymlinkTargets)); builder.addActionContext(new DarwinSandboxedStrategy(cmdEnv.getExecRoot(), spawnRunner)); }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java index f696b3a..b4d5561 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
@@ -195,6 +195,17 @@ ) public String sandboxfsPath; + @Option( + name = "experimental_sandboxfs_map_symlink_targets", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "If true, maps the targets of symbolic links specified as action inputs into the " + + "sandbox. This feature exists purely to workaround buggy rules that do not do " + + "this on their own and should be removed once all such rules are fixed.") + public boolean sandboxfsMapSymlinkTargets; + public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) { List<Path> inaccessiblePaths = new ArrayList<>(); for (String path : sandboxBlockPath) {
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java index 0328338..0ed138a 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -58,6 +59,9 @@ /** Collection of directories where the spawn can write files to relative to {@link #execRoot}. */ private final Set<PathFragment> writableDirs; + /** Map the targets of symlinks within the sandbox if true. */ + private final boolean mapSymlinkTargets; + /** * Writable directory where the spawn runner keeps control files and the execroot outside of the * sandboxfs instance. @@ -90,6 +94,7 @@ * @param outputs output files to expect from the spawn * @param writableDirs directories where the spawn can write files to, relative to the sandbox's * dynamically-allocated execroot + * @param mapSymlinkTargets map the targets of symlinks within the sandbox if true */ SandboxfsSandboxedSpawn( SandboxfsProcess process, @@ -98,7 +103,8 @@ Map<String, String> environment, Map<PathFragment, Path> inputs, SandboxOutputs outputs, - Set<PathFragment> writableDirs) { + Set<PathFragment> writableDirs, + boolean mapSymlinkTargets) { this.process = process; this.arguments = arguments; this.environment = environment; @@ -114,6 +120,7 @@ checkArgument(!path.isAbsolute(), "writable directory %s must be relative", path); } this.writableDirs = writableDirs; + this.mapSymlinkTargets = mapSymlinkTargets; this.sandboxPath = sandboxPath; this.sandboxScratchDir = sandboxPath.getRelative("scratch"); @@ -142,7 +149,8 @@ public void createFileSystem() throws IOException { sandboxScratchDir.createDirectory(); - List<Mapping> mappings = createMappings(innerExecRoot, sandboxScratchDir, inputs); + List<Mapping> mappings = + createMappings(innerExecRoot, sandboxScratchDir, inputs, mapSymlinkTargets); Set<PathFragment> dirsToCreate = new HashSet<>(writableDirs); for (PathFragment output : outputs.files()) { @@ -190,6 +198,58 @@ } /** + * Maps the targets of relative symlinks into the sandbox. + * + * <p>Symlinks with relative targets are tricky business. Consider this simple case: the source + * tree contains {@code dir/file.h} and {@code dir/symlink.h} where {@code dir/symlink.h}'s target + * is {@code ./file.h}. If {@code dir/symlink.h} is supplied as an input, we must preserve its + * target "as is" to avoid confusing any tooling: for example, the C compiler will understand that + * both {@code dir/file.h} and {@code dir/symlink.h} are the same entity and handle them + * appropriately. (We did encounter a case where the compiler complained about duplicate symbols + * because we exposed symlinks as regular files.) + * + * <p>However, there is no guarantee that the target of the symlink is mapped in the sandbox. You + * may think that this is a bug in the rules, and you would probably be right, but until those + * rules are fixed, we must supply a workaround. Therefore, we must handle these two cases: if the + * target is explicitly mapped, we do nothing. If it isn't, we have to compute where the target + * lives within the sandbox and map that as well. Oh, and we have to do this recursively. + * + * @param path path to expose within the sandbox + * @param symlink path to the target of the mapping specified by {@code path} + * @param mappings mutable collection of mappings to extend with the new symlink entries. Note + * that the entries added to this map may correspond to explicitly-mapped entries, so the + * caller must check this to avoid duplicate mappings + * @throws IOException if we fail to resolve symbolic links + */ + private static void computeSymlinkMappings( + PathFragment path, Path symlink, Map<PathFragment, Path> mappings) throws IOException { + for (; ; ) { + PathFragment symlinkTarget = symlink.readSymbolicLinkUnchecked(); + if (!symlinkTarget.isAbsolute()) { + PathFragment keyParent = path.getParentDirectory(); + if (keyParent == null) { + throw new IOException("Cannot resolve " + symlinkTarget + " relative to " + path); + } + PathFragment key = keyParent.getRelative(symlinkTarget); + + Path valueParent = symlink.getParentDirectory(); + if (valueParent == null) { + throw new IOException("Cannot resolve " + symlinkTarget + " relative to " + symlink); + } + Path value = valueParent.getRelative(symlinkTarget); + mappings.put(key, value); + + if (value.isSymbolicLink()) { + path = key; + symlink = value; + continue; + } + } + break; + } + } + + /** * Creates a new set of mappings to sandbox the given inputs. * * @param root unique working directory for the command, specified as an absolute path anchored at @@ -198,11 +258,16 @@ * @param inputs collection of paths to expose within the sandbox as read-only mappings, given as * a map of mapped path to target path. The target path may be null, in which case an empty * read-only file is mapped. + * @param sandboxfsMapSymlinkTargets map the targets of symlinks within the sandbox if true * @return the collection of mappings to use for reconfiguration * @throws IOException if we fail to resolve symbolic links */ private static List<Mapping> createMappings( - PathFragment root, Path scratchDir, Map<PathFragment, Path> inputs) throws IOException { + PathFragment root, + Path scratchDir, + Map<PathFragment, Path> inputs, + boolean sandboxfsMapSymlinkTargets) + throws IOException { List<Mapping> mappings = new ArrayList<>(); mappings.add( @@ -220,6 +285,10 @@ // FUSE file system (which sandboxfs is) requires root privileges. Path emptyFile = null; + // Collection of extra mappings needed to represent the targets of relative symlinks. Lazily + // created once we encounter the first symlink in the list of inputs. + Map<PathFragment, Path> symlinks = null; + for (Map.Entry<PathFragment, Path> entry : inputs.entrySet()) { PathFragment target; if (entry.getValue() == null) { @@ -229,6 +298,12 @@ } target = emptyFile.asFragment(); } else { + if (entry.getValue().isSymbolicLink() && sandboxfsMapSymlinkTargets) { + if (symlinks == null) { + symlinks = new HashMap<>(); + } + computeSymlinkMappings(entry.getKey(), entry.getValue(), symlinks); + } target = entry.getValue().asFragment(); } mappings.add( @@ -239,6 +314,19 @@ .build()); } + if (symlinks != null) { + for (Map.Entry<PathFragment, Path> entry : symlinks.entrySet()) { + if (!inputs.containsKey(entry.getKey())) { + mappings.add( + Mapping.builder() + .setPath(root.getRelative(entry.getKey())) + .setTarget(entry.getValue().asFragment()) + .setWritable(false) + .build()); + } + } + } + return mappings; } }
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawnTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawnTest.java index c94ad2a..61b4633 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawnTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawnTest.java
@@ -14,6 +14,7 @@ package com.google.devtools.build.lib.sandbox; import static com.google.common.truth.Truth.assertThat; +import static junit.framework.TestCase.fail; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -71,7 +72,8 @@ ImmutableMap.of(PathFragment.create("such/input.txt"), helloTxt), SandboxOutputs.create( ImmutableSet.of(PathFragment.create("very/output.txt")), ImmutableSet.of()), - ImmutableSet.of(PathFragment.create("wow/writable"))); + ImmutableSet.of(PathFragment.create("wow/writable")), + /* mapSymlinkTargets= */ false); spawn.createFileSystem(); Path execRoot = spawn.getSandboxExecRoot(); @@ -96,7 +98,8 @@ ImmutableMap.of(PathFragment.create("such/input.txt"), helloTxt), SandboxOutputs.create( ImmutableSet.of(PathFragment.create("very/output.txt")), ImmutableSet.of()), - ImmutableSet.of(PathFragment.create("wow/writable"))); + ImmutableSet.of(PathFragment.create("wow/writable")), + /* mapSymlinkTargets= */ false); spawn.createFileSystem(); Path execRoot = spawn.getSandboxExecRoot(); @@ -124,7 +127,8 @@ ImmutableMap.of(), ImmutableMap.of(), SandboxOutputs.create(ImmutableSet.of(outputFile), ImmutableSet.of()), - ImmutableSet.of()); + ImmutableSet.of(), + /* mapSymlinkTargets= */ false); spawn.createFileSystem(); Path execRoot = spawn.getSandboxExecRoot(); @@ -137,20 +141,36 @@ assertThat(outputsDir.getRelative(outputFile).isFile(Symlinks.NOFOLLOW)).isTrue(); } - @Test - public void testSymlinksAreKeptAsIs() throws Exception { - Path helloTxt = workspaceDir.getRelative("dir1/hello.txt"); - helloTxt.getParentDirectory().createDirectory(); - FileSystemUtils.createEmptyFile(helloTxt); + public void testSymlinks(boolean mapSymlinkTargets) throws Exception { + Path input1 = workspaceDir.getRelative("dir1/input-1.txt"); + input1.getParentDirectory().createDirectory(); + FileSystemUtils.createEmptyFile(input1); - Path linkToHello = workspaceDir.getRelative("dir2/link-to-hello"); - linkToHello.getParentDirectory().createDirectory(); - PathFragment linkTarget = PathFragment.create("../dir1/hello.txt"); - linkToHello.createSymbolicLink(linkTarget); + Path input2 = workspaceDir.getRelative("dir1/input-2.txt"); + input2.getParentDirectory().createDirectory(); + FileSystemUtils.createEmptyFile(input2); - // Ensure that the symlink we have created has a relative target, as otherwise we wouldn't - // exercise the functionality we are trying to test. - assertThat(linkToHello.readSymbolicLink().isAbsolute()).isFalse(); + Path linkToInput1 = workspaceDir.getRelative("dir2/link-to-input-1"); + linkToInput1.getParentDirectory().createDirectory(); + linkToInput1.createSymbolicLink(PathFragment.create("../dir1/input-1.txt")); + assertThat(linkToInput1.readSymbolicLink().isAbsolute()).isFalse(); + + Path linkToInput2 = workspaceDir.getRelative("dir2/link-to-input-2"); + linkToInput2.getParentDirectory().createDirectory(); + linkToInput2.createSymbolicLink(PathFragment.create("../dir1/input-2.txt")); + assertThat(linkToInput2.readSymbolicLink().isAbsolute()).isFalse(); + + Path linkToLink = workspaceDir.getRelative("dir2/link-to-link"); + linkToLink.getParentDirectory().createDirectory(); + linkToLink.createSymbolicLink(PathFragment.create("link-to-input-2")); + assertThat(linkToLink.readSymbolicLink().isAbsolute()).isFalse(); + + Path linkToAbsolutePath = workspaceDir.getRelative("dir2/link-to-absolute-path"); + linkToAbsolutePath.getParentDirectory().createDirectory(); + Path randomPath = workspaceDir.getRelative("/some-random-path"); + FileSystemUtils.createEmptyFile(randomPath); + linkToAbsolutePath.createSymbolicLink(randomPath.asFragment()); + assertThat(linkToAbsolutePath.readSymbolicLink().isAbsolute()).isTrue(); SandboxedSpawn spawn = new SandboxfsSandboxedSpawn( @@ -158,15 +178,63 @@ outerDir, ImmutableList.of("/bin/true"), ImmutableMap.of(), - ImmutableMap.of(PathFragment.create("such/input.txt"), linkToHello), + ImmutableMap.of( + PathFragment.create("dir1/input-1.txt"), input1, + // input2 and linkToInput2 intentionally left unmapped to verify they are mapped as + // symlink targets of linktoLink. + PathFragment.create("such/link-1.txt"), linkToInput1, + PathFragment.create("such/link-to-link.txt"), linkToLink, + PathFragment.create("such/abs-link.txt"), linkToAbsolutePath), SandboxOutputs.create( ImmutableSet.of(PathFragment.create("very/output.txt")), ImmutableSet.of()), - ImmutableSet.of()); + ImmutableSet.of(), + mapSymlinkTargets); spawn.createFileSystem(); Path execRoot = spawn.getSandboxExecRoot(); - assertThat(execRoot.getRelative("such/input.txt").isSymbolicLink()).isTrue(); - assertThat(execRoot.getRelative("such/input.txt").readSymbolicLink()).isEqualTo(linkTarget); + // Relative symlinks must be kept as such in the sandbox and they must resolve properly. + assertThat(execRoot.getRelative("such/link-1.txt").readSymbolicLink()) + .isEqualTo(PathFragment.create("../dir1/input-1.txt")); + assertThat(execRoot.getRelative("such/link-1.txt").resolveSymbolicLinks()).isEqualTo(input1); + assertThat(execRoot.getRelative("such/link-to-link.txt").readSymbolicLink()) + .isEqualTo(PathFragment.create("link-to-input-2")); + if (mapSymlinkTargets) { + assertThat(execRoot.getRelative("such/link-to-link.txt").resolveSymbolicLinks()) + .isEqualTo(input2); + assertThat(execRoot.getRelative("such/link-to-input-2").readSymbolicLink()) + .isEqualTo(PathFragment.create("../dir1/input-2.txt")); + assertThat(execRoot.getRelative("such/link-to-input-2").resolveSymbolicLinks()) + .isEqualTo(input2); + } else { + try { + execRoot.getRelative("such/link-to-link.txt").resolveSymbolicLinks(); + fail("Symlink resolution worked, which means the target was mapped when not expected"); + } catch (IOException expected) { + } + } + + // Targets of symlinks must have been mapped inside the sandbox only when requested. + assertThat(execRoot.getRelative("dir1/input-1.txt").exists()).isTrue(); + if (mapSymlinkTargets) { + assertThat(execRoot.getRelative("dir1/input-2.txt").exists()).isTrue(); + } else { + assertThat(execRoot.getRelative("dir1/input-2.txt").exists()).isFalse(); + } + + // Absolute symlinks must be kept as such in the sandbox no matter where they point to. + assertThat(execRoot.getRelative("such/abs-link.txt").isSymbolicLink()).isTrue(); + assertThat(execRoot.getRelative("such/abs-link.txt").readSymbolicLinkUnchecked()) + .isEqualTo(randomPath.asFragment()); + } + + @Test + public void testSymlinks_TargetsMappedIfRequested() throws Exception { + testSymlinks(true); + } + + @Test + public void testSymlinks_TargetsNotMappedIfNotRequested() throws Exception { + testSymlinks(false); } }