Bazel 7: --sandbox_add_mount_pair under /tmp fails

[fmeum]

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.

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

[fmeum]

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?

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;

[fmeum]

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.

[fmeum]

@bazel-io flag

[fmeum]

@bazel-io fork 7.0.1

[lberki]

I have a fix for this one. It's a one-line fix, modulo the test.

[fmeum]

@lberki Feel free to reuse the integration tests from #20583.

[lberki]

Sorry, too late, I already wrote my own :) (very similar to yours, though)

[fmeum]

@lberki Thanks for pushing the fix, but my mount targets under /tmp are still failing. I trimmed my PR at #20583 down to just the new tests.

[lberki]

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.

[fmeum]

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.

[lberki]

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.

[iancha1992]

@bazel-io fork 7.1.0

[iancha1992]

A fix for this issue has been included in Bazel 7.0.1 RC2. Please test out the release candidate and report any issues as soon as possible. Thanks!