From 525fa71b0d6f096e9bfb180f688a4418c4974eb4 Mon Sep 17 00:00:00 2001 From: Brian Silverman Date: Wed, 6 Apr 2016 08:57:17 +0000 Subject: Mount whole directories into the sandbox when possible This halves the overhead with sandboxing enabled vs disabled for a test that basically only mounts a bunch of files out of a directory, and slows that same test with a single extra file added to the directory (but not mounted) by only ~4%. The test is with 30000 inputs moved to a subdirectory and only 10 genrules. This change means symlinks will be mounted directly as their target rather than as a symlink, but this solves some weird behavior with multi-level symlinks and will only break things which don't declare all of their dependencies. -- Change-Id: I1aa39dccb2e5fca2893bdab9065ee043d34019b2 Reviewed-on: https://bazel-review.googlesource.com/#/c/3220/ MOS_MIGRATED_REVID=119138157 --- .../build/lib/sandbox/LinuxSandboxedStrategy.java | 93 +++++++++++++++++++-- src/main/tools/namespace-sandbox.c | 7 +- .../lib/sandbox/LinuxSandboxedStrategyTest.java | 96 ++++++++++++++++++++-- 3 files changed, 179 insertions(+), 17 deletions(-) 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 ce775591b5..99d7183182 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 @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.standalone.StandaloneSpawnStrategy; import com.google.devtools.build.lib.unix.NativePosixFiles; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -54,9 +55,12 @@ import com.google.devtools.build.lib.vfs.Symlinks; import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.AbstractMap.SimpleImmutableEntry; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutorService; import java.util.concurrent.atomic.AtomicInteger; @@ -252,13 +256,70 @@ public class LinuxSandboxedStrategy implements SpawnActionContext { MountMap finalizedMounts, Path target, Path source, FileStatus stat) throws IOException { // The source must exist. Preconditions.checkArgument(stat != null, "%s does not exist", source.toString()); - finalizedMounts.put(target, source); if (stat.isSymbolicLink()) { Path symlinkTarget = source.resolveSymbolicLinks(); Preconditions.checkArgument( symlinkTarget.exists(), "%s does not exist", symlinkTarget.toString()); - finalizedMounts.put(symlinkTarget, symlinkTarget); + finalizedMounts.put(target, symlinkTarget); + } else { + finalizedMounts.put(target, source); + } + } + + /** + * Checks if a folder is fully mounted (meaning it can be mounted as a whole instead + * of mounting its contents individually). + * + * The results of previous lookups are cached to make this process faster, and this + * function updates those caches as appropriate. + * + * @param directory the directory to check for being fully mounted or not + * @param mounts all of the files which are being mounted + * @param fullyMountedDirectories all the directories which have been found to be + * fully mounted previously + * @param notFullyMountedDirectories all the directories which been found to not be + * fully mounted previously + */ + private static boolean isFullyMounted( + Entry directory, + MountMap mounts, + Set> fullyMountedDirectories, + Set> notFullyMountedDirectories) + throws IOException { + if (fullyMountedDirectories.contains(directory)) { + return true; + } else if (notFullyMountedDirectories.contains(directory)) { + return false; + } else { + boolean result = true; + for (Dirent entry : directory.getValue().readdir(Symlinks.NOFOLLOW)) { + Path entryKey = directory.getKey().getChild(entry.getName()); + Path entryValue = directory.getValue().getChild(entry.getName()); + if (entry.getType() == Dirent.Type.DIRECTORY) { + if (!isFullyMounted( + new SimpleImmutableEntry(entryKey, entryValue), + mounts, + fullyMountedDirectories, + notFullyMountedDirectories)) { + result = false; + break; + } + } else { + Path mountsValue = mounts.get(entryKey); + if (mountsValue == null || !mountsValue.equals(entryValue)) { + result = false; + break; + } + } + } + + if (result) { + fullyMountedDirectories.add(directory); + } else { + notFullyMountedDirectories.add(directory); + } + return result; } } @@ -278,6 +339,7 @@ public class LinuxSandboxedStrategy implements SpawnActionContext { */ @VisibleForTesting static MountMap finalizeMounts(Map mounts) throws IOException { + Set> fullyMountedDirectories = new HashSet<>(); MountMap finalizedMounts = new MountMap(); for (Entry mount : mounts.entrySet()) { Path target = mount.getKey(); @@ -286,16 +348,37 @@ public class LinuxSandboxedStrategy implements SpawnActionContext { FileStatus stat = source.statNullable(Symlinks.NOFOLLOW); if (stat != null && stat.isDirectory()) { + fullyMountedDirectories.add(new SimpleImmutableEntry(target, source)); for (Path subSource : FileSystemUtils.traverseTree(source, Predicates.alwaysTrue())) { Path subTarget = target.getRelative(subSource.relativeTo(source)); - finalizeMountPath( - finalizedMounts, subTarget, subSource, subSource.statNullable(Symlinks.NOFOLLOW)); + FileStatus subStat = subSource.statNullable(Symlinks.NOFOLLOW); + if (subStat.isDirectory()) { + fullyMountedDirectories.add(new SimpleImmutableEntry(subTarget, subSource)); + } + finalizeMountPath(finalizedMounts, subTarget, subSource, subStat); } } else { finalizeMountPath(finalizedMounts, target, source, stat); } } - return finalizedMounts; + + MountMap deduplicatedMounts = new MountMap(); + Set> notFullyMountedDirectories = new HashSet<>(); + for (Entry mount : finalizedMounts.entrySet()) { + Entry parent = + new SimpleImmutableEntry<>( + mount.getKey().getParentDirectory(), mount.getValue().getParentDirectory()); + if (!parent.getKey().equals(parent.getValue()) + || !isFullyMounted( + parent, finalizedMounts, fullyMountedDirectories, notFullyMountedDirectories)) { + deduplicatedMounts.put(mount.getKey(), mount.getValue()); + } + } + + for (Entry mount : fullyMountedDirectories) { + deduplicatedMounts.put(mount.getKey(), mount.getValue()); + } + return deduplicatedMounts; } /** diff --git a/src/main/tools/namespace-sandbox.c b/src/main/tools/namespace-sandbox.c index 04f1c9c734..a454a70ebe 100644 --- a/src/main/tools/namespace-sandbox.c +++ b/src/main/tools/namespace-sandbox.c @@ -134,9 +134,10 @@ static void Usage(int argc, char *const *argv, const char *fmt, ...) { " -t in case timeout occurs, how long to wait before killing " "the child with SIGKILL\n" " -d create an empty directory in the sandbox\n" - " -M/-m system directory to mount inside the sandbox\n" - " Multiple directories can be specified and each of them will be " - "mounted readonly.\n" + " -M/-m system file or directory to mount inside the " + "sandbox\n" + " Multiple files or directories can be specified and each of them " + "will be mounted readonly.\n" " The -M option specifies which directory to mount, the -m option " "specifies where to\n" " mount it in the sandbox.\n" diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java index b2fe0e6720..d5d2439b77 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java @@ -29,7 +29,6 @@ import org.junit.runners.JUnit4; import java.io.IOException; import java.nio.charset.Charset; -import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -105,6 +104,9 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { private ImmutableMap userFriendlyAsserts(List asserts) { return userFriendlyMap(asserts(asserts)); } + private ImmutableMap userFriendlyAsserts(Map asserts) { + return userFriendlyMap(asserts(asserts)); + } private ImmutableMap asserts(List asserts) { ImmutableMap.Builder pathifiedAsserts = ImmutableMap.builder(); @@ -115,6 +117,15 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { return pathifiedAsserts.build(); } + private ImmutableMap asserts(Map asserts) { + ImmutableMap.Builder pathifiedAsserts = ImmutableMap.builder(); + for (Map.Entry file : asserts.entrySet()) { + pathifiedAsserts.put( + workspaceDir.getRelative(file.getKey()), workspaceDir.getRelative(file.getValue())); + } + return pathifiedAsserts.build(); + } + private void createTreeStructure(Map linksAndFiles) throws Exception { for (Entry entry : linksAndFiles.entrySet()) { Path filePath = workspaceDir.getRelative(entry.getKey()); @@ -137,10 +148,9 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { Map testFiles = new LinkedHashMap<>(); testFiles.put("symlink.txt", "goal.txt"); testFiles.put("goal.txt", ""); + testFiles.put("other.txt", ""); - List assertMounts = new ArrayList<>(); - assertMounts.add("symlink.txt"); - assertMounts.add("goal.txt"); + Map assertMounts = ImmutableMap.of("symlink.txt", "goal.txt"); assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts)); } @@ -150,9 +160,10 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { Map testFiles = ImmutableMap.of( "symlink.txt", "x/goal.txt", - "x/goal.txt", ""); + "x/goal.txt", "", + "x/other.txt", ""); - List assertMounts = ImmutableList.of("symlink.txt", "x/goal.txt"); + Map assertMounts = ImmutableMap.of("symlink.txt", "x/goal.txt"); assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts)); } @@ -161,9 +172,10 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { Map testFiles = ImmutableMap.of( "x/symlink.txt", "../goal.txt", - "goal.txt", ""); + "goal.txt", "", + "x/other.txt", ""); - List assertMounts = ImmutableList.of("x/symlink.txt", "goal.txt"); + Map assertMounts = ImmutableMap.of("x/symlink.txt", "goal.txt"); assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts)); } @@ -178,7 +190,73 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { "a/b/y.txt", "z.txt", "a/b/z.txt", ""); - List assertMounts = ImmutableList.of("a/b/x.txt", "a/b/y.txt", "a/b/z.txt"); + List assertMounts = ImmutableList.of("a/b"); + + assertThat(userFriendlyMounts(testFiles, inputFile)) + .isEqualTo(userFriendlyAsserts(assertMounts)); + } + + @Test + public void testDetectsWholeDir() throws Exception { + ImmutableList inputFile = ImmutableList.of("a/x.txt", "a/z.txt"); + + Map testFiles = + ImmutableMap.of( + "a/x.txt", "", + "a/z.txt", ""); + + List assertMounts = ImmutableList.of("a"); + + assertThat(userFriendlyMounts(testFiles, inputFile)) + .isEqualTo(userFriendlyAsserts(assertMounts)); + } + + @Test + public void testExcludesOtherDir() throws Exception { + ImmutableList inputFile = ImmutableList.of("a/x.txt", "a/y.txt"); + + Map testFiles = + ImmutableMap.of( + "a/x.txt", "", + "a/y.txt", "", + "a/b/", ""); + + List assertMounts = ImmutableList.of("a/x.txt", "a/y.txt"); + + assertThat(userFriendlyMounts(testFiles, inputFile)) + .isEqualTo(userFriendlyAsserts(assertMounts)); + } + + @Test + public void testExcludesOtherFiles() throws Exception { + ImmutableList inputFile = ImmutableList.of("a/x.txt", "a/z.txt"); + + Map testFiles = + ImmutableMap.of( + "a/x.txt", "", + "a/y.txt", "z.txt", + "a/z.txt", ""); + + List assertMounts = ImmutableList.of("a/x.txt", "a/z.txt"); + + assertThat(userFriendlyMounts(testFiles, inputFile)) + .isEqualTo(userFriendlyAsserts(assertMounts)); + } + + @Test + public void testRecognizesOtherSymlinks() throws Exception { + ImmutableList inputFile = ImmutableList.of("a/a/x.txt", "a/a/y.txt"); + + Map testFiles = + ImmutableMap.of( + "a/a/x.txt", "../b/x.txt", + "a/a/y.txt", "", + "a/b/x.txt", ""); + + Map assertMounts = + ImmutableMap.of( + "a/a/x.txt", "a/b/x.txt", + "a/a/y.txt", "a/a/y.txt"); assertThat(userFriendlyMounts(testFiles, inputFile)) .isEqualTo(userFriendlyAsserts(assertMounts)); -- cgit v1.2.3