vfs: fix race condition in JavaIoFileSystem.delete

JavaIoFileSystem.delete() now uses
Files.deleteIfExists() to delete files and empty
directories.

The old code used to have a race condition: a
file could have been deleted or changed to a
non-directory between checking it's a directory
and listing its contents.

WindowsFileSystem.delete() now uses the
DeletePath JNI method (wrapped by
WindowsFileOperations.deletePath) which is more
robust than JavaIoFileSystem.delete(), because it
can delete readonly files and can tolerate some
concurrent filesystem modifications.

Fixes https://github.com/bazelbuild/bazel/issues/5513

Change-Id: I5d2720afff8dfd3725880a6d7408d22de5935c3d

Closes #5527.

Change-Id: I5d2720afff8dfd3725880a6d7408d22de5935c3d
PiperOrigin-RevId: 203720575

4 files changed, 76 insertions, 12 deletions

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 ea541b4a1e..25fafac67b 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 @@ public class JavaIoFileSystem extends AbstractFileSystemWithCustomStat {
 
   @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 4b6e08c73a..61eb83acee 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 0fd7028d6b..869ba85d62 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 @@ package com.google.devtools.build.lib.windows;
 
 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;
@@ -49,6 +51,20 @@ public class WindowsFileSystem extends JavaIoFileSystem {
   }
 
   @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 =
         targetFragment.isAbsolute()
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 32868c204d..6a16ab8aab 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 @@ public class 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 class WindowsFileOperations {
           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]));
+    }
+  }
 }