Skip to content

Commit

Permalink
Remove the restriction that relative symlinks in a tree artifact may …
Browse files Browse the repository at this point in the history 
…not point outside the tree.

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.

PiperOrigin-RevId: 608926604
Change-Id: I967a383b9891360700f868abd5c2d292e0e7974e
  • Loading branch information
@thesayyn @copybara-github
thesayyn authored and copybara-github committed Feb 21, 2024
1 parent 2e82434 commit 5506a0f
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 @@ -553,8 +553,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 @@ -622,33 +620,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 @@ -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");
Expand All @@ -568,6 +568,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 @@ -594,29 +621,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 5506a0f

Please sign in to comment.