Add support for tmpfs mounts under /tmp with hermetic tmp

fmeum · copybara-github · commit 620d617b4402 · 2024-01-03T14:17:23.000-08:00
This is achieved by mounting tmpfs after regular mounts in the sandbox binary as well as creating the directories at which tmpfs are mounted under the sandbox tmp directory. Closes #20658. PiperOrigin-RevId: 595500822 Change-Id: Icf3d51bffdd004f668ba4fbbdbd5f833c20db3d9
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
@@ -59,7 +59,6 @@
 import java.time.Duration;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Optional;
 import java.util.SortedMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 import javax.annotation.Nullable;
@@ -74,7 +73,6 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
 
   // Since checking if sandbox is supported is expensive, we remember what we've checked.
   private static final Map<Path, Boolean> isSupportedMap = new HashMap<>();
-  private static final AtomicBoolean warnedAboutNonHermeticTmp = new AtomicBoolean();
 
   private static final AtomicBoolean warnedAboutUnsupportedModificationCheck = new AtomicBoolean();
 
@@ -205,22 +203,6 @@ private boolean useHermeticTmp() {
       return false;
     }
 
-    Optional<PathFragment> tmpfsPathUnderTmp =
-        getSandboxOptions().sandboxTmpfsPath.stream()
-            .filter(path -> path.startsWith(SLASH_TMP))
-            .findFirst();
-    if (tmpfsPathUnderTmp.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())));
-      }
-
-      return false;
-    }
-
     return true;
   }
 
@@ -295,6 +277,16 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
 
       createDirectoryWithinSandboxTmp(sandboxTmp, withinSandboxExecRoot);
       createDirectoryWithinSandboxTmp(sandboxTmp, withinSandboxWorkingDirectory);
+
+      for (PathFragment pathFragment : getSandboxOptions().sandboxTmpfsPath) {
+        Path path = fileSystem.getPath(pathFragment);
+        if (path.startsWith(SLASH_TMP)) {
+          // tmpfs mount points must exist, which is usually the user's responsibility. But if the
+          // user requests a tmpfs mount under /tmp, we have to create it under the sandbox tmp
+          // directory.
+          createDirectoryWithinSandboxTmp(sandboxTmp, path);
+        }
+      }
     }
 
     SandboxOutputs outputs = helpers.getOutputs(spawn);
diff --git a/src/main/tools/linux-sandbox-pid1.cc b/src/main/tools/linux-sandbox-pid1.cc
@@ -271,15 +271,6 @@ static void SetupUtsNamespace() {
 }
 
 static void MountFilesystems() {
-  for (const std::string &tmpfs_dir : opt.tmpfs_dirs) {
-    PRINT_DEBUG("tmpfs: %s", tmpfs_dir.c_str());
-    if (mount("tmpfs", tmpfs_dir.c_str(), "tmpfs",
-              MS_NOSUID | MS_NODEV | MS_NOATIME, nullptr) < 0) {
-      DIE("mount(tmpfs, %s, tmpfs, MS_NOSUID | MS_NODEV | MS_NOATIME, nullptr)",
-          tmpfs_dir.c_str());
-    }
-  }
-
   // An attempt to mount the sandbox in tmpfs will always fail, so this block is
   // slightly redundant with the next mount() check, but dumping the mount()
   // syscall is incredibly cryptic, so we explicitly check against and warn
@@ -307,6 +298,15 @@ static void MountFilesystems() {
     }
   }
 
+  for (const std::string &tmpfs_dir : opt.tmpfs_dirs) {
+    PRINT_DEBUG("tmpfs: %s", tmpfs_dir.c_str());
+    if (mount("tmpfs", tmpfs_dir.c_str(), "tmpfs",
+              MS_NOSUID | MS_NODEV | MS_NOATIME, nullptr) < 0) {
+      DIE("mount(tmpfs, %s, tmpfs, MS_NOSUID | MS_NODEV | MS_NOATIME, nullptr)",
+          tmpfs_dir.c_str());
+    }
+  }
+
   for (const std::string &writable_file : opt.writable_files) {
     PRINT_DEBUG("writable: %s", writable_file.c_str());
     if (bind_mount_sources.find(writable_file) != bind_mount_sources.end()) {
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunnerTest.java
@@ -203,27 +203,6 @@ public void hermeticTmp_sandboxTmpfsOnTmp_tmpNotCreatedOrMounted() throws Except
     assertThat(args).doesNotContain("-m /tmp");
   }
 
-  @Test
-  public void hermeticTmp_sandboxTmpfsUnderTmp_tmpNotCreatedOrMounted() throws Exception {
-    runtimeWrapper.addOptions(
-        "--incompatible_sandbox_hermetic_tmp", "--sandbox_tmpfs_path=/tmp/subdir");
-    CommandEnvironment commandEnvironment = createCommandEnvironment();
-    LinuxSandboxedSpawnRunner runner = setupSandboxAndCreateRunner(commandEnvironment);
-    Spawn spawn = new SpawnBuilder().build();
-    SandboxedSpawn sandboxedSpawn = runner.prepareSpawn(spawn, createSpawnExecutionContext(spawn));
-
-    Path sandboxPath =
-        sandboxedSpawn.getSandboxExecRoot().getParentDirectory().getParentDirectory();
-    Path hermeticTmpPath = sandboxPath.getRelative("_hermetic_tmp");
-    assertThat(hermeticTmpPath.isDirectory()).isFalse();
-
-    assertThat(sandboxedSpawn).isInstanceOf(SymlinkedSandboxedSpawn.class);
-    String args = String.join(" ", sandboxedSpawn.getArguments());
-    assertThat(args).contains("-w /tmp");
-    assertThat(args).contains("-e /tmp");
-    assertThat(args).doesNotContain("-m /tmp");
-  }
-
   private static LinuxSandboxedSpawnRunner setupSandboxAndCreateRunner(
       CommandEnvironment commandEnvironment) throws IOException {
     Path execRoot = commandEnvironment.getExecRoot();
diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -423,6 +423,47 @@ EOF
   bazel --output_base="$tmp_output_base" shutdown
 }
 
+function test_tmpfs_path_under_tmp() {
+  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 tmp_file=$(mktemp "/tmp/bazel_tmp.XXXXXXXX")
+  trap "rm $tmp_file" EXIT
+  echo BAD > "$tmp_file"
+
+  local tmpfs=$(mktemp -d "/tmp/bazel_tmpfs.XXXXXXXX")
+  trap "rm -fr $tmpfs" EXIT
+  echo BAD > "$tmpfs/data.txt"
+
+  mkdir -p pkg
+  cat > pkg/BUILD <<EOF
+genrule(
+    name = "gen",
+    outs = ["gen.txt"],
+    cmd = """
+# Verify that /tmp is still hermetic and that the tmpfs under /tmp exists, but is empty.
+[[ ! -e "${tmp_file}" ]]
+[[ -d /tmp/tmpfs ]]
+[[ ! -e /tmp/tmpfs/data.txt ]]
+# Verify that the tmpfs on /etc exists and is empty.
+[[ -d /etc ]]
+[[ -z "\$\$(ls -A /etc)" ]]
+touch \$@
+""",
+)
+EOF
+
+  # This assumes the existence of /etc on the host system
+  bazel build --sandbox_tmpfs_path=/tmp/tmpfs --sandbox_tmpfs_path=/etc \
+    //pkg:gen || fail "build failed"
+}
+
 # The test shouldn't fail if the environment doesn't support running it.
 check_sandbox_allowed || exit 0
 
