refactor: allow relative symlinks that points out of the treeartifact

src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java

@@ -553,7 +553,6 @@ private void visit(
  Path path = parentDir.getRelative(parentRelativePath);
 
  if (type == Dirent.Type.SYMLINK) {
- checkSymlink(parentRelativePath.getParentDirectory(), path);
 
  traversedSymlink = true;
 
@@ -622,33 +621,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 =

src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java

@@ -562,72 +562,50 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
  }
 
  @Test
- public void relativeSymlinkTraversingOutsideOfTreeArtifactRejected() {
- // Failure expected
- EventCollector eventCollector = new EventCollector(EventKind.ERROR);
- reporter.removeHandler(failFastHandler);
- reporter.addHandler(eventCollector);
-
+ public void relativeSymlinkTraversingToDirOutsideOfTreeArtifact() throws Exception {
  SpecialArtifact out = createTreeArtifact("output");
 
- Action action =
+ // 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"), "../../output/random/pointer");
+ 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");
+ buildArtifact(out);
  }
 
  @Test
- public void relativeSymlinkTraversingToDirOutsideOfTreeArtifactRejected() throws Exception {
- // Failure expected
- EventCollector eventCollector = new EventCollector(EventKind.ERROR);
- reporter.removeHandler(failFastHandler);
- reporter.addHandler(eventCollector);
-
+ public void relativeSymlinkTraversingOutsideOfTreeArtifact() throws Exception {
  SpecialArtifact out = createTreeArtifact("output");
 
- // Create a valid directory that can be referenced
- scratch.dir(out.getRoot().getRoot().getRelative("some/dir").getPathString());
+ scratch.file(out.getRoot().getRoot().getRelative("some/file").getPathString());
 
- TestAction action =
+
+ 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"), "../../some/dir");
+ out.getPath().getChild("links").getChild("link"), "../../some/file");
  }
  };
 
  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");
+ buildArtifact(out);
  }
 
+
  @Test
  public void constructMetadataForDigest() throws Exception {
  SpecialArtifact out = createTreeArtifact("output");

src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java

@@ -545,7 +545,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");
@@ -568,6 +568,32 @@ public void visitTree_pemitsUpLevelSymlinkInsideTree() throws Exception {
  VisitTreeArgs.of(PathFragment.create("a/up_link"), Dirent.Type.FILE, true));
  }
 
+ @Test
+ public void visitTree_permitsUpLevelSymlinkOutsideTree() throws Exception {
+ Path otherTreeDir = scratch.dir("other_tree");
+ scratch.file("other_tree/file");
+ Path treeDir = scratch.dir("tree");
+ scratch.file("tree/file");
+ scratch.dir("tree/a");
+ scratch.resolve("tree/a/uplink").createSymbolicLink(PathFragment.create("../../other_tree/file"));
+ List<VisitTreeArgs> children = new ArrayList<>();
+
+ TreeArtifactValue.visitTree(
+ treeDir,
+ (child, type, traversedSymlink) -> {
+ synchronized (children) {
+ children.add(VisitTreeArgs.of(child, type, traversedSymlink));
+ }
+ });
+
+ assertThat(children)
+ .containsExactly(
+ VisitTreeArgs.of(PathFragment.create(""), Dirent.Type.DIRECTORY, false),
+ VisitTreeArgs.of(PathFragment.create("file"), Dirent.Type.FILE, false),
+ VisitTreeArgs.of(PathFragment.create("a"), Dirent.Type.DIRECTORY, false),
+ VisitTreeArgs.of(PathFragment.create("a/uplink"), Dirent.Type.FILE, true));
+ }
+
  @Test
  public void visitTree_permitsAbsoluteSymlink() throws Exception {
  Path treeDir = scratch.dir("tree");
@@ -594,29 +620,27 @@ public void visitTree_permitsAbsoluteSymlink() throws Exception {
  }
 
  @Test
- public void visitTree_throwsOnSymlinkPointingOutsideTree() 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, traversedSymlink) -> {}));
- assertThat(e).hasMessageThat().contains("/tree/link pointing to ../outside");
- }
-
- @Test
- public void visitTree_throwsOnSymlinkTraversingOutsideThenBackInsideTree() throws Exception {
+ public void visitTree_permitsSymlinkTraversingOutsideThenBackInsideTree() throws Exception {
  Path treeDir = scratch.dir("tree");
  scratch.file("tree/file");
  scratch.resolve("tree/link").createSymbolicLink(PathFragment.create("../tree/file"));
 
- Exception e =
- assertThrows(
- IOException.class,
- () -> TreeArtifactValue.visitTree(treeDir, (child, type, traversedSymlink) -> {}));
- assertThat(e).hasMessageThat().contains("/tree/link pointing to ../tree/file");
+ List<VisitTreeArgs> children = new ArrayList<>();
+
+ TreeArtifactValue.visitTree(
+ treeDir,
+ (child, type, traversedSymlink) -> {
+ synchronized (children) {
+ children.add(VisitTreeArgs.of(child, type, traversedSymlink));
+ }
+ });
+
+ assertThat(children)
+ .containsExactly(
+ VisitTreeArgs.of(PathFragment.create(""), Dirent.Type.DIRECTORY, false),
+ VisitTreeArgs.of(PathFragment.create("file"), Dirent.Type.FILE, false),
+ VisitTreeArgs.of(
+ PathFragment.create("link"), Dirent.Type.FILE, true));
  }
 
  @Test