From eb762d4e7431637e607146b1c191485795047ef9 Mon Sep 17 00:00:00 2001 From: ajurkowski Date: Wed, 13 Jan 2021 10:22:00 -0800 Subject: [PATCH] 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 --- .../build/lib/sandbox/SandboxHelpers.java | 12 ++- .../google/devtools/build/lib/sandbox/BUILD | 1 + .../build/lib/sandbox/SandboxHelpersTest.java | 74 +++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) 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 cf38f92895b930..fbca3da699f954 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 @@ public static void atomicallyWriteVirtualInput( /** Wrapper class for the inputs of a sandbox. */ public static final class SandboxInputs { + + private static final AtomicInteger tempFileUniquifierForVirtualInputWrites = + new AtomicInteger(); + private final Map files; private final Set virtualInputs; private final Map symlinks; @@ -145,7 +150,12 @@ private static void materializeVirtualInput( 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 ff2c098cd7c83c..507ce365acf411 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 @@ java_test( "//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 5a40d1430b54df..02e1481ccecad0 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 @@ public class SandboxHelpersTest { 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 @@ public void sandboxInputMaterializeVirtualInputs_delayMaterialization_writesCorr 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 inputMap(ActionInput... inputs) { return Arrays.stream(inputs) .collect(toImmutableMap(ActionInput::getExecPath, Function.identity()));