Skip to content

Commit

Permalink
[7.1.0] Remove the restriction that relative symlinks in a tree artif…
Browse files Browse the repository at this point in the history 
…act may not point outside the tree. (#21449)

As discussed in #20891, the
restriction is pointless, as it can already be bypassed with an absolute
symlink. Note that the symlinks are still required not to dangle in all
cases.

Also rename and reorganize the tests in TreeArtifactBuildTest in a more
logical manner.

Fixes #20891.

Closes #21263.

Commit
5506a0f

PiperOrigin-RevId: 608926604
Change-Id: I967a383b9891360700f868abd5c2d292e0e7974e

Co-authored-by: thesayyn <thesayyn@gmail.com>
  • Loading branch information
@bazel-io @thesayyn
bazel-io and thesayyn authored Feb 21, 2024
1 parent 0e2f609 commit 1d46604
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 100 deletions.
29 changes: 0 additions & 29 deletions src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -552,8 +552,6 @@ private void visit(
Path path = parentDir.getRelative(parentRelativePath);

if (type == Dirent.Type.SYMLINK) {
checkSymlink(parentRelativePath.getParentDirectory(), path);

traversedSymlink = true;

FileStatus statFollow = path.statIfFound(Symlinks.FOLLOW);
Expand Down Expand Up @@ -621,33 +619,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 =
Expand Down
87 changes: 36 additions & 51 deletions src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,9 @@ void run(ActionExecutionContext context) throws IOException {
}

@Test
public void validRelativeSymlinkAccepted() throws Exception {
public void validAbsoluteSymlinkAccepted() throws Exception {
scratch.overwriteFile("/random/pointer");

SpecialArtifact out = createTreeArtifact("output");

Action action =
Expand All @@ -470,7 +472,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"), "../one");
out.getPath().getChild("links").getChild("link"), "/random/pointer");
}
};

Expand All @@ -479,7 +481,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
}

@Test
public void invalidSymlinkRejected() {
public void danglingAbsoluteSymlinkRejected() {
// Failure expected
EventCollector eventCollector = new EventCollector(EventKind.ERROR);
reporter.removeHandler(failFastHandler);
Expand All @@ -494,14 +496,14 @@ 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"), "../invalid");
out.getPath().getChild("links").getChild("link"), "/random/pointer");
}
};

registerAction(action);
assertThrows(BuildFailedException.class, () -> buildArtifact(out));

List<Event> errors = ImmutableList.copyOf(eventCollector);
ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
assertThat(errors).hasSize(2);
assertThat(errors.get(0).getMessage())
.contains(
Expand All @@ -510,12 +512,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
}

@Test
public void absoluteSymlinkBadTargetRejected() {
// Failure expected
EventCollector eventCollector = new EventCollector(EventKind.ERROR);
reporter.removeHandler(failFastHandler);
reporter.addHandler(eventCollector);

public void validRelativeSymlinkAccepted() throws Exception {
SpecialArtifact out = createTreeArtifact("output");

Action action =
Expand All @@ -525,24 +522,20 @@ 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"), "/random/pointer");
out.getPath().getChild("links").getChild("link"), "../one");
}
};

registerAction(action);
assertThrows(BuildFailedException.class, () -> buildArtifact(out));

List<Event> errors = ImmutableList.copyOf(eventCollector);
assertThat(errors).hasSize(2);
assertThat(errors.get(0).getMessage())
.contains(
"Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
buildArtifact(out);
}

@Test
public void absoluteSymlinkAccepted() throws Exception {
scratch.overwriteFile("/random/pointer");
public void danglingRelativeSymlinkRejected() {
// Failure expected
EventCollector eventCollector = new EventCollector(EventKind.ERROR);
reporter.removeHandler(failFastHandler);
reporter.addHandler(eventCollector);

SpecialArtifact out = createTreeArtifact("output");

Expand All @@ -553,78 +546,70 @@ 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"), "/random/pointer");
out.getPath().getChild("links").getChild("link"), "../invalid");
}
};

registerAction(action);
buildArtifact(out);
assertThrows(BuildFailedException.class, () -> buildArtifact(out));

ImmutableList<Event> errors = ImmutableList.copyOf(eventCollector);
assertThat(errors).hasSize(2);
assertThat(errors.get(0).getMessage())
.contains(
"Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
}

@Test
public void relativeSymlinkTraversingOutsideOfTreeArtifactRejected() {
// Failure expected
EventCollector eventCollector = new EventCollector(EventKind.ERROR);
reporter.removeHandler(failFastHandler);
reporter.addHandler(eventCollector);

public void validRelativeSymlinkToOutsideOfTreeArtifactAccepted() throws Exception {
SpecialArtifact out = createTreeArtifact("output");

Action action =
scratch.file(out.getPath().getRelative("../some/file").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/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 relativeSymlinkTraversingToDirOutsideOfTreeArtifactRejected() throws Exception {
public void danglingRelativeSymlinkOutsideOfTreeArtifactRejected() 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 =
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);

ImmutableList<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");
"Child links/link of tree artifact " + out.getPath() + " is a dangling symbolic link");
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
}

Expand Down
65 changes: 45 additions & 20 deletions src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,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");
Expand All @@ -569,6 +569,33 @@ 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.file("other_tree/file");
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");
Expand All @@ -595,29 +622,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_permitsUplevelSymlinkTraversingOutsideThenBackInsideTree()
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
Expand Down

0 comments on commit 1d46604

Please sign in to comment.