optionally error on files outside of known roots

The goal of this change is to provide a means for disallowing
files outside of declared, known locations in order to better
enforce the hermeticy and in turn reproducability of builds.
Previously when encountering these files (typically via external
symlinks) we would add a dependency on build_id, now we can
optionally fail the build.

--
MOS_MIGRATED_REVID=90015665

6 files changed, 122 insertions, 36 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
index 6e47a2d54b..b5a87a84ef 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
@@ -39,12 +39,15 @@ public class DirectoryListingStateFunction implements SkyFunction {
   public SkyValue compute(SkyKey skyKey, Environment env)
       throws DirectoryListingStateFunctionException {
     RootedPath dirRootedPath = (RootedPath) skyKey.argument();
-    externalFilesHelper.maybeAddDepOnBuildId(dirRootedPath, env);
-    if (env.valuesMissing()) {
-      return null;
-    }
+
     try {
+      externalFilesHelper.maybeHandleExternalFile(dirRootedPath, env);
+      if (env.valuesMissing()) {
+        return null;
+      }
       return DirectoryListingStateValue.create(dirRootedPath);
+    } catch (FileOutsidePackageRootsException e) {
+      throw new DirectoryListingStateFunctionException(e);
     } catch (IOException e) {
       throw new DirectoryListingStateFunctionException(e);
     }
@@ -64,5 +67,9 @@ public class DirectoryListingStateFunction implements SkyFunction {
     public DirectoryListingStateFunctionException(IOException e) {
       super(e, Transience.TRANSIENT);
     }
+
+    public DirectoryListingStateFunctionException(FileOutsidePackageRootsException e) {
+      super(e, Transience.PERSISTENT);
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
index 10a6e65ebe..9363111e7b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
 import com.google.devtools.build.lib.vfs.Path;
@@ -27,14 +28,32 @@ class ExternalFilesHelper {
 
   private final AtomicReference<PathPackageLocator> pkgLocator;
   private final Set<Path> immutableDirs;
+  private final boolean errorOnExternalFiles;
 
+  @VisibleForTesting
   ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator) {
-    this(pkgLocator, ImmutableSet.<Path>of());
+    this(pkgLocator, ImmutableSet.<Path>of(), /*errorOnExternalFiles=*/false);
   }
 
-  ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator, Set<Path> immutableDirs) {
+  @VisibleForTesting
+  ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator,
+      boolean errorOnExternalFiles) {
+    this(pkgLocator, ImmutableSet.<Path>of(), errorOnExternalFiles);
+  }
+
+  /**
+   * @param pkgLocator an {@link AtomicReference} to a {@link PathPackageLocator} used to
+   *    determine what files are internal
+   * @param immutableDirs directories whose contents may be considered unchangeable
+   * @param errorOnExternalFiles whether or not to allow references to files outside of
+   *    the directories provided by pkgLocator or immutableDirs. See
+   *    {@link #maybeHandleExternalFile(RootedPath, SkyFunction.Environment)} for more details.
+   */
+  ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator, Set<Path> immutableDirs,
+      boolean errorOnExternalFiles) {
     this.pkgLocator = pkgLocator;
     this.immutableDirs = immutableDirs;
+    this.errorOnExternalFiles = errorOnExternalFiles;
   }
 
   private enum FileType {
@@ -69,29 +88,45 @@ class ExternalFilesHelper {
     return getFileType(rootedPath) == FileType.EXTERNAL_IMMUTABLE_FILE;
   }
 
-  public void maybeAddDepOnBuildId(RootedPath rootedPath, SkyFunction.Environment env) {
-   if (getFileType(rootedPath) == FileType.EXTERNAL_MUTABLE_FILE) {
-      // For files outside the package roots that are not assumed to be immutable, add a dependency
-      // on the build_id. This is sufficient for correctness; all other files will be handled by
-      // diff awareness of their respective package path, but these files need to be addressed
-      // separately.
-      //
-      // Using the build_id here seems to introduce a performance concern because the upward
-      // transitive closure of these external files will get eagerly invalidated on each
-      // incremental build (e.g. if every file had a transitive dependency on the filesystem root,
-      // then we'd have a big performance problem). But this a non-issue by design:
-      // - We don't add a dependency on the parent directory at the package root boundary, so the
-      // only transitive dependencies from files inside the package roots to external files are
-      // through symlinks. So the upwards transitive closure of external files is small.
-      // - The only way external source files get into the skyframe graph in the first place is
-      // through symlinks outside the package roots, which we neither want to encourage nor
-      // optimize for since it is not common. So the set of external files is small.
-      //
-      // The above reasoning doesn't hold for bazel, because external repositories
-      // (e.g. new_local_repository) cause lots of external symlinks to be present in the build.
-      // So bazel pretends that these external repositories are immutable to avoid the performance
-      // penalty described above.
-      PrecomputedValue.dependOnBuildId(env);
+  /**
+   * Potentially adds a dependency on build_id to env if this instance is configured
+   * with errorOnExternalFiles=false and rootedPath is an external mutable file.
+   * If errorOnExternalFiles=true and rootedPath is an external mutable file then
+   * a FileOutsidePackageRootsException is thrown. This method is a no-op for any
+   * rootedPaths that fall within the known package roots.
+   *
+   * @param rootedPath
+   * @param env
+   * @throws FileOutsidePackageRootsException
+   */
+  public void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env)
+      throws FileOutsidePackageRootsException {
+    if (getFileType(rootedPath) == FileType.EXTERNAL_MUTABLE_FILE) {
+      if (!errorOnExternalFiles) {
+        // For files outside the package roots that are not assumed to be immutable, add a
+        // dependency on the build_id. This is sufficient for correctness; all other files
+        // will be handled by diff awareness of their respective package path, but these
+        // files need to be addressed separately.
+        //
+        // Using the build_id here seems to introduce a performance concern because the upward
+        // transitive closure of these external files will get eagerly invalidated on each
+        // incremental build (e.g. if every file had a transitive dependency on the filesystem root,
+        // then we'd have a big performance problem). But this a non-issue by design:
+        // - We don't add a dependency on the parent directory at the package root boundary, so the
+        // only transitive dependencies from files inside the package roots to external files are
+        // through symlinks. So the upwards transitive closure of external files is small.
+        // - The only way external source files get into the skyframe graph in the first place is
+        // through symlinks outside the package roots, which we neither want to encourage nor
+        // optimize for since it is not common. So the set of external files is small.
+        //
+        // The above reasoning doesn't hold for bazel, because external repositories
+        // (e.g. new_local_repository) cause lots of external symlinks to be present in the build.
+        // So bazel pretends that these external repositories are immutable to avoid the performance
+        // penalty described above.
+        PrecomputedValue.dependOnBuildId(env);
+      } else {
+        throw new FileOutsidePackageRootsException(rootedPath);
+      }
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileOutsidePackageRootsException.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileOutsidePackageRootsException.java
new file mode 100644
index 0000000000..75f2e91b86
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileOutsidePackageRootsException.java
@@ -0,0 +1,34 @@
+// Copyright 2015 Google Inc. 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.skyframe;
+
+import com.google.devtools.build.lib.vfs.RootedPath;
+
+import java.io.IOException;
+
+/**
+ * Exception thrown to signal reference to file outside of known/allowed directory structures.
+ * Extends IOException to ensure it is properly handled by skyframe.
+ */
+// TODO(bazel-team): Don't piggyback on existing handling of IOExceptions and instead implement
+// the desired semantics.
+public class FileOutsidePackageRootsException extends IOException {
+
+  /**
+   * @param outsideRoots the {@link RootedPath} that triggered this exception.
+   */
+  public FileOutsidePackageRootsException(RootedPath outsideRoots) {
+    super("Encountered reference to external mutable " + outsideRoots);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
index ec2e871bbd..928306c1eb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
@@ -42,12 +42,15 @@ public class FileStateFunction implements SkyFunction {
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env) throws FileStateFunctionException {
     RootedPath rootedPath = (RootedPath) skyKey.argument();
-    externalFilesHelper.maybeAddDepOnBuildId(rootedPath, env);
-    if (env.valuesMissing()) {
-      return null;
-    }
+
     try {
+      externalFilesHelper.maybeHandleExternalFile(rootedPath, env);
+      if (env.valuesMissing()) {
+        return null;
+      }
       return FileStateValue.create(rootedPath, tsgm);
+    } catch (FileOutsidePackageRootsException e) {
+      throw new FileStateFunctionException(e);
     } catch (IOException e) {
       throw new FileStateFunctionException(e);
     } catch (InconsistentFilesystemException e) {
@@ -72,5 +75,9 @@ public class FileStateFunction implements SkyFunction {
     public FileStateFunctionException(InconsistentFilesystemException e) {
       super(e, Transience.TRANSIENT);
     }
+
+    public FileStateFunctionException(FileOutsidePackageRootsException e) {
+      super(e, Transience.PERSISTENT);
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index be3d190933..f8265c9101 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -101,7 +101,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
     super(reporter, evaluatorSupplier, pkgFactory, tsgm, directories,
         workspaceStatusActionFactory, buildInfoFactories, immutableDirectories,
         allowedMissingInputs, preprocessorFactorySupplier,
-        extraSkyFunctions, extraPrecomputedValues);
+        extraSkyFunctions, extraPrecomputedValues, /*errorOnExternalFiles=*/false);
     this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories, reporter);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 26728347b0..3fb2ee87a4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -217,6 +217,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   protected int outputDirtyFiles;
   protected int modifiedFilesDuringPreviousBuild;
   private final Predicate<PathFragment> allowedMissingInputs;
+  private final boolean errorOnExternalFiles;
 
   private final ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions;
   private final ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues;
@@ -245,7 +246,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       Predicate<PathFragment> allowedMissingInputs,
       Preprocessor.Factory.Supplier preprocessorFactorySupplier,
       ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
-      ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues) {
+      ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues,
+      boolean errorOnExternalFiles) {
     // Strictly speaking, these arguments are not required for initialization, but all current
     // callsites have them at hand, so we might as well set them during construction.
     this.reporter = Preconditions.checkNotNull(reporter);
@@ -269,6 +271,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     this.preprocessorFactorySupplier = preprocessorFactorySupplier;
     this.extraSkyFunctions = extraSkyFunctions;
     this.extraPrecomputedValues = extraPrecomputedValues;
+    this.errorOnExternalFiles = errorOnExternalFiles;
   }
 
   private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions(
@@ -276,7 +279,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       PackageFactory pkgFactory,
       Predicate<PathFragment> allowedMissingInputs) {
     ExternalFilesHelper externalFilesHelper = new ExternalFilesHelper(pkgLocator,
-        immutableDirectories);
+        immutableDirectories, errorOnExternalFiles);
     // We use an immutable map builder for the nice side effect that it throws if a duplicate key
     // is inserted.
     ImmutableMap.Builder<SkyFunctionName, SkyFunction> map = ImmutableMap.builder();