Don't ever claim /dev/null is an execpath.

benjaminp · copybara-github · commit ceec93c35ead · 2021-01-06T14:26:35.000-08:00
SpawnInputExpander.EMPTY_FILE claimed an execpath of /dev/null, which could lead to clients trying to do mutating filesystem operations in /dev. In this change, SpawnInputExpander.EMPTY_FILE is replaced with an EmptyVirtualInput with no execpath. This way errant callers will explode before trying filesystem operations on it. Such a EmptyVirtualInput doesn't really meet the contract of being an ActionInput but that was really already true. Closes #12516. PiperOrigin-RevId: 350427786
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java b/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions.cache;
 
-import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.util.StreamWriter;
@@ -27,6 +26,11 @@
  * OutputStream.
  */
 public interface VirtualActionInput extends ActionInput, StreamWriter {
+  /**
+   * An empty virtual artifact <b>without</b> an execpath. This is used to denote empty files in
+   * runfiles and filesets.
+   */
+  public static final VirtualActionInput EMPTY_MARKER = new EmptyActionInput();
 
   /**
    * Gets a {@link ByteString} representation of the fake file. Used to avoid copying if the fake
@@ -48,15 +52,7 @@ default FileArtifactValue getMetadata() throws IOException {
    * use instances of this class to represent those files.
    */
   final class EmptyActionInput implements VirtualActionInput {
-    private final PathFragment execPath;
-
-    public EmptyActionInput(PathFragment execPath) {
-      this.execPath = Preconditions.checkNotNull(execPath);
-    }
-
-    public EmptyActionInput(String execPath) {
-      this(PathFragment.create(execPath));
-    }
+    private EmptyActionInput() {}
 
     @Override
     public boolean isSymlink() {
@@ -65,12 +61,12 @@ public boolean isSymlink() {
 
     @Override
     public String getExecPathString() {
-      return execPath.getPathString();
+      throw new UnsupportedOperationException("empty virtual artifact doesn't have an execpath");
     }
 
     @Override
     public PathFragment getExecPath() {
-      return execPath;
+      throw new UnsupportedOperationException("empty virtual artifact doesn't have an execpath");
     }
 
     @Override
@@ -85,7 +81,7 @@ public ByteString getBytes() throws IOException {
 
     @Override
     public String toString() {
-      return "EmptyActionInput: " + execPath;
+      return "EmptyActionInput";
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java
@@ -30,7 +30,7 @@
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
 import com.google.devtools.build.lib.actions.Spawn;
-import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput;
+import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.vfs.Path;
@@ -48,8 +48,6 @@
  * laid out.
  */
 public class SpawnInputExpander {
-  public static final ActionInput EMPTY_FILE = new EmptyActionInput("/dev/null");
-
   private final Path execRoot;
   private final boolean strict;
   private final RelativeSymlinkBehavior relSymlinkBehavior;
@@ -148,7 +146,7 @@ void addRunfilesToInputs(
             addMapping(inputMap, location, localArtifact);
           }
         } else {
-          addMapping(inputMap, location, EMPTY_FILE);
+          addMapping(inputMap, location, VirtualActionInput.EMPTY_MARKER);
         }
       }
     }
@@ -196,10 +194,10 @@ void addFilesetManifest(
 
       for (Map.Entry<PathFragment, String> mapping : filesetManifest.getEntries().entrySet()) {
         String value = mapping.getValue();
-        ActionInput artifact =
-            value == null
-                ? EMPTY_FILE
-                : ActionInputHelper.fromPath(execRoot.getRelative(value).getPathString());
+      ActionInput artifact =
+          value == null
+              ? VirtualActionInput.EMPTY_MARKER
+              : ActionInputHelper.fromPath(execRoot.getRelative(value).getPathString());
         addMapping(inputMappings, mapping.getKey(), artifact);
       }
   }
diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java
@@ -35,6 +35,7 @@
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
 import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl;
@@ -251,7 +252,7 @@ public void testRunfilesRootSymlink() throws Exception {
     // directory gets created.
     assertThat(inputMappings)
         .containsEntry(
-            PathFragment.create("runfiles/workspace/.runfile"), SpawnInputExpander.EMPTY_FILE);
+            PathFragment.create("runfiles/workspace/.runfile"), VirtualActionInput.EMPTY_MARKER);
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java
@@ -36,7 +36,6 @@
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.clock.JavaClock;
-import com.google.devtools.build.lib.exec.SpawnInputExpander;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.InMemoryCacheClient;
@@ -141,7 +140,7 @@ public void testStagingEmptyVirtualActionInput() throws Exception {
 
     // act
     actionInputFetcher.prefetchFiles(
-        ImmutableList.of(SpawnInputExpander.EMPTY_FILE), metadataProvider);
+        ImmutableList.of(VirtualActionInput.EMPTY_MARKER), metadataProvider);
 
     // assert that nothing happened
     assertThat(actionInputFetcher.downloadedFiles()).isEmpty();
