refactor: allow relative symlinks that points out of the treeartifact

bazelbuild · Feb 8, 2024 · 4622cb1 · 4622cb1
1 parent 67ffe04
commit 4622cb1
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 130 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
@@ -542,10 +542,6 @@ private void visitTree(PathFragment subdir) {
                     + parentDir);
           }
 
-          if (type == Dirent.Type.SYMLINK) {
-            checkSymlink(subdir, parentDir.getRelative(parentRelativePath));
-          }
-
           visitor.visit(parentRelativePath, type);
 
           if (type == Dirent.Type.DIRECTORY) {
@@ -582,33 +578,6 @@ public static void visitTree(Path parentDir, TreeArtifactVisitor treeArtifactVis
     visitor.run();
   }
 
-  private static void checkSymlink(PathFragment subDir, Path path) throws IOException {
-    PathFragment linkTarget = path.readSymbolicLinkUnchecked();
-    if (linkTarget.isAbsolute()) {
-      // We tolerate absolute symlinks here. They will probably be dangling if any downstream
-      // consumer tries to read them, but let that be downstream's problem.
-      return;
-    }
-
-    // Visit each path segment of the link target to catch any path traversal outside of the
-    // TreeArtifact root directory. For example, for TreeArtifact a/b/c, it is possible to have a
-    // symlink, a/b/c/sym_link that points to ../outside_dir/../c/link_target. Although this symlink
-    // points to a file under the TreeArtifact, the link target traverses outside of the
-    // TreeArtifact into a/b/outside_dir.
-    PathFragment intermediatePath = subDir;
-    for (String pathSegment : linkTarget.segments()) {
-      intermediatePath = intermediatePath.getRelative(pathSegment);
-      if (intermediatePath.containsUplevelReferences()) {
-        String errorMessage =
-            String.format(
-                "A TreeArtifact may not contain relative symlinks whose target paths traverse "
-                    + "outside of the TreeArtifact, found %s pointing to %s.",
-                path, linkTarget);
-        throw new IOException(errorMessage);
-      }
-    }
-  }
-
   /** Builder for a {@link TreeArtifactValue}. */
   public static final class Builder {
     private final ImmutableSortedMap.Builder<TreeFileArtifact, FileArtifactValue> childData =

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -502,73 +502,6 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
     buildArtifact(out);
   }
 
-  @Test
-  public void relativeSymlinkTraversingOutsideOfTreeArtifactRejected() {
-    // Failure expected
-    EventCollector eventCollector = new EventCollector(EventKind.ERROR);
-    reporter.removeHandler(failFastHandler);
-    reporter.addHandler(eventCollector);
-
-    SpecialArtifact out = createTreeArtifact("output");
-
-    Action action =
-        new SimpleTestAction(out) {
-          @Override
-          void run(ActionExecutionContext actionExecutionContext) throws IOException {
-            writeFile(out.getPath().getChild("one"), "one");
-            writeFile(out.getPath().getChild("two"), "two");
-            FileSystemUtils.ensureSymbolicLink(
-                out.getPath().getChild("links").getChild("link"), "../../output/random/pointer");
-          }
-        };
-
-    registerAction(action);
-
-    assertThrows(BuildFailedException.class, () -> buildArtifact(out));
-    List<Event> errors = ImmutableList.copyOf(eventCollector);
-    assertThat(errors).hasSize(2);
-    assertThat(errors.get(0).getMessage())
-        .contains(
-            "A TreeArtifact may not contain relative symlinks whose target paths traverse "
-                + "outside of the TreeArtifact");
-    assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
-  }
-
-  @Test
-  public void relativeSymlinkTraversingToDirOutsideOfTreeArtifactRejected() throws Exception {
-    // Failure expected
-    EventCollector eventCollector = new EventCollector(EventKind.ERROR);
-    reporter.removeHandler(failFastHandler);
-    reporter.addHandler(eventCollector);
-
-    SpecialArtifact out = createTreeArtifact("output");
-
-    // Create a valid directory that can be referenced
-    scratch.dir(out.getRoot().getRoot().getRelative("some/dir").getPathString());
-
-    TestAction action =
-        new SimpleTestAction(out) {
-          @Override
-          void run(ActionExecutionContext actionExecutionContext) throws IOException {
-            writeFile(out.getPath().getChild("one"), "one");
-            writeFile(out.getPath().getChild("two"), "two");
-            FileSystemUtils.ensureSymbolicLink(
-                out.getPath().getChild("links").getChild("link"), "../../some/dir");
-          }
-        };
-
-    registerAction(action);
-
-    assertThrows(BuildFailedException.class, () -> buildArtifact(out));
-    List<Event> errors = ImmutableList.copyOf(eventCollector);
-    assertThat(errors).hasSize(2);
-    assertThat(errors.get(0).getMessage())
-        .contains(
-            "A TreeArtifact may not contain relative symlinks whose target paths traverse "
-                + "outside of the TreeArtifact");
-    assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
-  }
-
   @Test
   public void constructMetadataForDigest() throws Exception {
     SpecialArtifact out = createTreeArtifact("output");

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
@@ -434,7 +434,7 @@ public void visitTree_propagatesIoExceptionFromVisitor() throws Exception {
   }
 
   @Test
-  public void visitTree_pemitsUpLevelSymlinkInsideTree() throws Exception {
+  public void visitTree_permitsUpLevelSymlinkInsideTree() throws Exception {
     Path treeDir = scratch.dir("tree");
     scratch.file("tree/file");
     scratch.dir("tree/a");
@@ -457,9 +457,11 @@ public void visitTree_pemitsUpLevelSymlinkInsideTree() throws Exception {
   }
 
   @Test
-  public void visitTree_permitsAbsoluteSymlink() throws Exception {
+  public void visitTree_permitsUpLevelDanglingSymlink() throws Exception {
     Path treeDir = scratch.dir("tree");
-    scratch.resolve("tree/absolute_link").createSymbolicLink(PathFragment.create("/tmp"));
+    scratch.file("tree/file");
+    scratch.dir("tree/a");
+    scratch.resolve("tree/a/dangling_uplink").createSymbolicLink(PathFragment.create("../../non_existent"));
     List<Pair<PathFragment, Dirent.Type>> children = new ArrayList<>();
 
     TreeArtifactValue.visitTree(
@@ -471,41 +473,28 @@ public void visitTree_permitsAbsoluteSymlink() throws Exception {
         });
 
     assertThat(children)
-        .containsExactly(Pair.of(PathFragment.create("absolute_link"), Dirent.Type.SYMLINK));
+        .containsExactly(
+            Pair.of(PathFragment.create("file"), Dirent.Type.FILE),
+            Pair.of(PathFragment.create("a"), Dirent.Type.DIRECTORY),
+            Pair.of(PathFragment.create("a/dangling_uplink"), Dirent.Type.SYMLINK));
   }
 
   @Test
-  public void visitTree_throwsOnSymlinkPointingOutsideTree() throws Exception {
+  public void visitTree_permitsAbsoluteSymlink() throws Exception {
     Path treeDir = scratch.dir("tree");
-    scratch.file("outside");
-    scratch.resolve("tree/link").createSymbolicLink(PathFragment.create("../outside"));
-
-    Exception e =
-        assertThrows(
-            IOException.class,
-            () ->
-                TreeArtifactValue.visitTree(
-                    treeDir, (child, type) -> fail("Should not be called")));
-    assertThat(e).hasMessageThat().contains("/tree/link pointing to ../outside");
-  }
+    scratch.resolve("tree/absolute_link").createSymbolicLink(PathFragment.create("/tmp"));
+    List<Pair<PathFragment, Dirent.Type>> children = new ArrayList<>();
 
-  @Test
-  public void visitTree_throwsOnSymlinkTraversingOutsideThenBackInsideTree() throws Exception {
-    Path treeDir = scratch.dir("tree");
-    scratch.file("tree/file");
-    scratch.resolve("tree/link").createSymbolicLink(PathFragment.create("../tree/file"));
+    TreeArtifactValue.visitTree(
+        treeDir,
+        (child, type) -> {
+          synchronized (children) {
+            children.add(Pair.of(child, type));
+          }
+        });
 
-    Exception e =
-        assertThrows(
-            IOException.class,
-            () ->
-                TreeArtifactValue.visitTree(
-                    treeDir,
-                    (child, type) -> {
-                      assertThat(child).isEqualTo(PathFragment.create("file"));
-                      assertThat(type).isEqualTo(Dirent.Type.FILE);
-                    }));
-    assertThat(e).hasMessageThat().contains("/tree/link pointing to ../tree/file");
+    assertThat(children)
+        .containsExactly(Pair.of(PathFragment.create("absolute_link"), Dirent.Type.SYMLINK));
   }
 
   @Test