Check for symlink collision

bazelbuild · Apr 25, 2023 · eb5f259 · eb5f259
1 parent 602a302
commit eb5f259
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 12 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -1092,21 +1092,37 @@ ActionResultMetadata parseActionResultMetadata(
           new FileMetadata(localPath, outputFile.getDigest(), outputFile.getIsExecutable()));
     }
 
-    ImmutableMap.Builder<Path, SymlinkMetadata> symlinks = ImmutableMap.builder();
-    var outputSymlinks = ImmutableSet.<OutputSymlink>builder()
-      .addAll(result.getOutputFileSymlinks())
-      .addAll(result.getOutputDirectorySymlinks())
-      .addAll(result.getOutputSymlinks())
-      .build();
-    for (OutputSymlink symlink : outputSymlinks) {
-      Path localPath =
-          remotePathResolver.outputPathToLocalPath(encodeBytestringUtf8(symlink.getPath()));
-      symlinks.put(
-          localPath, new SymlinkMetadata(localPath, PathFragment.create(symlink.getTarget())));
+    Map<Path, SymlinkMetadata> symlinkMap = new HashMap<>();
+    var symlinkLists =
+        ImmutableList.of(
+            result.getOutputFileSymlinks(),
+            result.getOutputDirectorySymlinks(),
+            result.getOutputSymlinks());
+    for (var list : symlinkLists) {
+      for (OutputSymlink symlink : list) {
+        Path localPath =
+            remotePathResolver.outputPathToLocalPath(encodeBytestringUtf8(symlink.getPath()));
+        var target = PathFragment.create(symlink.getTarget());
+
+        var existingMetadata = symlinkMap.get(localPath);
+        if (existingMetadata != null) {
+          if (!target.equals(existingMetadata.target())) {
+            throw new IOException(
+                String.format(
+                    "Symlink path collision: '%s' is mapped to both '%s' and '%s'. Action Result"
+                        + " should not contain multiple targets for the same symlink.",
+                    localPath, existingMetadata.target(), target));
+          }
+
+          continue;
+        }
+
+        symlinkMap.put(localPath, new SymlinkMetadata(localPath, target));
+      }
     }
 
     return new ActionResultMetadata(
-        files.buildOrThrow(), symlinks.buildOrThrow(), directories.buildOrThrow());
+        files.buildOrThrow(), ImmutableMap.copyOf(symlinkMap), directories.buildOrThrow());
   }
 
   /**

diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -729,6 +729,28 @@ public void downloadOutputs_absoluteSymlinkInDirectory_error() throws Exception
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
+  @Test
+  public void downloadOutputs_symlinkCollision_error() throws Exception {
+    ActionResult.Builder builder = ActionResult.newBuilder();
+    builder.addOutputDirectorySymlinksBuilder().setPath("outputs/foo").setTarget("foo1");
+    builder.addOutputSymlinksBuilder().setPath("outputs/foo").setTarget("foo2");
+    RemoteActionResult result =
+        RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build()));
+    Spawn spawn = newSpawnFromResult(result);
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteExecutionService service = newRemoteExecutionService();
+    RemoteAction action = service.buildRemoteAction(spawn, context);
+
+    IOException expected =
+        assertThrows(IOException.class, () -> service.downloadOutputs(action, result));
+
+    assertThat(expected.getSuppressed()).isEmpty();
+    assertThat(expected).hasMessageThat().contains("Symlink path collision");
+    assertThat(expected).hasMessageThat().contains("outputs/foo");
+    assertThat(expected).hasMessageThat().contains("foo1");
+    assertThat(expected).hasMessageThat().contains("foo2");
+  }
+
   @Test
   public void downloadOutputs_onFailure_maintainDirectories() throws Exception {
     // Test that output directories are not deleted on download failure. See