Separate Fileset-symlink errors out from I/O exceptions, and propagat…

…e them everywhere.

Because remaining I/O exceptions should all be environmental, made them 36 instead of 1.

Out of respect for ulfjack@, I left "strict" mode as a legitimate exception, even though it's completely dead in production.

PiperOrigin-RevId: 374474696

src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java

@@ -13,16 +13,12 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
-import static com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior.RESOLVE;
-import static com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior.RESOLVE_FULLY;
-
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior;
+import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehaviorWithoutError;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.io.IOException;
 import java.util.Map;
 
 /**
@@ -77,8 +73,7 @@ public static CompletionContext create(
       ActionInputMap inputMap,
       PathResolverFactory pathResolverFactory,
       Path execRoot,
-      String workspaceName)
-      throws IOException {
+      String workspaceName) {
     ArtifactPathResolver pathResolver =
         pathResolverFactory.shouldCreatePathResolverForArtifactValues()
             ? pathResolverFactory.createPathResolverForArtifactValues(
@@ -116,7 +111,12 @@ public void visitArtifacts(Iterable<Artifact> artifacts, ArtifactReceiver receiv
         continue;
       } else if (artifact.isFileset()) {
         if (expandFilesets) {
-          visitFileset(artifact, receiver, fullyResolveFilesetLinks ? RESOLVE_FULLY : RESOLVE);
+          visitFileset(
+              artifact,
+              receiver,
+              fullyResolveFilesetLinks
+                  ? RelativeSymlinkBehaviorWithoutError.RESOLVE_FULLY
+                  : RelativeSymlinkBehaviorWithoutError.RESOLVE);
         }
       } else if (artifact.isTreeArtifact()) {
         ImmutableCollection<? extends Artifact> expandedArtifacts =
@@ -133,17 +133,11 @@ public void visitArtifacts(Iterable<Artifact> artifacts, ArtifactReceiver receiv
   private void visitFileset(
       Artifact filesetArtifact,
       ArtifactReceiver receiver,
-      RelativeSymlinkBehavior relativeSymlinkBehavior) {
+      RelativeSymlinkBehaviorWithoutError relativeSymlinkBehavior) {
     ImmutableList<FilesetOutputSymlink> links = expandedFilesets.get(filesetArtifact);
-    FilesetManifest filesetManifest;
-    try {
-      filesetManifest =
-          FilesetManifest.constructFilesetManifest(
-              links, PathFragment.EMPTY_FRAGMENT, relativeSymlinkBehavior);
-    } catch (IOException e) {
-      // Unexpected: RelativeSymlinkBehavior.RESOLVE should not throw.
-      throw new IllegalStateException(e);
-    }
+    FilesetManifest filesetManifest =
+        FilesetManifest.constructFilesetManifestWithoutError(
+            links, PathFragment.EMPTY_FRAGMENT, relativeSymlinkBehavior);
 
     for (Map.Entry<PathFragment, String> mapping : filesetManifest.getEntries().entrySet()) {
       String targetFile = mapping.getValue();
@@ -165,8 +159,7 @@ ArtifactPathResolver createPathResolverForArtifactValues(
         ActionInputMap actionInputMap,
         Map<Artifact, ImmutableCollection<? extends Artifact>> expandedArtifacts,
         Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesets,
-        String workspaceName)
-        throws IOException;
+        String workspaceName);
 
     boolean shouldCreatePathResolverForArtifactValues();
   }

src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java

@@ -16,7 +16,6 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.flogger.GoogleLogger;
-import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -38,9 +37,7 @@ public final class FilesetManifest {
   private static final int MAX_SYMLINK_TRAVERSALS = 256;
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
-  /**
-   * Mode that determines how to handle relative target paths.
-   */
+  /** Mode that determines how to handle relative target paths. */
   public enum RelativeSymlinkBehavior {
     /** Ignore any relative target paths. */
     IGNORE,
@@ -52,14 +49,58 @@ public enum RelativeSymlinkBehavior {
     RESOLVE,
 
     /** Fully resolve all relative paths, even those pointing to internal directories. */
-    RESOLVE_FULLY;
+    RESOLVE_FULLY
+  }
+
+  /**
+   * Shadow of {@link RelativeSymlinkBehavior} without the {@link RelativeSymlinkBehavior#ERROR}
+   * value for callers who know there won't be an error thrown when constructing the manifest.
+   */
+  public enum RelativeSymlinkBehaviorWithoutError {
+    /** Ignore any relative target paths. */
+    IGNORE(RelativeSymlinkBehavior.IGNORE),
+
+    /** Resolve all relative target paths. */
+    RESOLVE(RelativeSymlinkBehavior.RESOLVE),
+
+    /** Fully resolve all relative paths, even those pointing to internal directories. */
+    RESOLVE_FULLY(RelativeSymlinkBehavior.RESOLVE_FULLY);
+
+    private final RelativeSymlinkBehavior target;
+
+    RelativeSymlinkBehaviorWithoutError(RelativeSymlinkBehavior target) {
+      this.target = target;
+    }
+  }
+
+  /**
+   * Constructs a FilesetManifest from the given {@code outputSymlinks}, processing relative
+   * symlinks according to {@code relSymlinkBehavior}. Use when {@link
+   * RelativeSymlinkBehavior#ERROR} is guaranteed not to be the behavior.
+   */
+  public static FilesetManifest constructFilesetManifestWithoutError(
+      List<FilesetOutputSymlink> outputSymlinks,
+      PathFragment targetPrefix,
+      RelativeSymlinkBehaviorWithoutError relSymlinkBehavior) {
+    try {
+      return constructFilesetManifest(outputSymlinks, targetPrefix, relSymlinkBehavior.target);
+    } catch (ForbiddenRelativeSymlinkException e) {
+      throw new IllegalStateException(
+          "Can't throw forbidden symlink exception unless behavior is ERROR: "
+              + relSymlinkBehavior
+              + ", "
+              + targetPrefix
+              + ", "
+              + outputSymlinks,
+          e);
+    }
   }
 
   public static FilesetManifest constructFilesetManifest(
       List<FilesetOutputSymlink> outputSymlinks,
       PathFragment targetPrefix,
       RelativeSymlinkBehavior relSymlinkBehavior)
-      throws IOException {
+      throws ForbiddenRelativeSymlinkException {
     LinkedHashMap<PathFragment, String> entries = new LinkedHashMap<>();
     Map<PathFragment, String> relativeLinks = new HashMap<>();
     Map<String, FileArtifactValue> artifactValues = new HashMap<>();
@@ -91,12 +132,10 @@ private static void addRelativeSymlinkEntry(
       PathFragment fullLocation,
       RelativeSymlinkBehavior relSymlinkBehavior,
       Map<PathFragment, String> relativeLinks)
-      throws IOException {
+      throws ForbiddenRelativeSymlinkException {
     switch (relSymlinkBehavior) {
       case ERROR:
-        IOException ioException = new IOException("runfiles target is not absolute: " + artifact);
-        BugReport.sendBugReport(ioException);
-        throw ioException;
+        throw new ForbiddenRelativeSymlinkException(artifact);
       case RESOLVE:
       case RESOLVE_FULLY:
         if (!relativeLinks.containsKey(fullLocation)) { // Keep consistent behavior: no overwriting.
@@ -110,8 +149,9 @@ private static void addRelativeSymlinkEntry(
 
   /** Fully resolve relative symlinks including internal directory symlinks. */
   private static void fullyResolveRelativeSymlinks(
-      Map<PathFragment, String> entries, Map<PathFragment, String> relativeLinks, boolean absolute)
-      throws IOException {
+      Map<PathFragment, String> entries,
+      Map<PathFragment, String> relativeLinks,
+      boolean absolute) {
     try {
       // Construct an in-memory Filesystem containing all the non-relative-symlink entries in the
       // Fileset. Treat these as regular files in the filesystem whose contents are the "real"
@@ -141,15 +181,7 @@ private static void fullyResolveRelativeSymlinks(
 
       addSymlinks(root, entries, absolute);
     } catch (IOException e) {
-      // TODO(janakr): make this crash hard if there are no bug reports.
-      BugReport.sendBugReport(
-          new IllegalStateException(
-              "Unexpected IOException from InMemoryFileSystem operations for "
-                  + entries
-                  + " with "
-                  + relativeLinks,
-              e));
-      throw e;
+      throw new IllegalStateException("InMemoryFileSystem can't throw", e);
     }
   }
 
@@ -181,8 +213,7 @@ private static void resolveRelativeSymlinks(
       Map<PathFragment, String> entries,
       Map<PathFragment, String> relativeLinks,
       boolean absolute,
-      RelativeSymlinkBehavior relSymlinkBehavior)
-      throws IOException {
+      RelativeSymlinkBehavior relSymlinkBehavior) {
     if (relSymlinkBehavior == RelativeSymlinkBehavior.RESOLVE_FULLY && !relativeLinks.isEmpty()) {
       fullyResolveRelativeSymlinks(entries, relativeLinks, absolute);
     } else if (relSymlinkBehavior == RelativeSymlinkBehavior.RESOLVE) {
@@ -258,4 +289,12 @@ public Map<PathFragment, String> getEntries() {
   public Map<String, FileArtifactValue> getArtifactValues() {
     return artifactValues;
   }
+
+  /** Exception indicating that a relative symlink was encountered but not permitted. */
+  public static final class ForbiddenRelativeSymlinkException
+      extends ForbiddenActionInputException {
+    private ForbiddenRelativeSymlinkException(String symlinkTarget) {
+      super("Fileset symlink " + symlinkTarget + " is not absolute");
+    }
+  }
 }

src/main/java/com/google/devtools/build/lib/actions/ForbiddenActionInputException.java

@@ -0,0 +1,26 @@
+// Copyright 2021 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.actions;
+
+/**
+ * Ancestor for exceptions thrown because of a bad input: an input that is not allowed for the given
+ * spawn/execution platform (like a relative symlink in a Fileset or a directory for a platform that
+ * does not support directory inputs). Indicates a user error, rather than an I/O error.
+ */
+public class ForbiddenActionInputException extends Exception {
+  protected ForbiddenActionInputException(String message) {
+    super(message);
+  }
+}

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

@@ -27,7 +27,7 @@
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.CommandLineItem;
 import com.google.devtools.build.lib.actions.FilesetManifest;
-import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior;
+import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehaviorWithoutError;
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -39,7 +39,6 @@
 import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.IllegalFormatException;
@@ -354,15 +353,11 @@ private static void expandFileset(
                 fileset),
             e);
       }
-      try {
-        FilesetManifest filesetManifest =
-            FilesetManifest.constructFilesetManifest(
-                expandedFileSet, fileset.getExecPath(), RelativeSymlinkBehavior.IGNORE);
-        for (PathFragment relativePath : filesetManifest.getEntries().keySet()) {
-          expandedValues.add(new FilesetSymlinkFile(fileset, relativePath));
-        }
-      } catch (IOException e) {
-        throw new CommandLineExpansionException("Could not expand fileset: " + e.getMessage());
+      FilesetManifest filesetManifest =
+          FilesetManifest.constructFilesetManifestWithoutError(
+              expandedFileSet, fileset.getExecPath(), RelativeSymlinkBehaviorWithoutError.IGNORE);
+      for (PathFragment relativePath : filesetManifest.getEntries().keySet()) {
+        expandedValues.add(new FilesetSymlinkFile(fileset, relativePath));
       }
     }
 

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
 import com.google.devtools.build.lib.actions.LostInputsActionExecutionException;
 import com.google.devtools.build.lib.actions.LostInputsExecException;
 import com.google.devtools.build.lib.actions.MetadataProvider;
@@ -36,6 +37,7 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
 import com.google.devtools.build.lib.actions.Spawns;
+import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
@@ -164,6 +166,13 @@ public ImmutableList<SpawnResult> exec(
       ex = e;
       spawnResult = e.getSpawnResult();
       // Log the Spawn and re-throw.
+    } catch (ForbiddenActionInputException e) {
+      throw new UserExecException(
+          e,
+          FailureDetail.newBuilder()
+              .setMessage("Exec failed due to forbidden input")
+              .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.FORBIDDEN_INPUT))
+              .build());
     }
 
     SpawnLogContext spawnLogContext = actionExecutionContext.getContext(SpawnLogContext.class);
@@ -175,7 +184,7 @@ public ImmutableList<SpawnResult> exec(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
             context.getTimeout(),
             spawnResult);
-      } catch (IOException e) {
+      } catch (IOException | ForbiddenActionInputException e) {
         actionExecutionContext
             .getEventHandler()
             .handle(
@@ -232,7 +241,8 @@ public int getId() {
     }
 
     @Override
-    public void prefetchInputs() throws IOException, InterruptedException {
+    public void prefetchInputs()
+        throws IOException, ForbiddenActionInputException, InterruptedException {
       if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) {
         actionExecutionContext
             .getActionInputPrefetcher()
@@ -290,7 +300,7 @@ public FileOutErr getFileOutErr() {
 
     @Override
     public SortedMap<PathFragment, ActionInput> getInputMapping(PathFragment baseDirectory)
-        throws IOException {
+        throws IOException, ForbiddenActionInputException {
       if (lazyInputMapping == null || !inputMappingBaseDirectory.equals(baseDirectory)) {
         try (SilentCloseable c =
             Profiler.instance().profile("AbstractSpawnStrategy.getInputMapping")) {

src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java

@@ -15,6 +15,7 @@
 
 import com.google.devtools.build.lib.actions.ActionContext;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
@@ -164,7 +165,7 @@ interface CacheHandle extends Closeable {
    * instance's {@link CacheHandle#store} is a no-op.
    */
   CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
-      throws ExecException, IOException, InterruptedException;
+      throws ExecException, IOException, InterruptedException, ForbiddenActionInputException;
 
   /**
    * Returns whether this cache implementation makes sense to use together with dynamic execution.