Repository: bazelbuild/bazel
Issue: Bazel 7: `--sandbox_add_mount_pair` under `/tmp` fails
Labels: type: bug
team-Local-Exec

### Description of the bug:


With Bazel 7, but neither Bazel 6.4.0 nor `--noincompatible_sandbox_hermetic_tmp`, builds with `--sandbox_add_mount_pair` referencing a path under `/tmp` fail since `/tmp` has been remounted before the manually specified mount pair is applied.

### Which category does this issue belong to?


Local Execution

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


```shell
touch WORKSPACE

cat > .bazelrc <<'EOF'
build --sandbox_add_mount_pair=/tmp/some/path:/etc
EOF

cat > BUILD <<'EOF'
genrule(
    name = "gen",
    outs = ["data.txt"],
    cmd = "ls /etc > $@",
)
EOF
```

Then:
```
$ mkdir -p /tmp/some/path
$ bazel clean --expunge && bazel shutdown && bazel build //:gen
...
src/main/tools/linux-sandbox-pid1.cc:305: "mount(/tmp/some/path, /etc, nullptr, MS_BIND | MS_REC, nullptr)": No such file or directory
Target //:gen failed to build
...
# bazel clean --expunge && bazel shutdown && bazel build //:gen --noincompatible_sandbox_hermetic_tmp
...
INFO: Build completed successfully, 2 total actions
```


### Which operating system are you running Bazel on?


Linux

### What is the output of `bazel info release`?


7.0.0

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.


_No response_

### Have you found anything relevant by searching the web?

_No response_

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
CC @lberki 

A simple workaround would be to just disable the hermetic `/tmp` in this case, similar to what we do for tmpfs paths. What do you think of this?

```java
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
index 3f6e49c72c..1e9b58ae00 100644
@@ -206,17 +207,21 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
       return false;
     }
 
-    Optional<PathFragment> tmpfsPathUnderTmp =
-        getSandboxOptions().sandboxTmpfsPath.stream()
+    Optional<PathFragment> mountUnderTmp =
+        Stream.concat(
+                getSandboxOptions().sandboxTmpfsPath.stream(),
+                getSandboxOptions().sandboxAdditionalMounts.stream()
+                    .map(Map.Entry::getKey)
+                    .map(PathFragment::create))
             .filter(path -> path.startsWith(SLASH_TMP))
             .findFirst();
-    if (tmpfsPathUnderTmp.isPresent()) {
+    if (mountUnderTmp.isPresent()) {
       if (warnedAboutNonHermeticTmp.compareAndSet(false, true)) {
         reporter.handle(
             Event.warn(
                 String.format(
-                    "Falling back to non-hermetic '/tmp' in sandbox due to '%s' being a tmpfs path",
-                    tmpfsPathUnderTmp.get())));
+                    "Falling back to non-hermetic '/tmp' in sandbox due to '%s' being a tmpfs path or mount source.",
+                    mountUnderTmp.get())));
       }
 
       return false;
```
Flipping source and target in my example also results in a failure, not sure why I couldn't reproduce this earlier. I have edited the issue description accordingly.
@bazel-io flag 
@bazel-io fork 7.0.1
I have a fix for this one. It's a one-line fix, modulo the test.
@lberki Feel free to reuse the integration tests from https://github.com/bazelbuild/bazel/pull/20583.
Sorry, too late, I already wrote my own :) (very similar to yours, though)
@lberki Thanks for pushing the fix, but my mount targets under `/tmp` are still failing. I trimmed my PR at https://github.com/bazelbuild/bazel/pull/20583 down to just the new tests.
Mount *targets*? I seem to remember discussing this with you at some point in time, and IIRC the consensus was that mount targets under `/tmp` aren't a good idea: the whole point of `--incompatible_sandbox_hermetic_tmp` is that `/tmp` is pristine and mount points under it seem to compromise that.
I would say that's still entirely true, but with the flag flip, the user is no longer the one who specified both `--incompatible_sandbox_hermetic_tmp` *and* the mount pair. I think that it would be entirely reasonable to just automatically disable the hermetic `/tmp` in that case, but not doing so results in a confusing error.
WDYT about emitting a non-confusing error?

I agree that the current situation isn't fantastic and technically speaking, allowing mount targets under `/tmp` is not impossible, but I'd rather not allow that, at least on the first attempt.

I also have an ardent desire to delete the old code path so reverting back to it under certain conditions isn't my favorite thing, either.
@bazel-io fork 7.1.0
A fix for this issue has been included in [Bazel 7.0.1 RC2](https://releases.bazel.build/7.0.1/rc2/index.html). Please test out the release candidate and report any issues as soon as possible. Thanks!

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
index 5e5e0a825d9cbf..8c333be0553f73 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
@@ -14,13 +14,13 @@
 
 package com.google.devtools.build.lib.sandbox;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NETNS_WITH_LOOPBACK;
 import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NO_NETNS;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Maps;
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -61,6 +61,7 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.SortedMap;
+import java.util.TreeMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 import javax.annotation.Nullable;
 
@@ -313,7 +314,8 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
             .addExecutionInfo(spawn.getExecutionInfo())
             .setWritableFilesAndDirectories(writableDirs)
             .setTmpfsDirectories(ImmutableSet.copyOf(getSandboxOptions().sandboxTmpfsPath))
-            .setBindMounts(getBindMounts(blazeDirs, inputs, sandboxExecRootBase, sandboxTmp))
+            .setBindMounts(
+                prepareAndGetBindMounts(blazeDirs, inputs, sandboxExecRootBase, sandboxTmp))
             .setUseFakeHostname(getSandboxOptions().sandboxFakeHostname)
             .setEnablePseudoterminal(getSandboxOptions().sandboxExplicitPseudoterminal)
             .setCreateNetworkNamespace(createNetworkNamespace ? NETNS_WITH_LOOPBACK : NO_NETNS)
@@ -403,50 +405,73 @@ protected ImmutableSet<Path> getWritableDirs(
     return writableDirs.build();
   }
 
-  private ImmutableList<BindMount> getBindMounts(
+  private ImmutableList<BindMount> prepareAndGetBindMounts(
       BlazeDirectories blazeDirs,
       SandboxInputs inputs,
       Path sandboxExecRootBase,
       @Nullable Path sandboxTmp)
-      throws UserExecException {
-    Path tmpPath = fileSystem.getPath("/tmp");
-    final SortedMap<Path, Path> bindMounts = Maps.newTreeMap();
+      throws UserExecException, IOException {
+    Path tmpPath = fileSystem.getPath(SLASH_TMP);
+    final SortedMap<Path, Path> userBindMounts = new TreeMap<>();
     SandboxHelpers.mountAdditionalPaths(
-        getSandboxOptions().sandboxAdditionalMounts, sandboxExecRootBase, bindMounts);
+        getSandboxOptions().sandboxAdditionalMounts, sandboxExecRootBase, userBindMounts);
 
     for (Path inaccessiblePath : getInaccessiblePaths()) {
       if (inaccessiblePath.isDirectory(Symlinks.NOFOLLOW)) {
-        bindMounts.put(inaccessiblePath, inaccessibleHelperDir);
+        userBindMounts.put(inaccessiblePath, inaccessibleHelperDir);
       } else {
-        bindMounts.put(inaccessiblePath, inaccessibleHelperFile);
+        userBindMounts.put(inaccessiblePath, inaccessibleHelperFile);
       }
     }
 
-    LinuxSandboxUtil.validateBindMounts(bindMounts);
-    ImmutableList.Builder<BindMount> result = ImmutableList.builder();
-    bindMounts.forEach((k, v) -> result.add(BindMount.of(k, v)));
+    LinuxSandboxUtil.validateBindMounts(userBindMounts);
 
-    if (sandboxTmp != null) {
-      // First mount the real exec root and the empty directory created as the working dir of the
-      // action under $SANDBOX/_tmp
-      result.add(BindMount.of(sandboxTmp.getRelative(BAZEL_EXECROOT), blazeDirs.getExecRootBase()));
-      result.add(
-          BindMount.of(sandboxTmp.getRelative(BAZEL_WORKING_DIRECTORY), sandboxExecRootBase));
-
-      // Then mount the individual package roots under $SANDBOX/_tmp/bazel-source-roots
-      inputs
-          .getSourceRootBindMounts()
-          .forEach(
-              (withinSandbox, real) -> {
-                PathFragment sandboxTmpSourceRoot = withinSandbox.asPath().relativeTo(tmpPath);
-                result.add(BindMount.of(sandboxTmp.getRelative(sandboxTmpSourceRoot), real));
-              });
-
-      // Then mount $SANDBOX/_tmp at /tmp. At this point, even if the output base (and execroot)
-      // and individual source roots are under /tmp, they are accessible at /tmp/bazel-*
-      result.add(BindMount.of(tmpPath, sandboxTmp));
+    if (sandboxTmp == null) {
+      return userBindMounts.entrySet().stream()
+          .map(e -> BindMount.of(e.getKey(), e.getValue()))
+          .collect(toImmutableList());
     }
 
+    SortedMap<Path, Path> bindMounts = new TreeMap<>();
+    for (var entry : userBindMounts.entrySet()) {
+      Path mountPoint = entry.getKey();
+      Path content = entry.getValue();
+      if (mountPoint.startsWith(tmpPath)) {
+        // sandboxTmp should be null if /tmp is an explicit mount point since useHermeticTmp()
+        // returns false in that case.
+        if (mountPoint.equals(tmpPath)) {
+          throw new IOException(
+              "Cannot mount /tmp explicitly with hermetic /tmp. Please file a bug at"
+                  + " https://github.com/bazelbuild/bazel/issues/new/choose.");
+        }
+        // We need to rewrite the mount point to be under the sandbox tmp directory, which will be
+        // mounted onto /tmp as the final mount.
+        mountPoint = sandboxTmp.getRelative(mountPoint.relativeTo(tmpPath));
+        mountPoint.createDirectoryAndParents();
+      }
+      bindMounts.put(mountPoint, content);
+    }
+
+    ImmutableList.Builder<BindMount> result = ImmutableList.builder();
+    bindMounts.forEach((k, v) -> result.add(BindMount.of(k, v)));
+
+    // First mount the real exec root and the empty directory created as the working dir of the
+    // action under $SANDBOX/_tmp
+    result.add(BindMount.of(sandboxTmp.getRelative(BAZEL_EXECROOT), blazeDirs.getExecRootBase()));
+    result.add(BindMount.of(sandboxTmp.getRelative(BAZEL_WORKING_DIRECTORY), sandboxExecRootBase));
+
+    // Then mount the individual package roots under $SANDBOX/_tmp/bazel-source-roots
+    inputs
+        .getSourceRootBindMounts()
+        .forEach(
+            (withinSandbox, real) -> {
+              PathFragment sandboxTmpSourceRoot = withinSandbox.asPath().relativeTo(tmpPath);
+              result.add(BindMount.of(sandboxTmp.getRelative(sandboxTmpSourceRoot), real));
+            });
+
+    // Then mount $SANDBOX/_tmp at /tmp. At this point, even if the output base (and execroot) and
+    // individual source roots are under /tmp, they are accessible at /tmp/bazel-*
+    result.add(BindMount.of(tmpPath, sandboxTmp));
     return result.build();
   }
 


Test patch:
diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh
index 86fa7ad1807cd4..ec7655ac36d53b 100755
--- a/src/test/shell/bazel/bazel_sandboxing_test.sh
+++ b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -316,21 +316,84 @@ function test_add_mount_pair_tmp_source() {
 
   sed -i.bak '/sandbox_tmpfs_path/d' $TEST_TMPDIR/bazelrc
 
+  local mounted=$(mktemp -d "/tmp/bazel_mounted.XXXXXXXX")
+  trap "rm -fr $mounted" EXIT
+  echo GOOD > "$mounted/data.txt"
+
   mkdir -p pkg
-  cat > pkg/BUILD <<'EOF'
+  cat > pkg/BUILD <<EOF
+genrule(
+    name = "gen",
+    outs = ["gen.txt"],
+    # Verify that /tmp is still hermetic.
+    cmd = """[ ! -e "${mounted}/data.txt" ] && cp /etc/data.txt \$@""",
+)
+EOF
+
+  # This assumes the existence of /etc on the host system
+  bazel build --sandbox_add_mount_pair="$mounted:/etc" //pkg:gen || fail "build failed"
+  assert_contains GOOD bazel-bin/pkg/gen.txt
+}
+
+function test_add_mount_pair_tmp_target() {
+  if [[ "$PLATFORM" == "darwin" ]]; then
+    # Tests Linux-specific functionality
+    return 0
+  fi
+
+  create_workspace_with_default_repos WORKSPACE
+
+  sed -i.bak '/sandbox_tmpfs_path/d' $TEST_TMPDIR/bazelrc
+
+  local source_dir=$(mktemp -d "/tmp/bazel_mounted.XXXXXXXX")
+  trap "rm -fr $source_dir" EXIT
+  echo BAD > "$source_dir/data.txt"
+
+  mkdir -p pkg
+  cat > pkg/BUILD <<EOF
 genrule(
     name = "gen",
     outs = ["gen.txt"],
-    cmd = "cp /etc/data.txt $@",
+    # Verify that /tmp is still hermetic.
+    cmd = """[ ! -e "${source_dir}/data.txt" ] && ls "$source_dir" > \$@""",
 )
 EOF
 
+
+  # This assumes the existence of /etc on the host system
+  bazel build --sandbox_add_mount_pair="/etc:$source_dir" //pkg:gen || fail "build failed"
+  assert_contains passwd bazel-bin/pkg/gen.txt
+}
+
+function test_add_mount_pair_tmp_target_and_source() {
+  if [[ "$PLATFORM" == "darwin" ]]; then
+    # Tests Linux-specific functionality
+    return 0
+  fi
+
+  create_workspace_with_default_repos WORKSPACE
+
+  sed -i.bak '/sandbox_tmpfs_path/d' $TEST_TMPDIR/bazelrc
+
   local mounted=$(mktemp -d "/tmp/bazel_mounted.XXXXXXXX")
   trap "rm -fr $mounted" EXIT
   echo GOOD > "$mounted/data.txt"
 
-  # This assumes the existence of /etc on the host system
-  bazel build --sandbox_add_mount_pair="$mounted:/etc" //pkg:gen || fail "build failed"
+  local tmp_file=$(mktemp "/tmp/bazel_tmp.XXXXXXXX")
+  trap "rm $tmp_file" EXIT
+  echo BAD > "$tmp_file"
+
+  mkdir -p pkg
+  cat > pkg/BUILD <<EOF
+genrule(
+    name = "gen",
+    outs = ["gen.txt"],
+    # Verify that /tmp is still hermetic.
+    cmd = """[ ! -e "${tmp_file}" ] && cp "$mounted/data.txt" \$@""",
+)
+EOF
+
+  bazel build --sandbox_add_mount_pair="$mounted" //pkg:gen || fail "build failed"
   assert_contains GOOD bazel-bin/pkg/gen.txt
 }