bazelbuild · keertk · Feb 20, 2024 · Feb 19, 2024
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
@@ -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
@@ -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
@@ -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;
@@ -486,20 +487,34 @@ public interface TreeArtifactVisitor {
      * Called for every directory entry encountered during tree traversal, in a nondeterministic
      * order.
      *
-     * <p>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.
+     * <p>Regular files and directories are reported as {@link Dirent.Type.FILE} or {@link
+     * Dirent.Type.DIRECTORY}, respectively. Directories are traversed recursively.
      *
-     * <p>{@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.
+     * <p>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.
      *
-     * <p>If the implementation throws {@link IOException}, traversal is immediately halted and the
+     * <p>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.
+     *
+     * <p>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.
+     *
+     * <p>If the visitor throws {@link IOException}, traversal is immediately halted and the
      * exception is propagated.
      *
      * <p>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. */
@@ -518,33 +533,62 @@ 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) {
         throw e.getCauseIOException();
       }
     }
 
-    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
@@ -1647,7 +1647,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
@@ -957,7 +957,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
@@ -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
@@ -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<Event> errors = ImmutableList.copyOf(eventCollector);
+    assertThat(errors).hasSize(2);
+    assertThat(errors.get(0).getMessage()).contains("Too many levels of symbolic links");
+    assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
+  }
+
   @Test
   public void validRelativeSymlinkAccepted() throws Exception {
     SpecialArtifact out = createTreeArtifact("output");
@@ -448,7 +503,9 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
 
     List<Event> errors = ImmutableList.copyOf(eventCollector);
     assertThat(errors).hasSize(2);
-    assertThat(errors.get(0).getMessage()).contains("Failed to resolve relative path links/link");
+    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");
   }
 
@@ -477,7 +534,9 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
 
     List<Event> errors = ImmutableList.copyOf(eventCollector);
     assertThat(errors).hasSize(2);
-    assertThat(errors.get(0).getMessage()).contains("Failed to resolve relative path links/link");
+    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");
   }
 
@@ -852,17 +911,18 @@ public void emptyInputAndOutputTreeArtifactInActionTemplate() throws Exception {
     assertThat(artifact2.getPath().getDirectoryEntries()).isEmpty();
   }
 
-  // This happens in the wild. See https://github.com/bazelbuild/bazel/issues/11813.
   @Test
-  public void treeArtifactContainsSymlinkToDirectory() throws Exception {
+  public void treeArtifactWithSymlinkToFile() throws Exception {
     SpecialArtifact treeArtifact = createTreeArtifact("tree");
     registerAction(
-        new SimpleTestAction(/*output=*/ treeArtifact) {
+        new SimpleTestAction(/* output= */ treeArtifact) {
           @Override
           void run(ActionExecutionContext context) throws IOException {
-            PathFragment subdir = PathFragment.create("subdir");
-            touchFile(treeArtifact.getPath().getRelative(subdir).getRelative("file"));
-            treeArtifact.getPath().getRelative("link").createSymbolicLink(subdir);
+            touchFile(treeArtifact.getPath().getRelative("subdir/file"));
+            treeArtifact
+                .getPath()
+                .getRelative("link")
+                .createSymbolicLink(PathFragment.create("subdir/file"));
           }
         });
 
@@ -874,6 +934,29 @@ void run(ActionExecutionContext context) throws IOException {
             TreeFileArtifact.createTreeOutput(treeArtifact, "link"));
   }
 
+  @Test
+  public void treeArtifactWithSymlinkToDirectory() throws Exception {
+    SpecialArtifact treeArtifact = createTreeArtifact("tree");
+    registerAction(
+        new SimpleTestAction(/* output= */ treeArtifact) {
+          @Override
+          void run(ActionExecutionContext context) throws IOException {
+            touchFile(treeArtifact.getPath().getRelative("subdir/file"));
+            treeArtifact
+                .getPath()
+                .getRelative("link")
+                .createSymbolicLink(PathFragment.create("subdir"));
+          }
+        });
+
+    TreeArtifactValue tree = buildArtifact(treeArtifact);
+
+    assertThat(tree.getChildren())
+        .containsExactly(
+            TreeFileArtifact.createTreeOutput(treeArtifact, "subdir/file"),
+            TreeFileArtifact.createTreeOutput(treeArtifact, "link/file"));
+  }
+
   private abstract static class SimpleTestAction extends TestAction {
     private final Button button;