From 620d617b440258799caa1be434ed66e9ca8fa8c5 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 5e5e0a825d9cbf..039c8db3a6c55b 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 @@ -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 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 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 index e032c689bb484f..c0626cb3b1fb74 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 <