sandbox: Improve MountMap to always return entries sorted by path depth and lexicographical order. This prevents certain edge cases in the sandbox, where a mounted child directory could be hidden by a later mount of a parent directory over its parent.

--
MOS_MIGRATED_REVID=104749937

2 files changed, 32 insertions, 33 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 945f267f03..a7657ad6af 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
@@ -193,7 +193,7 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
 
   private ImmutableMap<Path, Path> getMounts(
       Spawn spawn, ActionExecutionContext executionContext) throws IOException {
-    MountMap<Path, Path> mounts = new MountMap<>();
+    MountMap mounts = new MountMap();
     mounts.putAll(mountUsualUnixDirs());
     mounts.putAll(withRecursedDirs(setupBlazeUtils()));
     mounts.putAll(withRecursedDirs(mountRunfilesFromManifests(spawn)));
@@ -230,9 +230,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
    * @return a new mounts multimap with the added mounts.
    */
   @VisibleForTesting
-  static MountMap<Path, Path> withResolvedSymlinks(Map<Path, Path> mounts)
-      throws IOException {
-    MountMap<Path, Path> fixedMounts = new MountMap<>();
+  static MountMap withResolvedSymlinks(Map<Path, Path> mounts) throws IOException {
+    MountMap fixedMounts = new MountMap();
     for (Entry<Path, Path> mount : mounts.entrySet()) {
       Path target = mount.getKey();
       Path source = mount.getValue();
@@ -253,8 +252,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
    * @return a new mounts multimap with the added mounts.
    */
   @VisibleForTesting
-  static MountMap<Path, Path> withRecursedDirs(Map<Path, Path> mounts) throws IOException {
-    MountMap<Path, Path> fixedMounts = new MountMap<>();
+  static MountMap withRecursedDirs(Map<Path, Path> mounts) throws IOException {
+    MountMap fixedMounts = new MountMap();
     for (Entry<Path, Path> mount : mounts.entrySet()) {
       Path target = mount.getKey();
       Path source = mount.getValue();
@@ -275,8 +274,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
    * Mount a certain set of unix directories to make the usual tools and libraries available to the
    * spawn that runs.
    */
-  private MountMap<Path, Path> mountUsualUnixDirs() throws IOException {
-    MountMap<Path, Path> mounts = new MountMap<>();
+  private MountMap mountUsualUnixDirs() throws IOException {
+    MountMap mounts = new MountMap();
     FileSystem fs = blazeDirs.getFileSystem();
     mounts.put(fs.getPath("/bin"), fs.getPath("/bin"));
     mounts.put(fs.getPath("/etc"), fs.getPath("/etc"));
@@ -298,8 +297,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
   /**
    * Mount the embedded tools.
    */
-  private MountMap<Path, Path> setupBlazeUtils() throws IOException {
-    MountMap<Path, Path> mounts = new MountMap<>();
+  private MountMap setupBlazeUtils() throws IOException {
+    MountMap mounts = new MountMap();
     Path mount = blazeDirs.getEmbeddedBinariesRoot().getRelative("build-runfiles");
     mounts.put(mount, mount);
     return mounts;
@@ -308,9 +307,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
   /**
    * Mount all runfiles that the spawn needs as specified in its runfiles manifests.
    */
-  private MountMap<Path, Path> mountRunfilesFromManifests(Spawn spawn)
-      throws IOException {
-    MountMap<Path, Path> mounts = new MountMap<>();
+  private MountMap mountRunfilesFromManifests(Spawn spawn) throws IOException {
+    MountMap mounts = new MountMap();
     for (Entry<PathFragment, Artifact> manifest : spawn.getRunfilesManifests().entrySet()) {
       String manifestFilePath = manifest.getValue().getPath().getPathString();
       Preconditions.checkState(!manifest.getKey().isAbsolute());
@@ -321,9 +319,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
     return mounts;
   }
 
-  static MountMap<Path, Path> parseManifestFile(
-      Path targetDirectory, File manifestFile) throws IOException {
-    MountMap<Path, Path> mounts = new MountMap<>();
+  static MountMap parseManifestFile(Path targetDirectory, File manifestFile) throws IOException {
+    MountMap mounts = new MountMap();
     for (String line : Files.readLines(manifestFile, Charset.defaultCharset())) {
       String[] fields = line.trim().split(" ");
       Path source;
@@ -346,9 +343,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
   /**
    * Mount all runfiles that the spawn needs as specified via its runfiles suppliers.
    */
-  private MountMap<Path, Path> mountRunfilesFromSuppliers(Spawn spawn)
-      throws IOException {
-    MountMap<Path, Path> mounts = new MountMap<>();
+  private MountMap mountRunfilesFromSuppliers(Spawn spawn) throws IOException {
+    MountMap mounts = new MountMap();
     FileSystem fs = blazeDirs.getFileSystem();
     Map<PathFragment, Map<PathFragment, Artifact>> rootsAndMappings =
         spawn.getRunfilesSupplier().getMappings();
@@ -370,10 +366,9 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
   /**
    * Mount all inputs of the spawn.
    */
-  private MountMap<Path, Path> mountInputs(
-      Spawn spawn, ActionExecutionContext actionExecutionContext)
+  private MountMap mountInputs(Spawn spawn, ActionExecutionContext actionExecutionContext)
       throws IOException {
-    MountMap<Path, Path> mounts = new MountMap<>();
+    MountMap mounts = new MountMap();
 
     List<ActionInput> inputs =
         ActionInputHelper.expandMiddlemen(
@@ -404,8 +399,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
    * <p>If --run_under= refers to a label, it is automatically provided in the spawn's input files,
    * so mountInputs() will catch that case.
    */
-  private MountMap<Path, Path> mountRunUnderCommand(Spawn spawn) {
-    MountMap<Path, Path> mounts = new MountMap<>();
+  private MountMap mountRunUnderCommand(Spawn spawn) {
+    MountMap mounts = new MountMap();
 
     if (spawn.getResourceOwner() instanceof TestRunnerAction) {
       TestRunnerAction testRunnerAction = ((TestRunnerAction) spawn.getResourceOwner());
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/MountMap.java b/src/main/java/com/google/devtools/build/lib/sandbox/MountMap.java
index 4e0ae07e41..5051d27df3 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/MountMap.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/MountMap.java
@@ -13,26 +13,30 @@
 // limitations under the License.
 package com.google.devtools.build.lib.sandbox;
 
-import com.google.common.collect.ForwardingMap;
+import com.google.common.collect.ForwardingSortedMap;
+import com.google.devtools.build.lib.vfs.Path;
 
-import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
 
 /**
  * A map that throws an exception when trying to replace a key (i.e. once a key gets a value,
  * any additional attempt of putting a value on the same key will throw an exception).
+ *
+ * <p>Returns entries sorted by path depth (shorter paths first) and in lexicographical order.
  */
-final class MountMap<K, V> extends ForwardingMap<K, V> {
-  final LinkedHashMap<K, V> delegate = new LinkedHashMap<>();
+final class MountMap extends ForwardingSortedMap<Path, Path> {
+  final TreeMap<Path, Path> delegate = new TreeMap<>();
 
   @Override
-  protected Map<K, V> delegate() {
+  protected SortedMap<Path, Path> delegate() {
     return delegate;
   }
 
   @Override
-  public V put(K key, V value) {
-    V previousValue = get(key);
+  public Path put(Path key, Path value) {
+    Path previousValue = get(key);
     if (previousValue == null) {
       return super.put(key, value);
     } else if (previousValue.equals(value)) {
@@ -44,8 +48,8 @@ final class MountMap<K, V> extends ForwardingMap<K, V> {
   }
 
   @Override
-  public void putAll(Map<? extends K, ? extends V> map) {
-    for (Entry<? extends K, ? extends V> entry : map.entrySet()) {
+  public void putAll(Map<? extends Path, ? extends Path> map) {
+    for (Entry<? extends Path, ? extends Path> entry : map.entrySet()) {
       put(entry.getKey(), entry.getValue());
     }
   }