Allow using embedded tools in sandboxed spawn runners.

When using sandboxed runners, the only supported `VirtualActionInput` types are
empty inputs and param files. Add support for `BinTools.PathActionInput` so
that we can we can use embedded tools in sandboxed runners. Make sure to write
them as executable to the sandbox -- contrary to param files, embedded tools
can be run.

PiperOrigin-RevId: 344438370

Author: alexjski

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -15,12 +15,10 @@
 package com.google.devtools.build.lib.sandbox;
 
 import com.google.auto.value.AutoValue;
-import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
-import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput;
@@ -85,9 +83,7 @@ public static void atomicallyWriteVirtualInput(
     Path tmpPath = outputPath.getFileSystem().getPath(outputPath.getPathString() + uniqueSuffix);
     tmpPath.getParentDirectory().createDirectoryAndParents();
     try {
-      try (OutputStream outputStream = tmpPath.getOutputStream()) {
-        input.writeTo(outputStream);
-      }
+      writeVirtualInputTo(input, tmpPath);
       // We expect the following to replace the params file atomically in case we are using
       // the dynamic scheduler and we are racing the remote strategy writing this same file.
       tmpPath.renameTo(outputPath);
@@ -130,36 +126,34 @@ public Map<PathFragment, PathFragment> getSymlinks() {
     /**
      * Materializes a single virtual input inside the given execroot.
      *
+     * <p>When materializing inputs under a new sandbox exec root, we can expect the input to not
+     * exist, but we cannot make the same assumption for the non-sandboxed exec root therefore, we
+     * may need to delete existing files.
+     *
      * @param input virtual input to materialize
      * @param execroot path to the execroot under which to materialize the virtual input
-     * @param needsDelete whether to attempt to delete a previous instance of this virtual input.
-     *     When materializing under a new sandbox execroot, we can expect the input to not exist,
-     *     but we cannot make the same assumption for the non-sandboxed execroot.
+     * @param isExecRootSandboxed whether the execroot is sandboxed.
      * @throws IOException if the virtual input cannot be materialized
      */
     private static void materializeVirtualInput(
-        VirtualActionInput input, Path execroot, boolean needsDelete) throws IOException {
-      if (input instanceof ParamFileActionInput) {
-        ParamFileActionInput paramFileInput = (ParamFileActionInput) input;
-        Path outputPath = execroot.getRelative(paramFileInput.getExecPath());
-        if (needsDelete) {
-          if (outputPath.exists()) {
-            outputPath.delete();
-          }
-
-          outputPath.getParentDirectory().createDirectoryAndParents();
-          try (OutputStream outputStream = outputPath.getOutputStream()) {
-            paramFileInput.writeTo(outputStream);
-          }
-        } else {
-          atomicallyWriteVirtualInput(paramFileInput, outputPath, ".sandbox");
-        }
-      } else {
+        VirtualActionInput input, Path execroot, boolean isExecRootSandboxed) throws IOException {
+      if (input instanceof EmptyActionInput) {
         // TODO(b/150963503): We can turn this into an unreachable code path when the old
-        // !delayVirtualInputMaterialization code path is deleted.
-        // TODO(ulfjack): Handle all virtual inputs, e.g., by writing them to a file.
-        Preconditions.checkState(input instanceof EmptyActionInput);
+        //  !delayVirtualInputMaterialization code path is deleted.
+        return;
       }
+
+      Path outputPath = execroot.getRelative(input.getExecPath());
+      if (isExecRootSandboxed) {
+        atomicallyWriteVirtualInput(input, outputPath, ".sandbox");
+        return;
+      }
+
+      if (outputPath.exists()) {
+        outputPath.delete();
+      }
+      outputPath.getParentDirectory().createDirectoryAndParents();
+      writeVirtualInputTo(input, outputPath);
     }
 
     /**
@@ -179,6 +173,18 @@ public void materializeVirtualInputs(Path sandboxExecRoot) throws IOException {
     }
   }
 
+  private static void writeVirtualInputTo(VirtualActionInput input, Path target)
+      throws IOException {
+    try (OutputStream out = target.getOutputStream()) {
+      input.writeTo(out);
+    }
+    // Some of the virtual inputs can be executed, e.g. embedded tools. Setting executable flag for
+    // other is fine since that is only more permissive. Please note that for action outputs (e.g.
+    // file write, where the user can specify executable flag), we will have artifacts which do not
+    // go through this code path.
+    target.setExecutable(true);
+  }
+
   /**
    * Returns the inputs of a Spawn as a map of PathFragments relative to an execRoot to paths in the
    * host filesystem where the input files can be found.

src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java

@@ -125,7 +125,7 @@ public void testStagingVirtualActionInput() throws Exception {
     // assert
     Path p = execRoot.getRelative(a.getExecPath());
     assertThat(FileSystemUtils.readContent(p, StandardCharsets.UTF_8)).isEqualTo("hello world");
-    assertThat(p.isExecutable()).isFalse();
+    assertThat(p.isExecutable()).isTrue();
     assertThat(actionInputFetcher.downloadedFiles()).isEmpty();
     assertThat(actionInputFetcher.downloadsInProgress()).isEmpty();
   }

src/test/java/com/google/devtools/build/lib/sandbox/BUILD

@@ -54,11 +54,17 @@ java_test(
     deps = [
         ":sandboxfs-base-tests",
         ":testutil",
+        "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
+        "//src/main/java/com/google/devtools/build/lib/exec:bin_tools",
         "//src/main/java/com/google/devtools/build/lib/sandbox",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
         "//src/main/java/com/google/devtools/common/options",
+        "//src/test/java/com/google/devtools/build/lib/actions/util",
+        "//src/test/java/com/google/devtools/build/lib/exec/util",
+        "//src/test/java/com/google/devtools/build/lib/testutil",
         "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
         "//third_party:guava",
         "//third_party:junit4",

src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java

@@ -0,0 +1,204 @@
+// Copyright 2020 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.sandbox;
+
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.truth.Truth.assertThat;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
+import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
+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.exec.BinTools;
+import com.google.devtools.build.lib.exec.util.SpawnBuilder;
+import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
+import com.google.devtools.build.lib.testutil.Scratch;
+import com.google.devtools.build.lib.vfs.Dirent;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Symlinks;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.function.Function;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link SandboxHelpers}. */
+@RunWith(JUnit4.class)
+public class SandboxHelpersTest {
+
+  private static final ArtifactExpander EMPTY_EXPANDER = (ignored1, ignored2) -> {};
+  private static final Spawn SPAWN = new SpawnBuilder().build();
+
+  private final Scratch scratch = new Scratch();
+  private Path execRoot;
+
+  @Before
+  public void createExecRoot() throws IOException {
+    execRoot = scratch.dir("/execRoot");
+  }
+
+  @Test
+  public void processInputFiles_materializesParamFile() throws Exception {
+    SandboxHelpers sandboxHelpers = new SandboxHelpers(/*delayVirtualInputMaterialization=*/ false);
+    ParamFileActionInput paramFile =
+        new ParamFileActionInput(
+            PathFragment.create("paramFile"),
+            ImmutableList.of("-a", "-b"),
+            ParameterFileType.UNQUOTED,
+            UTF_8);
+
+    SandboxInputs inputs =
+        sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot);
+
+    assertThat(inputs.getFiles())
+        .containsExactly(PathFragment.create("paramFile"), execRoot.getChild("paramFile"));
+    assertThat(inputs.getSymlinks()).isEmpty();
+    assertThat(FileSystemUtils.readLines(execRoot.getChild("paramFile"), UTF_8))
+        .containsExactly("-a", "-b")
+        .inOrder();
+    assertThat(execRoot.getChild("paramFile").isExecutable()).isTrue();
+  }
+
+  @Test
+  public void processInputFiles_materializesBinToolsFile() throws Exception {
+    SandboxHelpers sandboxHelpers = new SandboxHelpers(/*delayVirtualInputMaterialization=*/ false);
+    BinTools.PathActionInput tool =
+        new BinTools.PathActionInput(
+            scratch.file("tool", "#!/bin/bash", "echo hello"),
+            PathFragment.create("_bin/say_hello"));
+
+    SandboxInputs inputs =
+        sandboxHelpers.processInputFiles(inputMap(tool), SPAWN, EMPTY_EXPANDER, execRoot);
+
+    assertThat(inputs.getFiles())
+        .containsExactly(
+            PathFragment.create("_bin/say_hello"), execRoot.getRelative("_bin/say_hello"));
+    assertThat(inputs.getSymlinks()).isEmpty();
+    assertThat(FileSystemUtils.readLines(execRoot.getRelative("_bin/say_hello"), UTF_8))
+        .containsExactly("#!/bin/bash", "echo hello")
+        .inOrder();
+    assertThat(execRoot.getRelative("_bin/say_hello").isExecutable()).isTrue();
+  }
+
+  @Test
+  public void processInputFiles_delayVirtualInputMaterialization_doesNotStoreVirtualInput()
+      throws Exception {
+    SandboxHelpers sandboxHelpers = new SandboxHelpers(/*delayVirtualInputMaterialization=*/ true);
+    ParamFileActionInput paramFile =
+        new ParamFileActionInput(
+            PathFragment.create("paramFile"),
+            ImmutableList.of("-a", "-b"),
+            ParameterFileType.UNQUOTED,
+            UTF_8);
+
+    SandboxInputs inputs =
+        sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot);
+
+    assertThat(inputs.getFiles()).isEmpty();
+    assertThat(inputs.getSymlinks()).isEmpty();
+    assertThat(execRoot.getChild("paramFile").exists()).isFalse();
+  }
+
+  @Test
+  public void sandboxInputMaterializeVirtualInputs_delayMaterialization_writesCorrectFiles()
+      throws Exception {
+    SandboxHelpers sandboxHelpers = new SandboxHelpers(/*delayVirtualInputMaterialization=*/ true);
+    ParamFileActionInput paramFile =
+        new ParamFileActionInput(
+            PathFragment.create("paramFile"),
+            ImmutableList.of("-a", "-b"),
+            ParameterFileType.UNQUOTED,
+            UTF_8);
+    BinTools.PathActionInput tool =
+        new BinTools.PathActionInput(
+            scratch.file("tool", "tool_code"), PathFragment.create("tools/tool"));
+    SandboxInputs inputs =
+        sandboxHelpers.processInputFiles(
+            inputMap(paramFile, tool), SPAWN, EMPTY_EXPANDER, execRoot);
+
+    inputs.materializeVirtualInputs(scratch.dir("/sandbox"));
+
+    Path sandboxParamFile = scratch.resolve("/sandbox/paramFile");
+    assertThat(FileSystemUtils.readLines(sandboxParamFile, UTF_8))
+        .containsExactly("-a", "-b")
+        .inOrder();
+    assertThat(sandboxParamFile.isExecutable()).isTrue();
+    Path sandboxToolFile = scratch.resolve("/sandbox/tools/tool");
+    assertThat(FileSystemUtils.readLines(sandboxToolFile, UTF_8)).containsExactly("tool_code");
+    assertThat(sandboxToolFile.isExecutable()).isTrue();
+  }
+
+  private static ImmutableMap<PathFragment, ActionInput> inputMap(ActionInput... inputs) {
+    return Arrays.stream(inputs)
+        .collect(toImmutableMap(ActionInput::getExecPath, Function.identity()));
+  }
+
+  @Test
+  public void atomicallyWriteVirtualInput_writesParamFile() throws Exception {
+    ParamFileActionInput paramFile =
+        new ParamFileActionInput(
+            PathFragment.create("paramFile"),
+            ImmutableList.of("-a", "-b"),
+            ParameterFileType.UNQUOTED,
+            UTF_8);
+
+    SandboxHelpers.atomicallyWriteVirtualInput(
+        paramFile, scratch.resolve("/outputs/paramFile"), "-1234");
+
+    assertThat(scratch.resolve("/outputs").readdir(Symlinks.NOFOLLOW))
+        .containsExactly(new Dirent("paramFile", Dirent.Type.FILE));
+    Path outputFile = scratch.resolve("/outputs/paramFile");
+    assertThat(FileSystemUtils.readLines(outputFile, UTF_8)).containsExactly("-a", "-b").inOrder();
+    assertThat(outputFile.isExecutable()).isTrue();
+  }
+
+  @Test
+  public void atomicallyWriteVirtualInput_writesBinToolsFile() throws Exception {
+    BinTools.PathActionInput tool =
+        new BinTools.PathActionInput(
+            scratch.file("tool", "tool_code"), PathFragment.create("tools/tool"));
+
+    SandboxHelpers.atomicallyWriteVirtualInput(tool, scratch.resolve("/outputs/tool"), "-1234");
+
+    assertThat(scratch.resolve("/outputs").readdir(Symlinks.NOFOLLOW))
+        .containsExactly(new Dirent("tool", Dirent.Type.FILE));
+    Path outputFile = scratch.resolve("/outputs/tool");
+    assertThat(FileSystemUtils.readLines(outputFile, UTF_8)).containsExactly("tool_code");
+    assertThat(outputFile.isExecutable()).isTrue();
+  }
+
+  @Test
+  public void atomicallyWriteVirtualInput_writesArbitraryVirtualInput() throws Exception {
+    VirtualActionInput input = ActionsTestUtil.createVirtualActionInput("file", "hello");
+
+    SandboxHelpers.atomicallyWriteVirtualInput(input, scratch.resolve("/outputs/file"), "-1234");
+
+    assertThat(scratch.resolve("/outputs").readdir(Symlinks.NOFOLLOW))
+        .containsExactly(new Dirent("file", Dirent.Type.FILE));
+    Path outputFile = scratch.resolve("/outputs/file");
+    assertThat(FileSystemUtils.readLines(outputFile, UTF_8)).containsExactly("hello");
+    assertThat(outputFile.isExecutable()).isTrue();
+  }
+}