[JENKINS-48405] Use NIO in tryOnceDeleteFile and makeWritable (#3169)

* Use NIO in tryOnceDeleteFile and makeWritable

* Don't try to set PosixFileAttributes on Windows

* Do not create arbitrary exceptions in makeWritable to fix test failures on Windows

* Remove unhelpful layer of exception wrapping

* Add test exercising Util#makeWritable in Util#tryOnceDeleteFile

* Add test for deleting a non-existant file

* Return early if changing permissions with NIO succeeds

core/src/main/java/hudson/Util.java

@@ -28,7 +28,6 @@ import jenkins.util.SystemProperties;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.model.TaskListener;
-import hudson.os.PosixAPI;
 import hudson.util.QuotedStringTokenizer;
 import hudson.util.VariableResolver;
 
@@ -36,16 +35,12 @@ import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.time.FastDateFormat;
 import org.apache.tools.ant.BuildException;
 import org.apache.tools.ant.Project;
-import org.apache.tools.ant.taskdefs.Chmod;
 import org.apache.tools.ant.taskdefs.Copy;
 import org.apache.tools.ant.types.FileSet;
 
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
-import jnr.posix.FileStat;
-import jnr.posix.POSIX;
-
 import javax.crypto.SecretKey;
 import javax.crypto.spec.SecretKeySpec;
 
@@ -70,6 +65,7 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.attribute.PosixFilePermission;
 import java.nio.file.attribute.BasicFileAttributes;
+import java.nio.file.attribute.PosixFileAttributes;
 import java.nio.file.attribute.DosFileAttributes;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
@@ -257,16 +253,16 @@ public class Util {
      * 
      * @param f
      *            What to delete. If a directory, it'll need to be empty.
-     * @throws IOException if it exists but could not be successfully deleted
+     * @throws IOException if it exists but could not be successfully deleted,
+     * or if it represents an invalid {@link Path}.
      */
     private static void tryOnceDeleteFile(File f) throws IOException {
-        if (!f.delete()) {
-            if(!f.exists())
-                // we are trying to delete a file that no longer exists, so this is not an error
-                return;
-
+        Path path = fileToPath(f);
+        try {
+            Files.deleteIfExists(path);
+        } catch (IOException e) {
             // perhaps this file is read-only?
-            makeWritable(f);
+            makeWritable(path);
             /*
              on Unix both the file and the directory that contains it has to be writable
              for a file deletion to be successful. (Confirmed on Solaris 9)
@@ -280,56 +276,47 @@ public class Util {
              $ rm x
              rm: x not removed: Permission denied
              */
-
-            makeWritable(f.getParentFile());
-
-            if(!f.delete() && f.exists()) {
-                // trouble-shooting.
-                try {
-                    Files.deleteIfExists(f.toPath());
-                } catch (InvalidPathException e) {
-                    throw new IOException(e);
-                }
-
+            Path parent = path.getParent();
+            if (parent != null) {
+                makeWritable(parent);
+            }
+            try {
+                Files.deleteIfExists(path);
+            } catch (IOException e2) {
                 // see https://java.net/projects/hudson/lists/users/archive/2008-05/message/357
                 // I suspect other processes putting files in this directory
                 File[] files = f.listFiles();
                 if(files!=null && files.length>0)
-                    throw new IOException("Unable to delete " + f.getPath()+" - files in dir: "+Arrays.asList(files));
-                throw new IOException("Unable to delete " + f.getPath());
+                    throw new IOException("Unable to delete " + f.getPath()+" - files in dir: "+Arrays.asList(files), e2);
+                throw e2;
             }
         }
     }
 
     /**
-     * Makes the given file writable by any means possible.
+     * Makes the file at the given path writable by any means possible.
      */
-    private static void makeWritable(@Nonnull File f) {
-        if (f.setWritable(true)) {
-            return;
-        }
-        // TODO do we still need to try anything else?
-
-        // try chmod. this becomes no-op if this is not Unix.
-        try {
-            Chmod chmod = new Chmod();
-            chmod.setProject(new Project());
-            chmod.setFile(f);
-            chmod.setPerm("u+w");
-            chmod.execute();
-        } catch (BuildException e) {
-            LOGGER.log(Level.INFO,"Failed to chmod "+f,e);
-        }
-
-        try {// try libc chmod
-            POSIX posix = PosixAPI.jnr();
-            String path = f.getAbsolutePath();
-            FileStat stat = posix.stat(path);
-            posix.chmod(path, stat.mode()|0200); // u+w
-        } catch (Throwable t) {
-            LOGGER.log(Level.FINE,"Failed to chmod(2) "+f,t);
+    private static void makeWritable(@Nonnull Path path) throws IOException {
+        if (!Functions.isWindows()) {
+            try {
+                PosixFileAttributes attrs = Files.readAttributes(path, PosixFileAttributes.class);
+                Set<PosixFilePermission> newPermissions = ((PosixFileAttributes)attrs).permissions();
+                newPermissions.add(PosixFilePermission.OWNER_WRITE);
+                Files.setPosixFilePermissions(path, newPermissions);
+                return;
+            } catch (NoSuchFileException e) {
+                return;
+            } catch (UnsupportedOperationException e) {
+                // PosixFileAttributes not supported, fall back to old IO.
+            }
         }
 
+        /**
+         * We intentionally do not check the return code of setWritable, because if it
+         * is false we prefer to rethrow the exception thrown by Files.deleteIfExists,
+         * which will have a more useful message than something we make up here.
+         */
+        path.toFile().setWritable(true);
     }
 
     /**

core/src/test/java/hudson/UtilTest.java

@@ -36,6 +36,9 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.FileSystemException;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.attribute.PosixFilePermissions;
 
 import static org.hamcrest.CoreMatchers.allOf;
@@ -287,12 +290,6 @@ public class UtilTest {
     @Test
     public void testDeleteFile_onWindows() throws Exception {
         Assume.assumeTrue(Functions.isWindows());
-        Class<?> c;
-        try {
-            c = Class.forName("java.nio.file.FileSystemException");
-        } catch (ClassNotFoundException x) {
-            throw new AssumptionViolatedException("prior to JDK 7", x);
-        }
         final int defaultDeletionMax = Util.DELETION_MAX;
         try {
             File f = tmp.newFile();
@@ -304,7 +301,7 @@ public class UtilTest {
                 Util.deleteFile(f);
                 fail("should not have been deletable");
             } catch (IOException x) {
-                assertThat(calcExceptionHierarchy(x), hasItem(c));
+                assertThat(calcExceptionHierarchy(x), hasItem(FileSystemException.class));
                 assertThat(x.getMessage(), containsString(f.getPath()));
             }
         } finally {
@@ -313,6 +310,27 @@ public class UtilTest {
         }
     }
 
+    @Test
+    public void testDeleteFileReadOnly() throws Exception {
+        // Removing the calls to Util#makeWritable in Util#tryOnceDeleteFile should cause this test to fail.
+        Path file = tmp.newFolder().toPath().resolve("file.tmp");
+        Files.createDirectories(file.getParent());
+        Files.createFile(file);
+        // Using old IO so the test can run on Windows.
+        file.getParent().toFile().setWritable(false);
+        file.toFile().setWritable(false);
+        Util.deleteFile(file.toFile());
+        assertFalse(Files.exists(file));
+    }
+
+    @Test
+    public void testDeleteFileDoesNotExist() throws Exception {
+        Path file = tmp.newFolder().toPath().resolve("file.tmp");
+        assertFalse(Files.exists(file));
+        // Should not throw an exception.
+        Util.deleteFile(file.toFile());
+    }
+
     @Test
     public void testDeleteContentsRecursive() throws Exception {
         final File dir = tmp.newFolder();