Fix racy write of temporary files while staging virtual inputs for the sandbox.

alexjski · copybara-github · commit eb762d4e7431 · 2021-01-13T10:23:12.000-08:00
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
@@ -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<PathFragment, Path> files;
     private final Set<VirtualActionInput> virtualInputs;
     private final Map<PathFragment, PathFragment> 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
@@ -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
@@ -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<PathFragment, ActionInput> inputMap(ActionInput... inputs) {
     return Arrays.stream(inputs)
         .collect(toImmutableMap(ActionInput::getExecPath, Function.identity()));
