Repository: bazelbuild/bazel
Issue: Bazel server: race condition in JavaIoFileSystem.delete()
Labels: type: bug

### Description of the problem / feature request:

There's a race condition in: https://github.com/bazelbuild/bazel/blob/a320e4460f35767da7fc8eb824ce1f4b304b5e0f/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java#L353

If the directory is deleted or replaced by a file between the `file.isDirectory()` check and the `file.list()` call, then `file.list()` returns null and throws an NPE.

There may be other such race conditions in Bazel's VFS library.

### What's the output of `bazel info release`?

release 0.15.0

Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
index ea541b4a1e020a..25fafac67bb3f7 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
@@ -343,22 +343,42 @@ protected long getFileSize(Path path, boolean followSymlinks) throws IOException
 
   @Override
   public boolean delete(Path path) throws IOException {
-    File file = getIoFile(path);
+    java.nio.file.Path nioPath = getNioPath(path);
     long startTime = Profiler.nanoTimeMaybe();
     try {
-      if (file.delete()) {
-        return true;
-      }
-      if (file.exists()) {
-        if (file.isDirectory() && file.list().length > 0) {
-          throw new IOException(path + ERR_DIRECTORY_NOT_EMPTY);
-        } else {
-          throw new IOException(path + ERR_PERMISSION_DENIED);
-        }
+      return Files.deleteIfExists(nioPath);
+    } catch (java.nio.file.DirectoryNotEmptyException e) {
+      throw new IOException(path.getPathString() + ERR_DIRECTORY_NOT_EMPTY);
+    } catch (java.nio.file.AccessDeniedException e) {
+      throw new IOException(path.getPathString() + ERR_PERMISSION_DENIED);
+    } catch (java.nio.file.AtomicMoveNotSupportedException
+        | java.nio.file.FileAlreadyExistsException
+        | java.nio.file.FileSystemLoopException
+        | java.nio.file.NoSuchFileException
+        | java.nio.file.NotDirectoryException
+        | java.nio.file.NotLinkException e) {
+      // All known but unexpected subclasses of FileSystemException.
+      throw new IOException(path.getPathString() + ": unexpected FileSystemException", e);
+    } catch (java.nio.file.FileSystemException e) {
+      // Files.deleteIfExists() throws FileSystemException on Linux if a path component is a file.
+      // We caught all known subclasses of FileSystemException so `e` is either an unknown
+      // subclass or it is indeed a "Not a directory" error. Non-English JDKs may use a different
+      // error message than "Not a directory", so we should not look for that text. Checking the
+      // parent directory if it's indeed a directory is unrealiable, because another process may
+      // modify it concurrently... but we have no better choice.
+      if (e.getClass().equals(java.nio.file.FileSystemException.class)
+          && !nioPath.getParent().toFile().isDirectory()) {
+        // Hopefully the try-block failed because a parent directory was in fact not a directory.
+        // Theoretically it's possible that the try-block failed for some other reason and all
+        // parent directories were indeed directories, but another process changed a parent
+        // directory into a file after the try-block failed but before this catch-block started, and
+        // we return false here losing the real exception in `e`, but we cannot know.
+        return false;
+      } else {
+        throw new IOException(path.getPathString() + ": unexpected FileSystemException", e);
       }
-      return false;
     } finally {
-      profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, file.getPath());
+      profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, path.getPathString());
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/windows/BUILD b/src/main/java/com/google/devtools/build/lib/windows/BUILD
index 4b6e08c73a3285..61eb83acee2bcf 100644
--- a/src/main/java/com/google/devtools/build/lib/windows/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/windows/BUILD
@@ -33,6 +33,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:os_util",
         "//src/main/java/com/google/devtools/build/lib/clock",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/windows/jni",
diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java
index 0fd7028d6b7b62..869ba85d627039 100644
--- a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java
@@ -15,6 +15,8 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.profiler.Profiler;
+import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.JavaIoFileSystem;
@@ -48,6 +50,20 @@ public String getFileSystemType(Path path) {
     return "ntfs";
   }
 
+  @Override
+  public boolean delete(Path path) throws IOException {
+    long startTime = Profiler.nanoTimeMaybe();
+    try {
+      return WindowsFileOperations.deletePath(path.getPathString());
+    } catch (java.nio.file.DirectoryNotEmptyException e) {
+      throw new IOException(path.getPathString() + ERR_DIRECTORY_NOT_EMPTY);
+    } catch (java.nio.file.AccessDeniedException e) {
+      throw new IOException(path.getPathString() + ERR_PERMISSION_DENIED);
+    } finally {
+      profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, path.getPathString());
+    }
+  }
+
   @Override
   protected void createSymbolicLink(Path linkPath, PathFragment targetFragment) throws IOException {
     Path targetPath =
diff --git a/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java b/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java
index 32868c204db75a..6a16ab8aab7668 100644
--- a/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java
+++ b/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java
@@ -49,12 +49,21 @@ private WindowsFileOperations() {
   private static final int IS_JUNCTION_NO = 1;
   private static final int IS_JUNCTION_ERROR = 2;
 
+  // Keep DELETE_PATH_* values in sync with src/main/native/windows/file.cc.
+  private static final int DELETE_PATH_SUCCESS = 0;
+  private static final int DELETE_PATH_DOES_NOT_EXIST = 1;
+  private static final int DELETE_PATH_DIRECTORY_NOT_EMPTY = 2;
+  private static final int DELETE_PATH_ACCESS_DENIED = 3;
+  private static final int DELETE_PATH_ERROR = 4;
+
   private static native int nativeIsJunction(String path, String[] error);
 
   private static native boolean nativeGetLongPath(String path, String[] result, String[] error);
 
   private static native boolean nativeCreateJunction(String name, String target, String[] error);
 
+  private static native int nativeDeletePath(String path, String[] error);
+
   /** Determines whether `path` is a junction point or directory symlink. */
   public static boolean isJunction(String path) throws IOException {
     WindowsJniLoader.loadJni();
@@ -121,4 +130,22 @@ public static void createJunction(String name, String target) throws IOException
           String.format("Cannot create junction (name=%s, target=%s): %s", name, target, error[0]));
     }
   }
+
+  public static boolean deletePath(String path) throws IOException {
+    WindowsJniLoader.loadJni();
+    String[] error = new String[] {null};
+    int result = nativeDeletePath(asLongPath(path), error);
+    switch (result) {
+      case DELETE_PATH_SUCCESS:
+        return true;
+      case DELETE_PATH_DOES_NOT_EXIST:
+        return false;
+      case DELETE_PATH_DIRECTORY_NOT_EMPTY:
+        throw new java.nio.file.DirectoryNotEmptyException(path);
+      case DELETE_PATH_ACCESS_DENIED:
+        throw new java.nio.file.AccessDeniedException(path);
+      default:
+        throw new IOException(String.format("Cannot delete path '%s': %s", path, error[0]));
+    }
+  }
 }


Test patch: