diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java index 5aac578fe3cd33..a6ebff503b0f77 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java @@ -289,26 +289,19 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa TreeArtifactValue.visitTree( treeDir, - (parentRelativePath, type) -> { - if (chmod && type != Dirent.Type.SYMLINK) { + (parentRelativePath, type, traversedSymlink) -> { + checkState(type == Dirent.Type.FILE || type == Dirent.Type.DIRECTORY); + // Set the output permissions when the execution mode requires it, unless at least one + // symlink was traversed on the way to this entry, as it might have led outside of the + // root directory. + if (chmod && !traversedSymlink) { setPathPermissions(treeDir.getRelative(parentRelativePath)); } if (type == Dirent.Type.DIRECTORY) { return; // The final TreeArtifactValue does not contain child directories. } TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, parentRelativePath); - FileArtifactValue metadata; - try { - metadata = constructFileArtifactValueFromFilesystem(child); - } catch (FileNotFoundException e) { - String errorMessage = - String.format( - "Failed to resolve relative path %s inside TreeArtifact %s. " - + "The associated file is either missing or is an invalid symlink.", - parentRelativePath, treeDir); - throw new IOException(errorMessage, e); - } - + FileArtifactValue metadata = constructFileArtifactValueFromFilesystem(child); // visitTree() uses multiple threads and putChild() is not thread-safe synchronized (tree) { if (metadata.isRemote()) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index 83c4af1aafe618..8b50478673e17f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java @@ -464,7 +464,7 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value) try { TreeArtifactValue.visitTree( path, - (child, type) -> { + (child, type, traversedSymlink) -> { if (type != Dirent.Type.DIRECTORY) { currentChildren.add(child); } 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 a6c8be5400bc00..a247913b381955 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 @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.Dirent; +import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.IORuntimeException; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -487,20 +488,34 @@ public interface TreeArtifactVisitor { * Called for every directory entry encountered during tree traversal, in a nondeterministic * order. * - *
Symlinks are not followed during traversal and are simply reported as {@link - * Dirent.Type#SYMLINK} regardless of whether they point to a file, directory, or are dangling. + *
Regular files and directories are reported as {@link Dirent.Type.FILE} or {@link + * Dirent.Type.DIRECTORY}, respectively. Directories are traversed recursively. * - *
{@code type} is guaranteed never to be {@link Dirent.Type#UNKNOWN}, since if this type is - * encountered, {@link IOException} is immediately thrown without invoking the visitor. + *
Symlinks that resolve to an existing file or directory are followed and reported as the + * regular files or directories they point to, recursively for directories. Symlinks that fail + * to resolve to an existing path cause an {@link IOException} to be immediately thrown without + * invoking the visitor. Thus, the visitor is never called with a {@link Dirent.Type.SYMLINK} + * type. * - *
If the implementation throws {@link IOException}, traversal is immediately halted and the + *
Special files or files whose type could not be determined, regardless of whether they are + * encountered directly or indirectly through symlinks, cause an {@link IOException} to be + * immediately thrown without invoking the visitor. Thus, the visitor is never called with a + * {@link Dirent.Type.UNKNOWN} type. + * + *
The {@code parentRelativePath} argument is always set to the apparent path relative to the + * tree directory root, without resolving any intervening symlinks. The {@code traversedSymlink} + * argument is true if at least one symlink was traversed on the way to the entry being + * reported. + * + *
If the visitor throws {@link IOException}, traversal is immediately halted and the * exception is propagated. * *
This method can be called from multiple threads in parallel during a single call of {@link
* TreeArtifactVisitor#visitTree(Path, TreeArtifactVisitor)}.
*/
@ThreadSafe
- void visit(PathFragment parentRelativePath, Dirent.Type type) throws IOException;
+ void visit(PathFragment parentRelativePath, Dirent.Type type, boolean traversedSymlink)
+ throws IOException;
}
/** An {@link AbstractQueueVisitor} that visits every file in the tree artifact. */
@@ -519,7 +534,12 @@ static class Visitor extends AbstractQueueVisitor {
}
void run() throws IOException, InterruptedException {
- execute(() -> visit(PathFragment.EMPTY_FRAGMENT, Dirent.Type.DIRECTORY));
+ execute(
+ () ->
+ visit(
+ PathFragment.EMPTY_FRAGMENT,
+ Dirent.Type.DIRECTORY,
+ /* traversedSymlink= */ false));
try {
awaitQuiescence(true);
} catch (IORuntimeException e) {
@@ -527,25 +547,49 @@ void run() throws IOException, InterruptedException {
}
}
- private void visit(PathFragment parentRelativePath, Dirent.Type type) {
+ private void visit(
+ PathFragment parentRelativePath, Dirent.Type type, boolean traversedSymlink) {
try {
Path path = parentDir.getRelative(parentRelativePath);
- if (type == Dirent.Type.UNKNOWN) {
- throw new IOException("Could not determine type of file for " + path.getPathString());
- }
-
if (type == Dirent.Type.SYMLINK) {
checkSymlink(parentRelativePath.getParentDirectory(), path);
+
+ traversedSymlink = true;
+
+ FileStatus statFollow = path.statIfFound(Symlinks.FOLLOW);
+
+ if (statFollow == null) {
+ throw new IOException(
+ String.format(
+ "Child %s of tree artifact %s is a dangling symbolic link",
+ parentRelativePath, parentDir));
+ }
+
+ if (statFollow.isFile() && !statFollow.isSpecialFile()) {
+ type = Dirent.Type.FILE;
+ } else if (statFollow.isDirectory()) {
+ type = Dirent.Type.DIRECTORY;
+ } else {
+ type = Dirent.Type.UNKNOWN;
+ }
+ }
+
+ if (type == Dirent.Type.UNKNOWN) {
+ throw new IOException(
+ String.format(
+ "Child %s of tree artifact %s has an unsupported type",
+ parentRelativePath, parentDir));
}
- visitor.visit(parentRelativePath, type);
+ visitor.visit(parentRelativePath, type, traversedSymlink);
if (type == Dirent.Type.DIRECTORY) {
for (Dirent dirent : path.readdir(Symlinks.NOFOLLOW)) {
PathFragment childPath = parentRelativePath.getChild(dirent.getName());
Dirent.Type childType = dirent.getType();
- execute(() -> visit(childPath, childType));
+ boolean finalTraversedSymlink = traversedSymlink;
+ execute(() -> visit(childPath, childType, finalTraversedSymlink));
}
}
} catch (IOException e) {
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
index c5f2de6c90d0ca..81ef77c862a455 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
@@ -1655,7 +1655,7 @@ public TreeArtifactValue getTreeArtifactValue(SpecialArtifact output)
if (treeDir.exists()) {
TreeArtifactValue.visitTree(
treeDir,
- (parentRelativePath, type) -> {
+ (parentRelativePath, type, traversedSymlink) -> {
if (type == Dirent.Type.DIRECTORY) {
return;
}
diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java
index 93486434e52baa..7185d4e315bb5d 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java
@@ -975,7 +975,7 @@ protected static TreeArtifactValue createTreeArtifactValue(Artifact tree) throws
TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder((SpecialArtifact) tree);
TreeArtifactValue.visitTree(
tree.getPath(),
- (parentRelativePath, type) -> {
+ (parentRelativePath, type, traversedSymlink) -> {
if (type.equals(Dirent.Type.DIRECTORY)) {
return;
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
index d92e30f69c906d..8b4b692ef956e1 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
@@ -676,4 +676,34 @@ public void downloadTopLevel_genruleSymlinkToOutput() throws Exception {
assertSymlink("foo-link", PathFragment.create("foo"));
assertValidOutputFile("foo-link", "hello\n");
}
+
+ @Test
+ public void remoteAction_inputTreeWithSymlinks() throws Exception {
+ setDownloadToplevel();
+ write(
+ "tree.bzl",
+ "def _impl(ctx):",
+ " d = ctx.actions.declare_directory(ctx.label.name)",
+ " ctx.actions.run_shell(",
+ " outputs = [d],",
+ " command = 'mkdir $1/dir && touch $1/file $1/dir/file && ln -s file $1/filesym && ln"
+ + " -s dir $1/dirsym',",
+ " arguments = [d.path],",
+ " )",
+ " return DefaultInfo(files = depset([d]))",
+ "tree = rule(_impl)");
+ write(
+ "BUILD",
+ "load(':tree.bzl', 'tree')",
+ "tree(name = 'tree')",
+ "genrule(name = 'gen', srcs = [':tree'], outs = ['out'], cmd = 'touch $@')");
+
+ // Populate cache
+ buildTarget("//:gen");
+
+ // Delete output, replay from cache
+ getOutputPath("tree").deleteTree();
+ getOutputPath("out").delete();
+ buildTarget("//:gen");
+ }
}
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 e97ad2c7ab21d8..54fe41af6b54db 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
@@ -404,6 +404,61 @@ void run(ActionExecutionContext context) throws IOException {
checkFilePermissions(out.getPath().getChild("three").getChild("four"));
}
+ @Test
+ public void doesNotSetPermissionsAfterTraversingSymlink() throws Exception {
+ SpecialArtifact out = createTreeArtifact("output");
+
+ Path fileTarget = scratch.file("file");
+ writeFile(fileTarget, "file");
+
+ Path dirTarget = scratch.dir("dir");
+ Path dirFileTarget = dirTarget.getChild("file");
+ writeFile(dirFileTarget, "dir/file");
+
+ Action action =
+ new SimpleTestAction(out) {
+ @Override
+ void run(ActionExecutionContext context) throws IOException {
+ out.getPath().getChild("file_link").createSymbolicLink(fileTarget.asFragment());
+ out.getPath().getChild("dir_link").createSymbolicLink(dirTarget.asFragment());
+ }
+ };
+
+ registerAction(action);
+ buildArtifact(out);
+
+ assertThat(fileTarget.isWritable()).isTrue();
+ assertThat(dirTarget.isWritable()).isTrue();
+ assertThat(dirFileTarget.isWritable()).isTrue();
+ }
+
+ @Test
+ public void symlinkLoopRejected() throws Exception {
+ // 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 context) throws IOException {
+ writeFile(out.getPath().getRelative("dir/file"), "contents");
+ out.getPath().getRelative("dir/sym").createSymbolicLink(PathFragment.create("../dir"));
+ }
+ };
+
+ registerAction(action);
+ assertThrows(BuildFailedException.class, () -> buildArtifact(out));
+
+ ImmutableList