Fix racy write of temporary files while staging virtual inputs for the sandbox.
When staging virtual inputs without delayed materialization, `SandboxHelpers`
performs atomic writes by first staging virtual inputs into a temporary file
and later moving it to the target destination. This is achieved by performing
the initial writes into a file with a uniquifying suffix. This suffix happens
to currently always be hardcoded to `.sandbox`, which means that staging the
same virtual inputs for 2 actions may race on that.
Fix the race condition by providing a truly unique suffix for the temporary
file.
PiperOrigin-RevId: 351613739
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
index cf38f92..fbca3da 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
@@ -34,6 +34,7 @@
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
+import java.util.concurrent.atomic.AtomicInteger;
/**
* Helper methods that are shared by the different sandboxing strategies.
@@ -102,6 +103,10 @@
/** Wrapper class for the inputs of a sandbox. */
public static final class SandboxInputs {
+
+ private static final AtomicInteger tempFileUniquifierForVirtualInputWrites =
+ new AtomicInteger();
+
private final Map<PathFragment, Path> files;
private final Set<VirtualActionInput> virtualInputs;
private final Map<PathFragment, PathFragment> symlinks;
@@ -145,7 +150,12 @@
Path outputPath = execroot.getRelative(input.getExecPath());
if (isExecRootSandboxed) {
- atomicallyWriteVirtualInput(input, outputPath, ".sandbox");
+ atomicallyWriteVirtualInput(
+ input,
+ outputPath,
+ // When 2 actions try to atomically create the same virtual input, they need to have a
+ // different suffix for the temporary file in order to avoid racy write to the same one.
+ ".sandbox" + tempFileUniquifierForVirtualInputWrites.incrementAndGet());
return;
}
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/BUILD b/src/test/java/com/google/devtools/build/lib/sandbox/BUILD
index ff2c098..507ce36 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/BUILD
@@ -67,6 +67,7 @@
"//src/test/java/com/google/devtools/build/lib/testutil",
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
"//third_party:guava",
+ "//third_party:jsr305",
"//third_party:junit4",
"//third_party:truth",
],
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java
index 5a40d14..02e1481 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java
@@ -17,6 +17,7 @@
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.concurrent.TimeUnit.SECONDS;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -31,14 +32,26 @@
import com.google.devtools.build.lib.exec.util.SpawnBuilder;
import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
import com.google.devtools.build.lib.testutil.Scratch;
+import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.Dirent;
+import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Symlinks;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import java.io.IOException;
import java.util.Arrays;
+import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.Semaphore;
import java.util.function.Function;
+import javax.annotation.Nullable;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -53,12 +66,23 @@
private final Scratch scratch = new Scratch();
private Path execRoot;
+ @Nullable private ExecutorService executorToCleanup;
@Before
public void createExecRoot() throws IOException {
execRoot = scratch.dir("/execRoot");
}
+ @After
+ public void shutdownExecutor() throws InterruptedException {
+ if (executorToCleanup == null) {
+ return;
+ }
+
+ executorToCleanup.shutdown();
+ executorToCleanup.awaitTermination(TestUtils.WAIT_TIMEOUT_SECONDS, SECONDS);
+ }
+
@Test
public void processInputFiles_materializesParamFile() throws Exception {
SandboxHelpers sandboxHelpers = new SandboxHelpers(/*delayVirtualInputMaterialization=*/ false);
@@ -150,6 +174,56 @@
assertThat(sandboxToolFile.isExecutable()).isTrue();
}
+ /**
+ * Test simulating a scenario when 2 parallel writes of the same virtual input both complete write
+ * of the temp file and then proceed with post-processing steps one-by-one.
+ */
+ @Test
+ public void sandboxInputMaterializeVirtualInput_parallelWritesForSameInput_writesCorrectFile()
+ throws Exception {
+ VirtualActionInput input = ActionsTestUtil.createVirtualActionInput("file", "hello");
+ executorToCleanup = Executors.newSingleThreadExecutor();
+ CyclicBarrier bothWroteTempFile = new CyclicBarrier(2);
+ Semaphore finishProcessingSemaphore = new Semaphore(1);
+ FileSystem customFs =
+ new InMemoryFileSystem(DigestHashFunction.SHA1) {
+ @Override
+ protected void setExecutable(Path path, boolean executable) throws IOException {
+ try {
+ bothWroteTempFile.await();
+ finishProcessingSemaphore.acquire();
+ } catch (BrokenBarrierException | InterruptedException e) {
+ throw new IllegalArgumentException(e);
+ }
+ super.setExecutable(path, executable);
+ }
+ };
+ Scratch customScratch = new Scratch(customFs);
+ Path customExecRoot = customScratch.dir("/execroot");
+ SandboxHelpers sandboxHelpers = new SandboxHelpers(/*delayVirtualInputMaterialization=*/ false);
+
+ Future<?> future =
+ executorToCleanup.submit(
+ () -> {
+ try {
+ sandboxHelpers.processInputFiles(
+ inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot);
+ finishProcessingSemaphore.release();
+ } catch (IOException e) {
+ throw new IllegalArgumentException(e);
+ }
+ });
+ sandboxHelpers.processInputFiles(inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot);
+ finishProcessingSemaphore.release();
+ future.get();
+
+ assertThat(customExecRoot.readdir(Symlinks.NOFOLLOW))
+ .containsExactly(new Dirent("file", Dirent.Type.FILE));
+ Path outputFile = customExecRoot.getChild("file");
+ assertThat(FileSystemUtils.readLines(outputFile, UTF_8)).containsExactly("hello");
+ assertThat(outputFile.isExecutable()).isTrue();
+ }
+
private static ImmutableMap<PathFragment, ActionInput> inputMap(ActionInput... inputs) {
return Arrays.stream(inputs)
.collect(toImmutableMap(ActionInput::getExecPath, Function.identity()));
