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 bazelbuild#5513

Change-Id: I5d2720afff8dfd3725880a6d7408d22de5935c3d

Closes bazelbuild#5527.

Change-Id: I5d2720afff8dfd3725880a6d7408d22de5935c3d
PiperOrigin-RevId: 203720575

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());
     }
   }
 

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",

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 =

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]));
+    }
+  }
 }