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 <https://gist.github.com/bsilver8192/10527a862ce16bb7f79a>;
with 30000 inputs moved to a subdirectory and on...

***

ROLLBACK_OF=119138157

--
MOS_MIGRATED_REVID=119828267

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<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;
+      finalizedMounts.put(symlinkTarget, symlinkTarget);
     }
   }
 
@@ -341,7 +280,6 @@ 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();
@@ -350,37 +288,16 @@ 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));
-          FileStatus subStat = subSource.statNullable(Symlinks.NOFOLLOW);
-          if (subStat.isDirectory()) {
-            fullyMountedDirectories.add(new SimpleImmutableEntry<Path, Path>(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<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;
+    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 <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 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 <source/target>  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<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();
@@ -117,15 +115,6 @@ 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());
@@ -148,9 +137,10 @@ 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", "");
 
-    Map<String, String> assertMounts = ImmutableMap.of("symlink.txt", "goal.txt");
+    List<String> 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<String, String> testFiles =
         ImmutableMap.of(
             "symlink.txt", "x/goal.txt",
-            "x/goal.txt", "",
-            "x/other.txt", "");
+            "x/goal.txt", "");
 
-    Map<String, String> assertMounts = ImmutableMap.of("symlink.txt", "x/goal.txt");
+    List<String> assertMounts = ImmutableList.of("symlink.txt", "x/goal.txt");
     assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts));
   }
 
@@ -172,10 +161,9 @@ public class LinuxSandboxedStrategyTest extends LinuxSandboxedStrategyTestCase {
     Map<String, String> testFiles =
         ImmutableMap.of(
             "x/symlink.txt", "../goal.txt",
-            "goal.txt", "",
-            "x/other.txt", "");
+            "goal.txt", "");
 
-    Map<String, String> assertMounts = ImmutableMap.of("x/symlink.txt", "goal.txt");
+    List<String> 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<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");
+    List<String> assertMounts = ImmutableList.of("a/b/x.txt", "a/b/y.txt", "a/b/z.txt");
 
     assertThat(userFriendlyMounts(testFiles, inputFile))
         .isEqualTo(userFriendlyAsserts(assertMounts));