Skip to content

Commit

Permalink
[JENKINS-48405] Use NIO in tryOnceDeleteFile and makeWritable (#3169)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@dwnusbaum @oleg-nenashev
dwnusbaum authored and oleg-nenashev committed Dec 16, 2017
1 parent 3a52963 commit 1270ba3
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 57 deletions.
87 changes: 37 additions & 50 deletions core/src/main/java/hudson/Util.java
View file
Expand Up @@ -28,24 +28,19 @@

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.model.TaskListener;
import hudson.os.PosixAPI;
import hudson.util.QuotedStringTokenizer;
import hudson.util.VariableResolver;

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;

Expand All @@ -70,6 +65,7 @@
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;
Expand Down Expand Up @@ -257,16 +253,16 @@ public static void deleteFile(@Nonnull File f) throws IOException {
*
* @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)
Expand All @@ -280,56 +276,47 @@ private static void tryOnceDeleteFile(File f) throws IOException {
$ 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);
}

/**
Expand Down
32 changes: 25 additions & 7 deletions core/src/test/java/hudson/UtilTest.java
View file
Expand Up @@ -36,6 +36,9 @@
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;
Expand Down Expand Up @@ -287,12 +290,6 @@ public void testDeleteFile() throws Exception {
@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();
Expand All @@ -304,7 +301,7 @@ public void testDeleteFile_onWindows() throws Exception {
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 {
Expand All @@ -313,6 +310,27 @@ public void testDeleteFile_onWindows() throws Exception {
}
}

@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();
Expand Down

0 comments on commit 1270ba3

Please sign in to comment.