From f2cbdcf67ac1990f05a8241ba8dae65795edac82 Mon Sep 17 00:00:00 2001 From: aiuto Date: Tue, 12 Jan 2021 07:33:28 -0800 Subject: [PATCH] Don't ever claim /dev/null is an execpath. 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. Roll forward of ceec93c35ead1bd487e96a5fee46e8d080f88858, which was rolled back in 0f626a472b9fb6ca8a6da17448ba8ff518188870 RELNOTES: None. PiperOrigin-RevId: 351367453 --- .../lib/actions/cache/VirtualActionInput.java | 22 ++++++++----------- .../build/lib/exec/SpawnInputExpander.java | 14 +++++------- .../lib/exec/SpawnInputExpanderTest.java | 3 ++- .../remote/RemoteActionInputFetcherTest.java | 3 +-- 4 files changed, 18 insertions(+), 24 deletions(-) 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 index 81d4bb1d129032..8a035a969a390b 100644 --- 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 without 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 index 08dcda04b67bbe..ad4fe00e5b98ac 100644 --- 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 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 index f8a2d675f86dd9..d3cfa3c89a5fdf 100644 --- 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 index f4fc6770c8059d..ab3dcdc5c1101f 100644 --- 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();