Skip to content

Commit

Permalink
Don't ever claim /dev/null is an execpath.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
benjaminp authored and copybara-github committed Jan 6, 2021
1 parent 73011da commit ceec93c
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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() {
Expand All @@ -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
Expand All @@ -85,7 +81,7 @@ public ByteString getBytes() throws IOException {

@Override
public String toString() {
return "EmptyActionInput: " + execPath;
return "EmptyActionInput";
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -148,7 +146,7 @@ void addRunfilesToInputs(
addMapping(inputMap, location, localArtifact);
}
} else {
addMapping(inputMap, location, EMPTY_FILE);
addMapping(inputMap, location, VirtualActionInput.EMPTY_MARKER);
}
}
}
Expand Down Expand Up @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit ceec93c

Please sign in to comment.