From cd3e908c84671e7a875c73efc57072fca4cb4b8d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 3 Jan 2024 14:15:55 -0800 Subject: [PATCH] Add support for tmpfs mounts under `/tmp` with hermetic tmp 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 --- .../sandbox/LinuxSandboxedSpawnRunner.java | 28 +++++-------- src/main/tools/linux-sandbox-pid1.cc | 18 ++++---- .../LinuxSandboxedSpawnRunnerTest.java | 21 ---------- src/test/shell/bazel/bazel_sandboxing_test.sh | 41 +++++++++++++++++++ 4 files changed, 60 insertions(+), 48 deletions(-) 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 ac9c5ae72fcc13..b28bbc480e6b2d 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 @@ -60,7 +60,6 @@ import java.time.Duration; import java.util.HashMap; import java.util.Map; -import java.util.Optional; import java.util.SortedMap; import java.util.Set; import java.util.TreeSet; @@ -77,7 +76,6 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { // Since checking if sandbox is supported is expensive, we remember what we've checked. private static final Map isSupportedMap = new HashMap<>(); - private static final AtomicBoolean warnedAboutNonHermeticTmp = new AtomicBoolean(); private static final AtomicBoolean warnedAboutUnsupportedModificationCheck = new AtomicBoolean(); @@ -208,22 +206,6 @@ private boolean useHermeticTmp() { return false; } - Optional 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; } @@ -298,6 +280,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 index 72e8e70c4afb47..0dce5b3a4041da 100644 --- 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 index dd5363686efcc6..1dfbf40572d4d0 100644 --- 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 index 86fa7ad1807cd4..f74bf082fc21aa 100755 --- 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 <