From 1270ba3b40ade4b822ee7538e31f0692ffbf64d2 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Sat, 16 Dec 2017 03:23:41 -0500 Subject: [PATCH] [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 | 87 +++++++++++-------------- core/src/test/java/hudson/UtilTest.java | 32 +++++++-- 2 files changed, 62 insertions(+), 57 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 8ee5c7390a..1e13ea9957 100644 --- a/core/src/main/java/hudson/Util.java +++ b/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 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); } /** diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index 21a8993de4..f1cf614055 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/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();