From dde13d99bbc5bd8e2e2a50bb0b5e085f043a46af Mon Sep 17 00:00:00 2001 From: thesayyn Date: Thu, 8 Feb 2024 11:26:25 -0800 Subject: [PATCH 1/4] refactor: allow relative symlinks that points out of the treeartifact --- .../build/lib/skyframe/TreeArtifactValue.java | 28 -------- .../lib/skyframe/TreeArtifactBuildTest.java | 67 ------------------- .../lib/skyframe/TreeArtifactValueTest.java | 54 ++++++++++++--- 3 files changed, 44 insertions(+), 105 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 index a247913b381955..65c46a1a7d46e9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/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 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 index 54fe41af6b54db..c1c284a17a2f1b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -561,73 +561,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 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 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 index 09d466a309f28c..bf0e3983f7947c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/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,29 @@ 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 treeDir = scratch.dir("tree"); + scratch.file("tree/file"); + scratch.dir("tree/a"); + scratch.resolve("tree/a/uplink").createSymbolicLink(PathFragment.create("../../non_existent")); + List> children = new ArrayList<>(); + + TreeArtifactValue.visitTree( + treeDir, + (child, type) -> { + synchronized (children) { + children.add(Pair.of(child, type)); + } + }); + + assertThat(children) + .containsExactly( + Pair.of(PathFragment.create("file"), Dirent.Type.FILE), + Pair.of(PathFragment.create("a"), Dirent.Type.DIRECTORY), + Pair.of(PathFragment.create("a/uplink"), Dirent.Type.SYMLINK)); + } + @Test public void visitTree_permitsAbsoluteSymlink() throws Exception { Path treeDir = scratch.dir("tree"); @@ -594,18 +617,29 @@ 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")); + public void relativeSymlinkTraversingToDirOutsideOfTreeArtifact() throws Exception { + SpecialArtifact out = createTreeArtifact("output"); - Exception e = - assertThrows( - IOException.class, - () -> TreeArtifactValue.visitTree(treeDir, (child, type, traversedSymlink) -> {})); - assertThat(e).hasMessageThat().contains("/tree/link pointing to ../outside"); + // 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); + buildArtifact(out); + } + @Test public void visitTree_throwsOnSymlinkTraversingOutsideThenBackInsideTree() throws Exception { Path treeDir = scratch.dir("tree"); From 067d0416b84acd78bacffe1afa0763ce227190b4 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Mon, 19 Feb 2024 21:15:44 -0800 Subject: [PATCH 2/4] remove test --- .../lib/skyframe/TreeArtifactValueTest.java | 24 ------------------- 1 file changed, 24 deletions(-) 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 index bf0e3983f7947c..e56fb94ada716f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -616,30 +616,6 @@ public void visitTree_permitsAbsoluteSymlink() throws Exception { PathFragment.create("absolute_dir_link"), Dirent.Type.DIRECTORY, true)); } - @Test - public void relativeSymlinkTraversingToDirOutsideOfTreeArtifact() throws Exception { - 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); - buildArtifact(out); - - } - - @Test public void visitTree_throwsOnSymlinkTraversingOutsideThenBackInsideTree() throws Exception { Path treeDir = scratch.dir("tree"); From cf5c36cfab08d4f8983e7a9dd8fcc7d0b1444af0 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Mon, 19 Feb 2024 21:25:34 -0800 Subject: [PATCH 3/4] tests --- .../lib/skyframe/TreeArtifactBuildTest.java | 42 +++++++++++++++++++ .../lib/skyframe/TreeArtifactValueTest.java | 12 +++--- 2 files changed, 48 insertions(+), 6 deletions(-) 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 index c1c284a17a2f1b..8728c785b93bb2 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -561,6 +561,48 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException { buildArtifact(out); } + @Test + public void relativeSymlinkTraversingToDirOutsideOfTreeArtifact() throws Exception { + 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); + buildArtifact(out); + } + + @Test + public void relativeSymlinkTraversingOutsideOfTreeArtifact() throws Exception { + 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); + buildArtifact(out); + } + + @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 index e56fb94ada716f..9d1dbe62ac9c44 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -574,21 +574,21 @@ public void visitTree_permitsUpLevelSymlinkOutsideTree() throws Exception { scratch.file("tree/file"); scratch.dir("tree/a"); scratch.resolve("tree/a/uplink").createSymbolicLink(PathFragment.create("../../non_existent")); - List> children = new ArrayList<>(); + List children = new ArrayList<>(); TreeArtifactValue.visitTree( treeDir, - (child, type) -> { + (child, type, traversedSymlink) -> { synchronized (children) { - children.add(Pair.of(child, type)); + children.add(VisitTreeArgs.of(child, type, traversedSymlink)); } }); assertThat(children) .containsExactly( - Pair.of(PathFragment.create("file"), Dirent.Type.FILE), - Pair.of(PathFragment.create("a"), Dirent.Type.DIRECTORY), - Pair.of(PathFragment.create("a/uplink"), Dirent.Type.SYMLINK)); + 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.SYMLINK, true)); } @Test From d346546a6e4f59f574c77b46836c1ff6163f4d81 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Mon, 19 Feb 2024 21:45:09 -0800 Subject: [PATCH 4/4] fix tests --- .../lib/skyframe/TreeArtifactBuildTest.java | 5 +++- .../lib/skyframe/TreeArtifactValueTest.java | 30 ++++++++++++++----- 2 files changed, 26 insertions(+), 9 deletions(-) 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 index 8728c785b93bb2..092c2077dab26a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -587,6 +587,9 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException { public void relativeSymlinkTraversingOutsideOfTreeArtifact() throws Exception { SpecialArtifact out = createTreeArtifact("output"); + scratch.file(out.getRoot().getRoot().getRelative("some/file").getPathString()); + + Action action = new SimpleTestAction(out) { @Override @@ -594,7 +597,7 @@ 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/file"); } }; 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 index 9d1dbe62ac9c44..9b1c0f5260b2ab 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -570,10 +570,12 @@ public void visitTree_permitsUpLevelSymlinkInsideTree() throws Exception { @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("../../non_existent")); + scratch.resolve("tree/a/uplink").createSymbolicLink(PathFragment.create("../../other_tree/file")); List children = new ArrayList<>(); TreeArtifactValue.visitTree( @@ -586,9 +588,10 @@ public void visitTree_permitsUpLevelSymlinkOutsideTree() throws Exception { 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.SYMLINK, true)); + VisitTreeArgs.of(PathFragment.create("a/uplink"), Dirent.Type.FILE, true)); } @Test @@ -617,16 +620,27 @@ public void visitTree_permitsAbsoluteSymlink() throws Exception { } @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 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