Add support for multiplex worker sandboxing. This sandboxing requires worker support.

Design doc: https://docs.google.com/document/d/1ncLW0hz6uDhNvci1dpzfEoifwTiNTqiBEm1vi-bIIRM/edit

The multiplex worker process will run in a subdir "<mnemonic>-multiplex-workdir/<workspacename>" under the global execroot. Inside that directory, each request gets its own subdir "sandbox/<workerid>/<workspacename>".

RELNOTES: Adds `--experimental_worker_multiplex_sandboxing` flag that controls whether to sandbox multiplex workers that support it.
PiperOrigin-RevId: 419591728

Author: larsrc-google

site/docs/creating-workers.md

@@ -59,6 +59,9 @@ values indicate more verbose output. Passing the `--worker_verbose` flag to
 Bazel sets the `verbosity` field to 10, but smaller or larger values can be
 used manually for different amounts of output.
 
+The optional `sandbox_dir` field is used only by workers that support
+[multiplex sandboxing](multiplex-worker.md).
+
 ### Work responses
 
 A `WorkResponse` contains a request id, a zero or nonzero exit

site/docs/multiplex-worker.md

@@ -66,14 +66,33 @@ A ruleset is encouraged to use multiplex workers if possible, to reduce memory
 pressure and improve performance. However, multiplex workers are not currently
 compatible with [dynamic execution](/dynamic-execution.html).
 
-### Warning about rare bug
+## Multiplex sandboxing
 
-Due to a rare bug, multiplex workers are currently not enabled by default.
-Occasionally,  Bazel hangs indefinitely at the execution phase. If you see this
-behavior,  stop the Bazel server and rerun. This delay is probably caused by
+Multiplex workers can be sandboxed by adding explicit support for it in the
+worker implementations. While singleplex worker sandboxing can be done by
+running each worker process in its own sandbox, multiplex workers share the
+process working directory between multiple parallel requests. To allow
+sandboxing of multiplex workers, the worker must support reading from and
+writing to a subdirectory specified in each request, instead of directly in
+its working directory.
 
- * Multiplex workers waiting for responses from the worker process that never
-   comes.
- * Incorrectly configured ruleset worker implementation where a thread dies or
-   a race condition occurs. To counteract this, ensure the worker process
-   returns responses in all circumstances.
+To support multiplex sandboxing, the worker must use the `sandbox_dir` field
+from the `WorkRequest` and use that as a prefix for all file reads and writes.
+While the `arguments` and `inputs` fields remain unchanged from an unsandboxed
+request, the actual inputs are relative to the `sandbox_dir`. The worker must
+translate file paths found in `arguments` and `inputs` to read from this
+modified path, and must also write all outputs relative to the `sandbox_dir`.
+This includes paths such as '.', as well as paths found in files specified
+in the arguments (e.g. ["argfile"](https://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#commandlineargfile) arguments).
+
+Once a worker supports multiplex sandboxing, the ruleset can declare this
+support by adding `supports-multiplex-sandboxing` to the
+`execution_requirements` of an action. Bazel will then use multiplex sandboxing
+if the `--experimental_worker_multiplex_sandboxing` flag is passed, or if
+the worker is used with dynamic execution.
+
+The worker files of a sandboxed multiplex worker are still relative to the
+working directory of the worker process. Thus, if a file is
+used both for running the worker and as an input, it must be specified both as
+an input in the flagfile argument as well as in `tools`, `executable`, or
+`runfiles`.

site/docs/persistent-workers.md

@@ -250,6 +250,11 @@ Even when using the input digests to guard against unwanted caching, sandboxed
 workers offer less strict sandboxing than a pure sandbox, because the tool may
 keep other internal state that has been affected by previous requests.
 
+Multiplex workers can only be sandboxed if the worker implementation support it,
+and this sandboxing must be separately enabled with the
+`--experimental_worker_multiplex_sandboxing` flag. See more details in
+[the design doc](https://docs.google.com/document/d/1ncLW0hz6uDhNvci1dpzfEoifwTiNTqiBEm1vi-bIIRM/edit))
+
 ## Further reading <a name="further-reading"></a>
 
 For more information on persistent workers, see:

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java

@@ -162,6 +162,8 @@ public String parseIfMatches(String tag) throws ValidationException {
 
   public static final String SUPPORTS_WORKER_CANCELLATION = "supports-worker-cancellation";
 
+  public static final String SUPPORTS_MULTIPLEX_SANDBOXING = "supports-multiplex-sandboxing";
+
   /** Denotes what the type of worker protocol the worker uses. */
   public enum WorkerProtocolFormat {
     JSON,

src/main/java/com/google/devtools/build/lib/actions/Spawns.java

@@ -101,6 +101,15 @@ public static boolean supportsWorkerCancellation(Spawn spawn) {
         .equals(spawn.getExecutionInfo().get(ExecutionRequirements.SUPPORTS_WORKER_CANCELLATION));
   }
 
+  /**
+   * Returns whether the {@link Spawn} supports sandboxing for multiplex workers through the {@code
+   * WorkRequest.sandbox_dir} field.
+   */
+  public static boolean supportsMultiplexSandboxing(Spawn spawn) {
+    return "1"
+        .equals(spawn.getExecutionInfo().get(ExecutionRequirements.SUPPORTS_MULTIPLEX_SANDBOXING));
+  }
+
   /**
    * Returns which worker protocol format a Spawn claims a persistent worker uses. Defaults to proto
    * if the protocol format is not specified.

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

@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.sandbox;
 
 
-import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -27,7 +26,6 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
-import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -115,7 +113,7 @@ public void createFileSystem() throws IOException {
     filterInputsAndDirsToCreate(inputsToCreate, dirsToCreate);
 
     // Finally create what needs creating.
-    createDirectories(dirsToCreate);
+    SandboxHelpers.createDirectories(dirsToCreate, sandboxExecRoot, /* strict=*/ true);
     createInputs(inputsToCreate, inputs);
     inputs.materializeVirtualInputs(sandboxExecRoot);
   }
@@ -124,45 +122,6 @@ protected void filterInputsAndDirsToCreate(
       Set<PathFragment> inputsToCreate, LinkedHashSet<PathFragment> dirsToCreate)
       throws IOException {}
 
-  /**
-   * Creates all directories needed for the sandbox.
-   *
-   * <p>No input can be a child of another input, because otherwise we might try to create a symlink
-   * below another symlink we created earlier - which means we'd actually end up writing somewhere
-   * in the workspace.
-   *
-   * <p>If all inputs were regular files, this situation could naturally not happen - but
-   * unfortunately, we might get the occasional action that has directories in its inputs.
-   *
-   * <p>Creating all parent directories first ensures that we can safely create symlinks to
-   * directories, too, because we'll get an IOException with EEXIST if inputs happen to be nested
-   * once we start creating the symlinks for all inputs.
-   */
-  void createDirectories(Iterable<PathFragment> dirsToCreate) throws IOException {
-    Set<Path> knownDirectories = new HashSet<>();
-    // Add sandboxExecRoot and it's parent -- all paths must fall under the parent of
-    // sandboxExecRoot and we know that sandboxExecRoot exists. This stops the recursion in
-    // createDirectoryAndParentsInSandboxRoot.
-    knownDirectories.add(sandboxExecRoot);
-    knownDirectories.add(sandboxExecRoot.getParentDirectory());
-
-    for (PathFragment path : dirsToCreate) {
-      Preconditions.checkArgument(!path.isAbsolute(), path);
-      if (path.containsUplevelReferences() && path.isMultiSegment()) {
-        // Allow a single up-level reference to allow inputs from the siblings of the main
-        // repository in the sandbox execution root, but forbid multiple up-level references.
-        // PathFragment is normalized, so up-level references are guaranteed to be at the beginning.
-        Preconditions.checkArgument(
-            !PathFragment.containsUplevelReferences(path.getSegment(1)),
-            "%s escapes the sandbox exec root.",
-            path);
-      }
-
-      SandboxHelpers.createDirectoryAndParentsInSandboxRoot(
-          sandboxExecRoot.getRelative(path), knownDirectories, sandboxExecRoot);
-    }
-  }
-
   /**
    * Creates all inputs needed for this spawn's sandbox.
    *

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

@@ -19,9 +19,11 @@
 import static com.google.devtools.build.lib.vfs.Dirent.Type.SYMLINK;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Maps;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -309,6 +311,51 @@ public static void createDirectoryAndParentsInSandboxRoot(
     knownDirectories.add(path);
   }
 
+  /**
+   * Creates all directories needed for the sandbox.
+   *
+   * <p>No input can be a child of another input, because otherwise we might try to create a symlink
+   * below another symlink we created earlier - which means we'd actually end up writing somewhere
+   * in the workspace.
+   *
+   * <p>If all inputs were regular files, this situation could naturally not happen - but
+   * unfortunately, we might get the occasional action that has directories in its inputs.
+   *
+   * <p>Creating all parent directories first ensures that we can safely create symlinks to
+   * directories, too, because we'll get an IOException with EEXIST if inputs happen to be nested
+   * once we start creating the symlinks for all inputs.
+   *
+   * @param strict If true, absolute directories or directories with multiple up-level references
+   *     are disallowed, for stricter sandboxing.
+   */
+  public static void createDirectories(
+      Iterable<PathFragment> dirsToCreate, Path dir, boolean strict) throws IOException {
+    Set<Path> knownDirectories = new HashSet<>();
+    // Add sandboxExecRoot and it's parent -- all paths must fall under the parent of
+    // sandboxExecRoot and we know that sandboxExecRoot exists. This stops the recursion in
+    // createDirectoryAndParentsInSandboxRoot.
+    knownDirectories.add(dir);
+    knownDirectories.add(dir.getParentDirectory());
+
+    for (PathFragment path : dirsToCreate) {
+      if (strict) {
+        Preconditions.checkArgument(!path.isAbsolute(), path);
+        if (path.containsUplevelReferences() && path.isMultiSegment()) {
+          // Allow a single up-level reference to allow inputs from the siblings of the main
+          // repository in the sandbox execution root, but forbid multiple up-level references.
+          // PathFragment is normalized, so up-level references are guaranteed to be at the
+          // beginning.
+          Preconditions.checkArgument(
+              !PathFragment.containsUplevelReferences(path.getSegment(1)),
+              "%s escapes the sandbox exec root.",
+              path);
+        }
+      }
+
+      createDirectoryAndParentsInSandboxRoot(dir.getRelative(path), knownDirectories, dir);
+    }
+  }
+
   /** Wrapper class for the inputs of a sandbox. */
   public static final class SandboxInputs {
 
@@ -397,6 +444,17 @@ public void materializeVirtualInputs(Path sandboxExecRoot) throws IOException {
       }
     }
 
+    /**
+     * Returns a new SandboxInputs instance with only the inputs/symlinks listed in {@code allowed}
+     * included.
+     */
+    public SandboxInputs limitedCopy(Set<PathFragment> allowed) {
+      return new SandboxInputs(
+          Maps.filterKeys(files, allowed::contains),
+          ImmutableSet.of(),
+          Maps.filterKeys(symlinks, allowed::contains));
+    }
+
     @Override
     public String toString() {
       return "Files: " + files + "\nVirtualInputs: " + virtualInputs + "\nSymlinks: " + symlinks;

src/main/java/com/google/devtools/build/lib/worker/SandboxedWorkerProxy.java

@@ -0,0 +1,116 @@
+// Copyright 2021 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.worker;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import com.google.common.flogger.GoogleLogger;
+import com.google.devtools.build.lib.sandbox.SandboxHelpers;
+import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
+import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest;
+import java.io.IOException;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.Set;
+
+/**
+ * A multiplex worker proxy with sandboxing. The multiplexer process runs in {@code workDir}, while
+ * each proxy has a fixed subdir where it sets up its files. The subdir is then passed to the worker
+ * in {@link WorkRequest#sandbox_dir}. The worker implementation is responsible for reading from and
+ * writing to that subdir only.
+ */
+public class SandboxedWorkerProxy extends WorkerProxy {
+  private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
+  /** The sandbox directory for the current request, inside {@code workDir}. */
+  private final Path sandboxDir;
+
+  private final PathFragment sandboxName;
+
+  SandboxedWorkerProxy(
+      WorkerKey workerKey,
+      int workerId,
+      Path logFile,
+      WorkerMultiplexer workerMultiplexer,
+      Path workDir) {
+    super(workerKey, workerId, logFile, workerMultiplexer, workDir);
+    sandboxName =
+        PathFragment.create(
+            Joiner.on(PathFragment.SEPARATOR_CHAR)
+                .join(
+                    "__sandbox",
+                    Integer.toString(workerId),
+                    workerKey.getExecRoot().getBaseName()));
+    sandboxDir = this.workDir.getRelative(sandboxName);
+  }
+
+  @Override
+  public boolean isSandboxed() {
+    return true;
+  }
+
+  @Override
+  public void prepareExecution(
+      SandboxInputs inputFiles, SandboxOutputs outputs, Set<PathFragment> workerFiles)
+      throws IOException {
+    workerMultiplexer.createSandboxedProcess(workDir, workerFiles, inputFiles);
+
+    sandboxDir.createDirectoryAndParents();
+    LinkedHashSet<PathFragment> dirsToCreate = new LinkedHashSet<>();
+    Set<PathFragment> inputsToCreate = new HashSet<>();
+
+    SandboxHelpers.populateInputsAndDirsToCreate(
+        ImmutableSet.of(),
+        inputsToCreate,
+        dirsToCreate,
+        Iterables.concat(inputFiles.getFiles().keySet(), inputFiles.getSymlinks().keySet()),
+        outputs.files(),
+        outputs.dirs());
+    SandboxHelpers.cleanExisting(
+        sandboxDir.getParentDirectory(), inputFiles, inputsToCreate, dirsToCreate, sandboxDir);
+    // Finally, create anything that is still missing. This is non-strict only for historical
+    // reasons, we haven't seen what would break if we make it strict.
+    SandboxHelpers.createDirectories(dirsToCreate, sandboxDir, /* strict=*/ false);
+    WorkerExecRoot.createInputs(inputsToCreate, inputFiles, sandboxDir);
+  }
+
+  /** Send the WorkRequest to multiplexer. */
+  @Override
+  protected void putRequest(WorkRequest request) throws IOException {
+    // Modifying the request on the way out is not great. The alternatives are having the
+    // spawn runner ask the worker for the dir or making the spawn runner understand the sandbox,
+    // dir structure, neither of which are nice either.
+    workerMultiplexer.putRequest(
+        request.toBuilder().setSandboxDir(sandboxName.getPathString()).build());
+  }
+
+  @Override
+  public void finishExecution(Path execRoot, SandboxOutputs outputs) throws IOException {
+    SandboxHelpers.moveOutputs(outputs, sandboxDir, execRoot);
+  }
+
+  @Override
+  synchronized void destroy() {
+    super.destroy();
+    try {
+      workDir.deleteTree();
+    } catch (IOException e) {
+      logger.atWarning().withCause(e).log("Caught IOException while deleting workdir.");
+    }
+  }
+}

src/main/java/com/google/devtools/build/lib/worker/Worker.java

@@ -103,7 +103,12 @@ public abstract void prepareExecution(
    */
   abstract WorkResponse getResponse(int requestId) throws IOException, InterruptedException;
 
-  /** Does whatever cleanup may be required after execution is done. */
+  /**
+   * Does whatever cleanup may be required after execution is done.
+   *
+   * @param execRoot The global execRoot, where outputs must go.
+   * @param outputs The expected outputs.
+   */
   public abstract void finishExecution(Path execRoot, SandboxOutputs outputs) throws IOException;
 
   /**