From 89a3d5e1ba7db769a2998b901a919a942ec5879d Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Tue, 11 Apr 2023 11:15:50 -0700
Subject: [PATCH] Report failures to add permissions with JavaIoFileSystem

Previously the return value of the `File#setX` functions was not
checked, thus masking potential errors. Since these functions are
expected to fail on Windows if asked to remove a permission, errors are
only reported when attempting to add one.

Also removes misleading `throws` declarations that may mask similar
issues of this type.

Closes #18004.

PiperOrigin-RevId: 523454138
Change-Id: Ia85db5e912d07af1ca6f7cd4b8e2d784395e7811
---
 .../build/lib/vfs/JavaIoFileSystem.java       | 99 ++++++++++---------
 .../build/lib/windows/WindowsFileSystem.java  |  4 +-
 2 files changed, 55 insertions(+), 48 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 a3e1c2c0cb3d33..9311eeba1ed92b 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
@@ -176,7 +176,9 @@ protected void setReadable(PathFragment path, boolean readable) throws IOExcepti
     if (!file.exists()) {
       throw new FileNotFoundException(path + ERR_NO_SUCH_FILE_OR_DIR);
     }
-    file.setReadable(readable);
+    if (!file.setReadable(readable) && readable) {
+      throw new IOException(String.format("Failed to make %s readable", path));
+    }
   }
 
   @Override
@@ -185,7 +187,9 @@ public void setWritable(PathFragment path, boolean writable) throws IOException
     if (!file.exists()) {
       throw new FileNotFoundException(path + ERR_NO_SUCH_FILE_OR_DIR);
     }
-    file.setWritable(writable);
+    if (!file.setWritable(writable) && writable) {
+      throw new IOException(String.format("Failed to make %s writable", path));
+    }
   }
 
   @Override
@@ -194,7 +198,9 @@ protected void setExecutable(PathFragment path, boolean executable) throws IOExc
     if (!file.exists()) {
       throw new FileNotFoundException(path + ERR_NO_SUCH_FILE_OR_DIR);
     }
-    file.setExecutable(executable);
+    if (!file.setExecutable(executable) && executable) {
+      throw new IOException(String.format("Failed to make %s executable", path));
+    }
   }
 
   @Override
@@ -442,49 +448,50 @@ protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOEx
     } catch (java.nio.file.FileSystemException e) {
       throw new FileNotFoundException(path + ERR_NO_SUCH_FILE_OR_DIR);
     }
-    FileStatus status =  new FileStatus() {
-      @Override
-      public boolean isFile() {
-        return attributes.isRegularFile() || isSpecialFile();
-      }
-
-      @Override
-      public boolean isSpecialFile() {
-        return attributes.isOther();
-      }
-
-      @Override
-      public boolean isDirectory() {
-        return attributes.isDirectory();
-      }
-
-      @Override
-      public boolean isSymbolicLink() {
-        return attributes.isSymbolicLink();
-      }
-
-      @Override
-      public long getSize() throws IOException {
-        return attributes.size();
-      }
-
-      @Override
-      public long getLastModifiedTime() throws IOException {
-        return attributes.lastModifiedTime().toMillis();
-      }
-
-      @Override
-      public long getLastChangeTime() {
-        // This is the best we can do with Java NIO...
-        return attributes.lastModifiedTime().toMillis();
-      }
-
-      @Override
-      public long getNodeId() {
-        // TODO(bazel-team): Consider making use of attributes.fileKey().
-        return -1;
-      }
-    };
+    FileStatus status =
+        new FileStatus() {
+          @Override
+          public boolean isFile() {
+            return attributes.isRegularFile() || isSpecialFile();
+          }
+
+          @Override
+          public boolean isSpecialFile() {
+            return attributes.isOther();
+          }
+
+          @Override
+          public boolean isDirectory() {
+            return attributes.isDirectory();
+          }
+
+          @Override
+          public boolean isSymbolicLink() {
+            return attributes.isSymbolicLink();
+          }
+
+          @Override
+          public long getSize() {
+            return attributes.size();
+          }
+
+          @Override
+          public long getLastModifiedTime() {
+            return attributes.lastModifiedTime().toMillis();
+          }
+
+          @Override
+          public long getLastChangeTime() {
+            // This is the best we can do with Java NIO...
+            return attributes.lastModifiedTime().toMillis();
+          }
+
+          @Override
+          public long getNodeId() {
+            // TODO(bazel-team): Consider making use of attributes.fileKey().
+            return -1;
+          }
+        };
 
     return status;
   }
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 5055817ba5b17b..a9ea39d1b118db 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
@@ -182,12 +182,12 @@ public boolean isSymbolicLink() {
           }
 
           @Override
-          public long getSize() throws IOException {
+          public long getSize() {
             return attributes.size();
           }
 
           @Override
-          public long getLastModifiedTime() throws IOException {
+          public long getLastModifiedTime() {
             return attributes.lastModifiedTime().toMillis();
           }