Skip to content

Commit

Permalink
Sandboxing: Fix crash triggered by precondition.
Browse files Browse the repository at this point in the history
The precondition was being triggered when the sandbox was left in a bad state
caused by an unfinished initialization of the linux-sandbox. This change
detects when that happens and cleans up accordingly.

RELNOTES:none
PiperOrigin-RevId: 630056883
Change-Id: Iea2423f2894966e8b53e1cd94e148c7de527544d
  • Loading branch information
oquenchil authored and copybara-github committed May 2, 2024
1 parent e3e530d commit 978c50b
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

/** Utility functions for the {@code linux-sandbox} embedded tool. */
public final class LinuxSandboxUtil {

private static final String LINUX_SANDBOX = "linux-sandbox" + OsUtils.executableExtension();

/** Returns whether using the {@code linux-sandbox} is supported in the command environment. */
Expand Down Expand Up @@ -99,7 +100,7 @@ public static void validateBindMounts(Map<Path, Path> bindMounts) throws UserExe
public static Path getInaccessibleHelperFile(Path sandboxBase) throws IOException {
// The order of the permissions settings calls matters, see
// https://github.com/bazelbuild/bazel/issues/16364
Path inaccessibleHelperFile = sandboxBase.getRelative("inaccessibleHelperFile");
Path inaccessibleHelperFile = sandboxBase.getRelative(SandboxHelpers.INACCESSIBLE_HELPER_FILE);
FileSystemUtils.touchFile(inaccessibleHelperFile);
inaccessibleHelperFile.setExecutable(false);
inaccessibleHelperFile.setWritable(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
public final class SandboxHelpers {

public static final String INACCESSIBLE_HELPER_DIR = "inaccessibleHelperDir";
public static final String INACCESSIBLE_HELPER_FILE = "inaccessibleHelperFile";

private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package com.google.devtools.build.lib.sandbox;

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.collect.ImmutableList.toImmutableList;

import com.google.common.base.Joiner;
import com.google.common.base.Splitter;
Expand Down Expand Up @@ -59,8 +58,10 @@
import java.io.IOException;
import java.time.Duration;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.Collectors;
import javax.annotation.Nullable;

/** This module provides the Sandbox spawn strategy. */
Expand Down Expand Up @@ -531,10 +532,25 @@ public void cleanStarting(@SuppressWarnings("unused") CleanStartingEvent event)
*/
private static void checkSandboxBaseTopOnlyContainsPersistentDirs(Path sandboxBase) {
try {
ImmutableList<String> directoryEntries =
List<String> directoryEntries =
sandboxBase.getDirectoryEntries().stream()
.map(Path::getBaseName)
.collect(toImmutableList());
.collect(Collectors.toList());
// If sandbox initialization failed in-between creating the inaccessible dir/file and adding
// the Linux sandboxing strategy to spawnRunners, then the sandbox base will be in a bad
// state. We check for that here and clean up.
if (directoryEntries.contains(SandboxHelpers.INACCESSIBLE_HELPER_DIR)) {
Path inaccessibleHelperDir = sandboxBase.getChild(SandboxHelpers.INACCESSIBLE_HELPER_DIR);
inaccessibleHelperDir.chmod(0700);
directoryEntries.remove(SandboxHelpers.INACCESSIBLE_HELPER_DIR);
inaccessibleHelperDir.deleteTree();
}
if (directoryEntries.contains(SandboxHelpers.INACCESSIBLE_HELPER_FILE)) {
Path inaccessibleHelperFile = sandboxBase.getChild(SandboxHelpers.INACCESSIBLE_HELPER_FILE);
directoryEntries.remove(SandboxHelpers.INACCESSIBLE_HELPER_FILE);
inaccessibleHelperFile.delete();
}

if (!SANDBOX_BASE_PERSISTENT_DIRS.containsAll(directoryEntries)) {
StringBuilder message =
new StringBuilder(
Expand Down
23 changes: 23 additions & 0 deletions src/test/shell/integration/sandboxing_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1061,6 +1061,29 @@ EOF
|| fail "Expected build to succeed"
}

function test_bad_state_linux_sandboxing() {
mkdir pkg

# This test is meant to catch a bad state being left over by an unfinished
# linux-sandboxing initialization. Since it's difficult to replicate the same
# conditions that end up in that state, this instead runs a null build
# where linux-sandboxing is unsupported by passing -1 grace seconds.
# Then we create inaccessibleHelperFile/Dir (the bad state) artificially and
# run a null build again making sure there is no crash.
bazel build --local_termination_grace_seconds=-1 \
|| fail "Expected build to succeed"
file_path="$(bazel info output_base)/sandbox/inaccessibleHelperFile"
dir_path="$(bazel info output_base)/sandbox/inaccessibleHelperDir"

touch $file_path
mkdir $dir_path
chmod 000 $file_path
chmod 000 $dir_path

bazel build --local_termination_grace_seconds=-1 \
|| fail "Expected build to succeed"
}

function is_bazel() {
[ $TEST_WORKSPACE == "_main" ]
}
Expand Down

3 comments on commit 978c50b

@fmeum
Copy link
Collaborator

@fmeum fmeum commented on 978c50b May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iancha1992 Could we cherry-pick this into 7.2.0?

@iancha1992
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iancha1992 Could we cherry-pick this into 7.2.0?

@fmeum @oquenchil Unfortunately, I am getting a conflict with src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java.
Maybe you can take a look?

@fmeum
Copy link
Collaborator

@fmeum fmeum commented on 978c50b May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sent #22342

Please sign in to comment.