From b416193075642017e13c774422b49cb07fb65c23 Mon Sep 17 00:00:00 2001 From: ajurkowski Date: Thu, 26 Nov 2020 10:22:31 -0800 Subject: [PATCH] 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 --- .../build/lib/sandbox/SandboxHelpers.java | 62 +++--- .../remote/RemoteActionInputFetcherTest.java | 2 +- .../google/devtools/build/lib/sandbox/BUILD | 6 + .../build/lib/sandbox/SandboxHelpersTest.java | 204 ++++++++++++++++++ 4 files changed, 245 insertions(+), 29 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java index d14b1b59155b4b..cf38f92895b930 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java +++ b/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 getSymlinks() { /** * Materializes a single virtual input inside the given execroot. * + *

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. 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 6f22ce6041b87d..0356f32faf122d 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 @@ -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(); } diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/BUILD b/src/test/java/com/google/devtools/build/lib/sandbox/BUILD index 876770c96df4ca..ff2c098cd7c83c 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/BUILD +++ b/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", diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java new file mode 100644 index 00000000000000..5a40d1430b54df --- /dev/null +++ b/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 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(); + } +}