sandbox: properly add `tmpDir` to `writablePaths`

When Bazel creates the sandbox for an action,
Bazel collects a set of paths that the action may
write to.

The action needs write access to its temp
directory, so Bazel needs to add it to the
writable paths.

See https://github.com/bazelbuild/bazel/issues/4376

Change-Id: Ifd3c482aa67ff8a2070045356abad8b39c808db8
PiperOrigin-RevId: 181591520

1 files changed, 34 insertions, 26 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
index 6cead52294..8d7a3389a6 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
@@ -229,37 +229,24 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner {
    */
   protected ImmutableSet<Path> getWritableDirs(
       Path sandboxExecRoot, Map<String, String> env, Path tmpDir) throws IOException {
-    FileSystem fileSystem = sandboxExecRoot.getFileSystem();
-
     // We have to make the TEST_TMPDIR directory writable if it is specified.
     ImmutableSet.Builder<Path> writablePaths = ImmutableSet.builder();
     writablePaths.add(sandboxExecRoot);
-    String tmpDirString = env.get("TEST_TMPDIR");
-    if (tmpDirString != null) {
-      Path p = sandboxExecRoot.getRelative(tmpDirString);
-      if (p.startsWith(sandboxExecRoot)) {
-        // We add this path even though it is below sandboxExecRoot (and thus already writable as a
-        // subpath) to take advantage of the side-effect that SymlinkedExecRoot also creates this
-        // needed directory if it doesn't exist yet.
-        writablePaths.add(p);
-      } else if (p.exists()) {
-        // If `p` itself is a symlink, then adding it to `writablePaths` would result in making the
-        // symlink itself writable, not what it points to. Therefore we need to resolve symlinks in
-        // `p`, however for that we need `p` to exist.
-        writablePaths.add(p.resolveSymbolicLinks());
-      } else {
-        throw new IOException(
-            String.format(
-                "Cannot resolve symlinks in TEST_TMPDIR because it doesn't exist: \"%s\"",
-                p.getPathString()));
-      }
+    String testTmpdir = env.get("TEST_TMPDIR");
+    if (testTmpdir != null) {
+      addWritablePath(
+          sandboxExecRoot,
+          writablePaths,
+          sandboxExecRoot.getRelative(testTmpdir),
+          "Cannot resolve symlinks in TEST_TMPDIR because it doesn't exist: \"%s\"");
     }
+    addWritablePath(
+        sandboxExecRoot,
+        writablePaths,
+        tmpDir,
+        "Cannot resolve symlinks in tmpDir because it doesn't exist: \"%s\"");
 
-    // TODO(laszlocsomor): Extract the logic that adds TEST_TMPDIR to writablePaths, and add tmpDir
-    // using the same method. Currently we don't resolve symlinks in tmpDir and so we might be
-    // adding a symlink to the writable paths, and not what the symlink points to.
-    writablePaths.add(tmpDir);
-
+    FileSystem fileSystem = sandboxExecRoot.getFileSystem();
     for (String writablePath : sandboxOptions.sandboxWritablePath) {
       Path path = fileSystem.getPath(writablePath);
       writablePaths.add(path);
@@ -269,6 +256,27 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner {
     return writablePaths.build();
   }
 
+  private void addWritablePath(
+      Path sandboxExecRoot,
+      ImmutableSet.Builder<Path> writablePaths,
+      Path path,
+      String pathDoesNotExistErrorTemplate)
+      throws IOException {
+    if (path.startsWith(sandboxExecRoot)) {
+      // We add this path even though it is below sandboxExecRoot (and thus already writable as a
+      // subpath) to take advantage of the side-effect that SymlinkedExecRoot also creates this
+      // needed directory if it doesn't exist yet.
+      writablePaths.add(path);
+    } else if (path.exists()) {
+      // If `path` itself is a symlink, then adding it to `writablePaths` would result in making
+      // the symlink itself writable, not what it points to. Therefore we need to resolve symlinks
+      // in `path`, however for that we need `path` to exist.
+      writablePaths.add(path.resolveSymbolicLinks());
+    } else {
+      throw new IOException(String.format(pathDoesNotExistErrorTemplate, path.getPathString()));
+    }
+  }
+
   protected ImmutableSet<Path> getInaccessiblePaths() {
     return inaccessiblePaths;
   }