From 386f242788a3d0189e6882466105c57ec1149d20 Mon Sep 17 00:00:00 2001 From: Yue Gan Date: Thu, 14 Apr 2016 08:27:41 +0000 Subject: Automated [] rollback of commit 525fa71b0d6f096e9bfb180f688a4418c4974eb4. *** Reason for rollback *** Contributor finds some bugs and after fixing some bugs there are more bugs to fix now. *** Original change description *** 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 on... *** ROLLBACK_OF=119138157 -- MOS_MIGRATED_REVID=119828267 --- .../build/lib/sandbox/LinuxSandboxedStrategy.java | 93 ++------------------- src/main/tools/namespace-sandbox.c | 7 +- .../lib/sandbox/LinuxSandboxedStrategyTest.java | 96 ++-------------------- 3 files changed, 17 insertions(+), 179 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 4aabf17897..b953b8a193 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,7 +43,6 @@ 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; @@ -55,12 +54,9 @@ 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; @@ -258,70 +254,13 @@ 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(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; + finalizedMounts.put(symlinkTarget, symlinkTarget); } } @@ -341,7 +280,6 @@ 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(); @@ -350,37 +288,16 @@ 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)); - FileStatus subStat = subSource.statNullable(Symlinks.NOFOLLOW); - if (subStat.isDirectory()) { - fullyMountedDirectories.add(new SimpleImmutableEntry(subTarget, subSource)); - } - finalizeMountPath(finalizedMounts, subTarget, subSource, subStat); + finalizeMountPath( + finalizedMounts, subTarget, subSource, subSource.statNullable(Symlinks.NOFOLLOW)); } } else { finalizeMountPath(finalizedMounts, target, source, stat); } } - - 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; + return finalizedMounts; } /** diff --git a/src/main/tools/namespace-sandbox.c b/src/main/tools/namespace-sandbox.c index a454a70ebe..04f1c9c734 100644 --- a/src/main/tools/namespace-sandbox.c +++ b/src/main/tools/namespace-sandbox.c @@ -134,10 +134,9 @@ 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 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" + " -M/-m system directory to mount inside the sandbox\n" + " Multiple 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 d5d2439b77..b2fe0e6720 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,6 +29,7 @@ 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; @@ -104,9 +105,6 @@ 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(); @@ -117,15 +115,6 @@ 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()); @@ -148,9 +137,10 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { Map testFiles = new LinkedHashMap<>(); testFiles.put("symlink.txt", "goal.txt"); testFiles.put("goal.txt", ""); - testFiles.put("other.txt", ""); - Map assertMounts = ImmutableMap.of("symlink.txt", "goal.txt"); + List assertMounts = new ArrayList<>(); + assertMounts.add("symlink.txt"); + assertMounts.add("goal.txt"); assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts)); } @@ -160,10 +150,9 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { Map testFiles = ImmutableMap.of( "symlink.txt", "x/goal.txt", - "x/goal.txt", "", - "x/other.txt", ""); + "x/goal.txt", ""); - Map assertMounts = ImmutableMap.of("symlink.txt", "x/goal.txt"); + List assertMounts = ImmutableList.of("symlink.txt", "x/goal.txt"); assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts)); } @@ -172,10 +161,9 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { Map testFiles = ImmutableMap.of( "x/symlink.txt", "../goal.txt", - "goal.txt", "", - "x/other.txt", ""); + "goal.txt", ""); - Map assertMounts = ImmutableMap.of("x/symlink.txt", "goal.txt"); + List assertMounts = ImmutableList.of("x/symlink.txt", "goal.txt"); assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts)); } @@ -190,73 +178,7 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase { "a/b/y.txt", "z.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"); + List assertMounts = ImmutableList.of("a/b/x.txt", "a/b/y.txt", "a/b/z.txt"); assertThat(userFriendlyMounts(testFiles, inputFile)) .isEqualTo(userFriendlyAsserts(assertMounts)); -- cgit v1.2.3