Do not force unresolved symlinks to be absolute

The documentation of `ctx.actions.symlink(target_path = ...)` states
that the given path is used as the symlink target without any changes,
but in reality this path has so far always been converted into an
absolute path by prepending the exec root. This is changed by this
commit, which gets very close to the documented behavior by preserving
the target path as is except for the standard normalization applied by
`PathFragment`. Improving the situation even further would require
modifying or adding to Bazel's core file system API, which may be done
in a follow-up PR.

Since relative symlinks only resolve correctly at the location they were
originally created, the logic for staging symlinks as runfiles in the
sandbox necessarily becomes more complicated: Analogously to the case of
local unsandboxed execution, symlinks in runfiles are now staged as
symlinks to the original exec path of the relative symlink under the
sandboxed exec root as well as the relative symlink itself in that
location.

As a side-effect of the switch to relative symlinks, this commit
resolves a non-hermeticity issue observed in
bazelbuild#10298 (comment)
Integration tests are added to verify that symlinks staged in the
sandbox are no longer resolved non-hermetically, including the case
where they are contained in runfiles.

Fixes bazelbuild#14224

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -249,6 +249,7 @@ java_library(
         "starlark/StarlarkRuleContext.java",
         "starlark/StarlarkRuleTransitionProvider.java",
         "starlark/StarlarkTransition.java",
+        "starlark/UnresolvedSymlinkAction.java",
         "test/AnalysisTestActionBuilder.java",
         "test/BaselineCoverageAction.java",
         "test/CoverageCommon.java",

src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java

@@ -36,7 +36,10 @@
 import java.io.IOException;
 import javax.annotation.Nullable;
 
-/** Action to create a symbolic link. */
+/**
+ * Action to create a symlink to a known-to-exist target with alias semantics similar to a true copy
+ * of the input (if any).
+ */
 public final class SymlinkAction extends AbstractAction {
   private static final String GUID = "7f4fab4d-d0a7-4f0f-8649-1d0337a21fee";
 
@@ -152,13 +155,6 @@ public static SymlinkAction toFileset(
         owner, execPath, primaryInput, primaryOutput, progressMessage, TargetType.FILESET);
   }
 
-  public static SymlinkAction createUnresolved(
-      ActionOwner owner, Artifact primaryOutput, PathFragment targetPath, String progressMessage) {
-    Preconditions.checkArgument(primaryOutput.isSymlink());
-    return new SymlinkAction(
-        owner, targetPath, null, primaryOutput, progressMessage, TargetType.OTHER);
-  }
-
   /**
    * Creates a new SymlinkAction instance, where an input artifact is not present. This is useful
    * when dealing with special cases where input paths that are outside the exec root directory

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java

@@ -273,7 +273,7 @@ public void symlink(
             ? (String) progressMessageUnchecked
             : "Creating symlink " + outputArtifact.getExecPathString();
 
-    SymlinkAction action;
+    Action action;
     if (targetFile != Starlark.NONE) {
       Artifact inputArtifact = (Artifact) targetFile;
       if (outputArtifact.isSymlink()) {
@@ -324,10 +324,10 @@ public void symlink(
       }
 
       action =
-          SymlinkAction.createUnresolved(
+          new UnresolvedSymlinkAction(
               ruleContext.getActionOwner(),
               outputArtifact,
-              PathFragment.create((String) targetPath),
+              (String) targetPath,
               progressMessage);
     }
     registerAction(action);

src/main/java/com/google/devtools/build/lib/analysis/starlark/UnresolvedSymlinkAction.java

@@ -0,0 +1,112 @@
+// Copyright 2022 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.analysis.starlark;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.actions.AbstractAction;
+import com.google.devtools.build.lib.actions.ActionExecutionContext;
+import com.google.devtools.build.lib.actions.ActionExecutionException;
+import com.google.devtools.build.lib.actions.ActionKeyContext;
+import com.google.devtools.build.lib.actions.ActionOwner;
+import com.google.devtools.build.lib.actions.ActionResult;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.server.FailureDetails;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.server.FailureDetails.SymlinkAction.Code;
+import com.google.devtools.build.lib.util.DetailedExitCode;
+import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.io.IOException;
+import javax.annotation.Nullable;
+
+/**
+ * Action to create a possibly unresolved symbolic link to a raw path.
+ *
+ * To create a symlink to a known-to-exist target with alias semantics similar to a true copy of the
+ * input, use {@link SymlinkAction} instead.
+ */
+public final class UnresolvedSymlinkAction extends AbstractAction {
+  private static final String GUID = "0f302651-602c-404b-881c-58913193cfe7";
+
+  private final String target;
+  private final String progressMessage;
+
+  public UnresolvedSymlinkAction(
+      ActionOwner owner,
+      Artifact primaryOutput,
+      String target,
+      String progressMessage) {
+    super(
+        owner,
+        NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+        ImmutableSet.of(primaryOutput));
+    this.target = target;
+    this.progressMessage = progressMessage;
+  }
+
+  @Override
+  public ActionResult execute(ActionExecutionContext actionExecutionContext)
+      throws ActionExecutionException {
+
+    Path outputPath = actionExecutionContext.getInputPath(getPrimaryOutput());
+    try {
+      // TODO: PathFragment#create normalizes the symlink target, which may change how it resolves
+      //  when combined with directory symlinks. Ideally, Bazel's file system abstraction would
+      //  offer a way to create symlinks without any preprocessing of the target.
+      outputPath.createSymbolicLink(PathFragment.create(target));
+    } catch (IOException e) {
+      String message =
+          String.format(
+              "failed to create symbolic link '%s' to '%s' due to I/O error: %s",
+              getPrimaryOutput().getExecPathString(), target, e.getMessage());
+      DetailedExitCode code = createDetailedExitCode(message, Code.LINK_CREATION_IO_EXCEPTION);
+      throw new ActionExecutionException(message, e, this, false, code);
+    }
+
+    return ActionResult.EMPTY;
+  }
+
+  @Override
+  protected void computeKey(
+      ActionKeyContext actionKeyContext,
+      @Nullable ArtifactExpander artifactExpander,
+      Fingerprint fp) {
+    fp.addString(GUID);
+    fp.addString(target);
+  }
+
+  @Override
+  public String getMnemonic() {
+    return "UnresolvedSymlink";
+  }
+
+  @Override
+  protected String getRawProgressMessage() {
+    return progressMessage;
+  }
+
+  private static DetailedExitCode createDetailedExitCode(String message, Code detailedCode) {
+    return DetailedExitCode.of(
+        FailureDetail.newBuilder()
+            .setMessage(message)
+            .setSymlinkAction(FailureDetails.SymlinkAction.newBuilder().setCode(detailedCode))
+            .build());
+  }
+}

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -21,6 +21,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -548,8 +549,7 @@ public SandboxInputs processInputFiles(Map<PathFragment, ActionInput> inputMap,
             virtualInputsWithDelayedMaterialization.add((VirtualActionInput) actionInput);
           }
         } else if (actionInput.isSymlink()) {
-          Path inputPath = execRoot.getRelative(actionInput.getExecPath());
-          inputSymlinks.put(pathFragment, inputPath.readSymbolicLink());
+          stageSymlink(inputSymlinks, execRoot, actionInput, pathFragment);
         } else {
           Path inputPath = execRoot.getRelative(actionInput.getExecPath());
           inputFiles.put(pathFragment, inputPath);
@@ -563,8 +563,7 @@ public SandboxInputs processInputFiles(Map<PathFragment, ActionInput> inputMap,
         }
 
         if (actionInput.isSymlink()) {
-          Path inputPath = execRoot.getRelative(actionInput.getExecPath());
-          inputSymlinks.put(pathFragment, inputPath.readSymbolicLink());
+          stageSymlink(inputSymlinks, execRoot, actionInput, pathFragment);
         } else {
           Path inputPath =
               actionInput instanceof EmptyActionInput
@@ -581,6 +580,26 @@ public SandboxInputs processInputFiles(Map<PathFragment, ActionInput> inputMap,
         inputSymlinks);
   }
 
+  private void stageSymlink(Map<PathFragment, PathFragment> inputSymlinks, Path execRoot,
+      ActionInput symlink, PathFragment location) throws IOException {
+    // Symlinks may be relative and thus only resolve correctly from their original location under
+    // the exec root. Thus, always stage symlinks at their original location.
+    Path inputPath = execRoot.getRelative(symlink.getExecPath());
+    inputSymlinks.put(symlink.getExecPath(), inputPath.readSymbolicLink());
+    if (location.equals(symlink.getExecPath())) {
+      return;
+    }
+    // If the symlink has to be staged at a different location (e.g., in a runfiles tree),
+    // additionally emit a relative symlink pointing to the actual symlink. This closely mimics how
+    // the symlink would be staged for local execution.
+    // Note: It may seem simpler to use a symlink to the absolute path to the original symlink under
+    // the unsandboxed exec root, but that may result in the symlink resolving non-hermetically to
+    // undeclared input files.
+    int locationDepth = location.getParentDirectory().segmentCount();
+    PathFragment backToExecRoot = PathFragment.create(Strings.repeat("../", locationDepth));
+    inputSymlinks.put(location, backToExecRoot.getRelative(symlink.getExecPath()));
+  }
+
   /** The file and directory outputs of a sandboxed spawn. */
   @AutoValue
   public abstract static class SandboxOutputs {

src/test/shell/bazel/bazel_symlink_test.sh

@@ -411,11 +411,11 @@ EOF
   cat > a/BUILD <<'EOF'
 load(":a.bzl", "a")
 
-a(name="a", link_target="/somewhere/in/my/heart")
+a(name="a", link_target="../somewhere/in/my/heart")
 EOF
 
   bazel build --experimental_allow_unresolved_symlinks //a:a || fail "build failed"
-  assert_contains "input link is /somewhere/in/my/heart" bazel-bin/a/a.file
+  assert_contains "input link is ../somewhere/in/my/heart" bazel-bin/a/a.file
 }
 
 function test_symlink_file_to_file_created_from_symlink_action() {
@@ -731,4 +731,126 @@ EOF
   bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:exec || fail "build failed"
 }
 
+function test_unresolved_symlink_hermetic_in_sandbox() {
+  if "$is_windows"; then
+    # TODO(#10298): Support unresolved symlinks on Windows.
+    return 0
+  fi
+
+  mkdir -p pkg
+  cat > pkg/def.bzl <<'EOF'
+def _r(ctx):
+  symlink = ctx.actions.declare_symlink(ctx.label.name + "_s")
+  ctx.actions.symlink(output=symlink, target_path=ctx.file.file.basename)
+
+  output = ctx.actions.declare_file(ctx.label.name)
+  ctx.actions.run_shell(
+    command = "{ cat %s || true; } >  %s" % (symlink.path, output.path),
+    inputs = [symlink] + ([ctx.file.file] if ctx.attr.stage_target else []),
+    outputs = [output],
+  )
+  return DefaultInfo(files=depset([output]))
+
+r = rule(
+    implementation=_r,
+    attrs = {
+        "file": attr.label(allow_single_file=True),
+        "stage_target": attr.bool(),
+    }
+)
+EOF
+  cat > pkg/BUILD <<'EOF'
+load(":def.bzl", "r")
+
+genrule(name = "a", outs = ["file"], cmd = "echo hello >$@")
+r(name="b", file="file")
+r(name="c", file="file", stage_target=True)
+EOF
+
+  bazel build //pkg:b --experimental_allow_unresolved_symlinks --spawn_strategy=local
+  [ -s bazel-bin/pkg/b ] && fail "symlink should not resolve"
+
+  bazel clean
+  bazel build //pkg:a --spawn_strategy=sandboxed
+  bazel build //pkg:b --experimental_allow_unresolved_symlinks --spawn_strategy=local
+  [ -s bazel-bin/pkg/b ] || fail "symlink should resolve"
+
+  bazel clean
+  bazel build //pkg:c --experimental_allow_unresolved_symlinks --spawn_strategy=local
+  [ -s bazel-bin/pkg/c ] || fail "symlink should resolve"
+
+  bazel clean
+  bazel build //pkg:b --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed
+  [ -s bazel-bin/pkg/b ] && fail "sandboxed build is not hermetic"
+
+  bazel clean
+  bazel build //pkg:a --spawn_strategy=sandboxed
+  bazel build //pkg:b --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed
+  [ -s bazel-bin/pkg/b ] && fail "sandboxed build is not hermetic"
+
+  bazel clean
+  bazel build //pkg:c --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed
+  [ -s bazel-bin/pkg/c ] || fail "symlink should resolve"
+}
+
+
+function test_unresolved_symlink_in_runfiles() {
+  if "$is_windows"; then
+    # TODO(#10298): Support unresolved symlinks on Windows.
+    return 0
+  fi
+
+  mkdir -p pkg
+  cat > pkg/def.bzl <<'EOF'
+def _r(ctx):
+  symlink = ctx.actions.declare_symlink(ctx.label.name + "_s")
+  target = ctx.file.file.basename
+  ctx.actions.symlink(output=symlink, target_path=target)
+
+  script = ctx.actions.declare_file(ctx.label.name)
+  content = """
+#!/usr/bin/env bash
+cd $0.runfiles/{workspace_name}
+[ -L {symlink} ] || {{ echo "runfile is not a symlink"; exit 1; }}
+[ $(cd $(dirname {symlink}) && readlink $(readlink $(basename {symlink}))) == "{target}" ] || {{ echo "runfile symlink does not point to a symlink pointing to the expected target"; exit 1; }}
+[ -s {symlink} ] || {{ echo "runfile not resolved"; exit 1; }}
+""".format(
+    symlink = symlink.short_path,
+    target = target,
+    workspace_name = ctx.workspace_name,
+  )
+  ctx.actions.write(script, content, is_executable=True)
+  return DefaultInfo(executable=script, runfiles=ctx.runfiles(files = [symlink]))
+
+r = rule(implementation=_r, attrs={"file":attr.label(allow_single_file=True)}, executable = True)
+EOF
+  cat > pkg/BUILD <<'EOF'
+load(":def.bzl", "r")
+
+genrule(name="a", outs=["file"], cmd="echo hello >$@")
+r(name="tool", file="file")
+genrule(
+    name = "use_tool",
+    outs = ["out"],
+    cmd = "$(location :tool) && touch $@",
+    tools = [":tool", "file"],
+)
+genrule(
+    name = "use_tool_non_hermetically",
+    outs = ["out_non_hermetic"],
+    cmd = "$(location :tool) && touch $@",
+    tools = [":tool"],
+)
+EOF
+
+  bazel build --experimental_allow_unresolved_symlinks --spawn_strategy=local //pkg:use_tool || fail "local build failed"
+  # Keep the implicitly built //pkg:a around to make the symlink resolve.
+  bazel build --experimental_allow_unresolved_symlinks --spawn_strategy=local //pkg:use_tool_non_hermetically || fail "local build failed"
+
+  bazel clean
+  bazel build --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed //pkg:use_tool || fail "sandboxed build failed"
+  # Keep the implicitly built //pkg:a around to make the symlink resolve outside the sandbox.
+  bazel build --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed //pkg:use_tool_non_hermetically && fail "sandboxed build is not hermetic" || true
+}
+
 run_suite "Tests for symlink artifacts"