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 <https://gist.github.com/bsilver8192/10527a862ce16bb7f79a>;
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

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<Path, Path> directory,
+      MountMap mounts,
+      Set<Entry<Path, Path>> fullyMountedDirectories,
+      Set<Entry<Path, Path>> 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<Path, Path>(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<Path, Path> mounts) throws IOException {
+    Set<Entry<Path, Path>> fullyMountedDirectories = new HashSet<>();
     MountMap finalizedMounts = new MountMap();
     for (Entry<Path, Path> 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<Path, Path>(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<Path, Path>(subTarget, subSource));
+          }
+          finalizeMountPath(finalizedMounts, subTarget, subSource, subStat);
         }
       } else {
         finalizeMountPath(finalizedMounts, target, source, stat);
       }
     }
-    return finalizedMounts;
+
+    MountMap deduplicatedMounts = new MountMap();
+    Set<Entry<Path, Path>> notFullyMountedDirectories = new HashSet<>();
+    for (Entry<Path, Path> mount : finalizedMounts.entrySet()) {
+      Entry<Path, Path> 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<Path, Path> 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 <timeout>  in case timeout occurs, how long to wait before killing "
       "the child with SIGKILL\n"
       "  -d <dir>  create an empty directory in the sandbox\n"
-      "  -M/-m <source/target>  system directory to mount inside the sandbox\n"
-      "    Multiple directories can be specified and each of them will be "
-      "mounted readonly.\n"
+      "  -M/-m <source/target>  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<String, String> userFriendlyAsserts(List<String> asserts) {
     return userFriendlyMap(asserts(asserts));
   }
+  private ImmutableMap<String, String> userFriendlyAsserts(Map<String, String> asserts) {
+    return userFriendlyMap(asserts(asserts));
+  }
 
   private ImmutableMap<Path, Path> asserts(List<String> asserts) {
     ImmutableMap.Builder<Path, Path> pathifiedAsserts = ImmutableMap.builder();
@@ -115,6 +117,15 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase {
     return pathifiedAsserts.build();
   }
 
+  private ImmutableMap<Path, Path> asserts(Map<String, String> asserts) {
+    ImmutableMap.Builder<Path, Path> pathifiedAsserts = ImmutableMap.builder();
+    for (Map.Entry<String, String> file : asserts.entrySet()) {
+      pathifiedAsserts.put(
+          workspaceDir.getRelative(file.getKey()), workspaceDir.getRelative(file.getValue()));
+    }
+    return pathifiedAsserts.build();
+  }
+
   private void createTreeStructure(Map<String, String> linksAndFiles) throws Exception {
     for (Entry<String, String> entry : linksAndFiles.entrySet()) {
       Path filePath = workspaceDir.getRelative(entry.getKey());
@@ -137,10 +148,9 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase {
     Map<String, String> testFiles = new LinkedHashMap<>();
     testFiles.put("symlink.txt", "goal.txt");
     testFiles.put("goal.txt", "");
+    testFiles.put("other.txt", "");
 
-    List<String> assertMounts = new ArrayList<>();
-    assertMounts.add("symlink.txt");
-    assertMounts.add("goal.txt");
+    Map<String, String> assertMounts = ImmutableMap.of("symlink.txt", "goal.txt");
 
     assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts));
   }
@@ -150,9 +160,10 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase {
     Map<String, String> testFiles =
         ImmutableMap.of(
             "symlink.txt", "x/goal.txt",
-            "x/goal.txt", "");
+            "x/goal.txt", "",
+            "x/other.txt", "");
 
-    List<String> assertMounts = ImmutableList.of("symlink.txt", "x/goal.txt");
+    Map<String, String> assertMounts = ImmutableMap.of("symlink.txt", "x/goal.txt");
     assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts));
   }
 
@@ -161,9 +172,10 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase {
     Map<String, String> testFiles =
         ImmutableMap.of(
             "x/symlink.txt", "../goal.txt",
-            "goal.txt", "");
+            "goal.txt", "",
+            "x/other.txt", "");
 
-    List<String> assertMounts = ImmutableList.of("x/symlink.txt", "goal.txt");
+    Map<String, String> 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<String> assertMounts = ImmutableList.of("a/b/x.txt", "a/b/y.txt", "a/b/z.txt");
+    List<String> assertMounts = ImmutableList.of("a/b");
+
+    assertThat(userFriendlyMounts(testFiles, inputFile))
+        .isEqualTo(userFriendlyAsserts(assertMounts));
+  }
+
+  @Test
+  public void testDetectsWholeDir() throws Exception {
+    ImmutableList<String> inputFile = ImmutableList.of("a/x.txt", "a/z.txt");
+
+    Map<String, String> testFiles =
+        ImmutableMap.of(
+            "a/x.txt", "",
+            "a/z.txt", "");
+
+    List<String> assertMounts = ImmutableList.of("a");
+
+    assertThat(userFriendlyMounts(testFiles, inputFile))
+        .isEqualTo(userFriendlyAsserts(assertMounts));
+  }
+
+  @Test
+  public void testExcludesOtherDir() throws Exception {
+    ImmutableList<String> inputFile = ImmutableList.of("a/x.txt", "a/y.txt");
+
+    Map<String, String> testFiles =
+        ImmutableMap.of(
+            "a/x.txt", "",
+            "a/y.txt", "",
+            "a/b/", "");
+
+    List<String> assertMounts = ImmutableList.of("a/x.txt", "a/y.txt");
+
+    assertThat(userFriendlyMounts(testFiles, inputFile))
+        .isEqualTo(userFriendlyAsserts(assertMounts));
+  }
+
+  @Test
+  public void testExcludesOtherFiles() throws Exception {
+    ImmutableList<String> inputFile = ImmutableList.of("a/x.txt", "a/z.txt");
+
+    Map<String, String> testFiles =
+        ImmutableMap.of(
+            "a/x.txt", "",
+            "a/y.txt", "z.txt",
+            "a/z.txt", "");
+
+    List<String> assertMounts = ImmutableList.of("a/x.txt", "a/z.txt");
+
+    assertThat(userFriendlyMounts(testFiles, inputFile))
+        .isEqualTo(userFriendlyAsserts(assertMounts));
+  }
+
+  @Test
+  public void testRecognizesOtherSymlinks() throws Exception {
+    ImmutableList<String> inputFile = ImmutableList.of("a/a/x.txt", "a/a/y.txt");
+
+    Map<String, String> testFiles =
+        ImmutableMap.of(
+            "a/a/x.txt", "../b/x.txt",
+            "a/a/y.txt", "",
+            "a/b/x.txt", "");
+
+    Map<String, String> 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));