From 4c6160d710f903884d6268caed14794bccb5372a Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 10 Jan 2019 10:58:01 -0600 Subject: [PATCH 01/14] Add test for deleting a symlink Signed-off-by: Matt Sicker --- .../java/jenkins/util/io/PathRemoverTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/core/src/test/java/jenkins/util/io/PathRemoverTest.java b/core/src/test/java/jenkins/util/io/PathRemoverTest.java index 55c7970424..b89607bb44 100644 --- a/core/src/test/java/jenkins/util/io/PathRemoverTest.java +++ b/core/src/test/java/jenkins/util/io/PathRemoverTest.java @@ -36,6 +36,8 @@ import java.io.FileWriter; import java.io.IOException; import java.nio.file.FileSystem; import java.nio.file.FileSystemException; +import java.nio.file.Files; +import java.nio.file.LinkOption; import java.nio.file.Path; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.spi.FileSystemProvider; @@ -140,6 +142,19 @@ public class PathRemoverTest { assertFalse("Unable to delete file: " + file, file.exists()); } + @Test + public void testForceRemoveFile_SymbolicLink() throws IOException { + File file = tmp.newFile(); + touchWithFileName(file); + Path link = Files.createSymbolicLink(tmp.getRoot().toPath().resolve("test-link"), file.toPath()); + + PathRemover remover = PathRemover.newSimpleRemover(); + remover.forceRemoveFile(link); + + assertTrue("Unable to delete symbolic link: " + link, Files.notExists(link, LinkOption.NOFOLLOW_LINKS)); + assertTrue("Should not have deleted target file: " + file, file.exists()); + } + @Test public void testForceRemoveDirectoryContents() throws IOException { File dir = tmp.newFolder(); From c05d00ad69deb165b9bec7477036cd0e828f3eb3 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 10 Jan 2019 11:00:54 -0600 Subject: [PATCH 02/14] Update FilePath.delete* to use Util.delete* This reduces duplication of file deletion strategy code and potentially fixes a regression noted in JENKINS-55448. Signed-off-by: Matt Sicker --- core/src/main/java/hudson/FilePath.java | 27 ++----------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index 3ad30c4ac9..2f3c08fcba 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -1271,7 +1271,7 @@ public final class FilePath implements Serializable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { - deleteRecursive(deleting(f)); + Util.deleteRecursive(deleting(f)); return null; } } @@ -1286,34 +1286,11 @@ public final class FilePath implements Serializable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { - deleteContentsRecursive(f); + Util.deleteContentsRecursive(deleting(f)); return null; } } - private void deleteRecursive(File dir) throws IOException { - if(!isSymlink(dir)) - deleteContentsRecursive(dir); - try { - deleteFile(deleting(dir)); - } catch (IOException e) { - // if some of the child directories are big, it might take long enough to delete that - // it allows others to create new files, causing problems like JENKINS-10113 - // so give it one more attempt before we give up. - if(!isSymlink(dir)) - deleteContentsRecursive(dir); - deleteFile(deleting(dir)); - } - } - - private void deleteContentsRecursive(File file) throws IOException { - File[] files = file.listFiles(); - if(files==null) - return; // the directory didn't exist in the first place - for (File child : files) - deleteRecursive(child); - } - /** * Gets the file name portion except the extension. * From 5dfef2bd17e592ae245938cf90a3ca575246a20f Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 10 Jan 2019 11:48:03 -0600 Subject: [PATCH 03/14] Add test for deleting symlinks in descendant files Signed-off-by: Matt Sicker --- .../java/jenkins/util/io/PathRemoverTest.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/core/src/test/java/jenkins/util/io/PathRemoverTest.java b/core/src/test/java/jenkins/util/io/PathRemoverTest.java index b89607bb44..b2f673ee86 100644 --- a/core/src/test/java/jenkins/util/io/PathRemoverTest.java +++ b/core/src/test/java/jenkins/util/io/PathRemoverTest.java @@ -42,6 +42,7 @@ import java.nio.file.Path; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.spi.FileSystemProvider; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -324,6 +325,27 @@ public class PathRemoverTest { assertNotNull(interrupted.get()); } + @Test + public void testForceRemoveRecursive_ContainsSymbolicLinks() throws IOException { + File folder = tmp.newFolder(); + File d1 = new File(folder, "d1"); + File d1f1 = new File(d1, "d1f1"); + File f2 = new File(folder, "f2"); + mkdirs(d1); + touchWithFileName(d1f1, f2); + Path path = tmp.newFolder().toPath(); + Files.createSymbolicLink(path.resolve("sym-dir"), d1.toPath()); + Files.createSymbolicLink(path.resolve("sym-file"), f2.toPath()); + + PathRemover remover = PathRemover.newSimpleRemover(); + remover.forceRemoveRecursive(path); + + assertTrue("Unable to delete directory: " + path, Files.notExists(path)); + for (File file : Arrays.asList(d1, d1f1, f2)) { + assertTrue("Should not have deleted target: " + file, file.exists()); + } + } + private static void mkdirs(File... dirs) { for (File dir : dirs) { assertTrue("Could not mkdir " + dir, dir.mkdir()); From 301ba7af51d854a009705509d834327d132e2071 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 10 Jan 2019 12:27:16 -0600 Subject: [PATCH 04/14] [JENKINS-55448] Fix Util.delete regression This adds path normalization behavior back to the delete methods to match the behavior of the previous version of these methods. Signed-off-by: Matt Sicker --- .../java/jenkins/util/io/PathRemover.java | 16 +++++++++------- .../java/jenkins/util/io/PathRemoverTest.java | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/jenkins/util/io/PathRemover.java b/core/src/main/java/jenkins/util/io/PathRemover.java index 763f8f8589..7474399277 100644 --- a/core/src/main/java/jenkins/util/io/PathRemover.java +++ b/core/src/main/java/jenkins/util/io/PathRemover.java @@ -188,7 +188,7 @@ public class PathRemover { private static Optional tryRemoveFile(@Nonnull Path path) { try { - removeOrMakeRemovableThenRemove(path); + removeOrMakeRemovableThenRemove(path.normalize()); return Optional.empty(); } catch (IOException e) { return Optional.of(e); @@ -196,16 +196,18 @@ public class PathRemover { } private static List tryRemoveRecursive(@Nonnull Path path) { - List accumulatedErrors = Util.isSymlink(path) ? new ArrayList<>() : - tryRemoveDirectoryContents(path); - tryRemoveFile(path).ifPresent(accumulatedErrors::add); + Path normalized = path.normalize(); + List accumulatedErrors = Util.isSymlink(normalized) ? new ArrayList<>() : + tryRemoveDirectoryContents(normalized); + tryRemoveFile(normalized).ifPresent(accumulatedErrors::add); return accumulatedErrors; } private static List tryRemoveDirectoryContents(@Nonnull Path path) { + Path normalized = path.normalize(); List accumulatedErrors = new ArrayList<>(); - if (!Files.isDirectory(path)) return accumulatedErrors; - try (DirectoryStream children = Files.newDirectoryStream(path)) { + if (!Files.isDirectory(normalized)) return accumulatedErrors; + try (DirectoryStream children = Files.newDirectoryStream(normalized)) { for (Path child : children) { accumulatedErrors.addAll(tryRemoveRecursive(child)); } @@ -254,7 +256,7 @@ public class PathRemover { $ rm x rm: x not removed: Permission denied */ - Path parent = path.getParent(); + Path parent = path.getParent().normalize(); if (parent != null && !Files.isWritable(parent)) { makeWritable(parent); } diff --git a/core/src/test/java/jenkins/util/io/PathRemoverTest.java b/core/src/test/java/jenkins/util/io/PathRemoverTest.java index b2f673ee86..747775531d 100644 --- a/core/src/test/java/jenkins/util/io/PathRemoverTest.java +++ b/core/src/test/java/jenkins/util/io/PathRemoverTest.java @@ -30,6 +30,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.rules.TemporaryFolder; import org.junit.rules.Timeout; +import org.jvnet.hudson.test.Issue; import java.io.File; import java.io.FileWriter; @@ -39,6 +40,7 @@ import java.nio.file.FileSystemException; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.spi.FileSystemProvider; import java.util.ArrayList; @@ -346,6 +348,23 @@ public class PathRemoverTest { } } + @Test + @Issue("JENKINS-55448") + public void testForceRemoveRecursive_ContainsDotPath() throws IOException { + File folder = tmp.newFolder(); + File d1 = new File(folder, "d1"); + File d1f1 = new File(d1, "d1f1"); + File f2 = new File(folder, "f2"); + mkdirs(d1); + touchWithFileName(d1f1, f2); + Path path = Paths.get(d1.getPath(), "..", "d1"); + + PathRemover remover = PathRemover.newSimpleRemover(); + remover.forceRemoveRecursive(path); + + assertTrue("Unable to delete directory: " + folder, Files.notExists(path)); + } + private static void mkdirs(File... dirs) { for (File dir : dirs) { assertTrue("Could not mkdir " + dir, dir.mkdir()); From 59880c2fcde9581f20b5876fadb6927ae5027d3d Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 10 Jan 2019 13:40:17 -0600 Subject: [PATCH 05/14] Add path filter support to PathRemover This allows for simplifying FilePath to use PathRemover rather than duplicating logic. Signed-off-by: Matt Sicker --- core/src/main/java/hudson/FilePath.java | 4 +-- core/src/main/java/hudson/Util.java | 33 ++++++++++++++++--- .../java/jenkins/util/io/PathRemover.java | 28 +++++++++------- .../java/jenkins/util/io/PathRemoverTest.java | 1 + 4 files changed, 47 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index 2f3c08fcba..fb6a09a2d5 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -1271,7 +1271,7 @@ public final class FilePath implements Serializable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { - Util.deleteRecursive(deleting(f)); + Util.deleteRecursive(fileToPath(f), path -> deleting(path.toFile()).delete()); return null; } } @@ -1286,7 +1286,7 @@ public final class FilePath implements Serializable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { - Util.deleteContentsRecursive(deleting(f)); + Util.deleteContentsRecursive(fileToPath(f), path -> deleting(path.toFile()).delete()); return null; } } diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 5c258d9469..3e857f481c 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -75,6 +75,7 @@ import java.time.temporal.ChronoUnit; import java.util.*; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Predicate; import java.util.logging.Level; import java.util.logging.LogRecord; import java.util.logging.Logger; @@ -241,7 +242,18 @@ public class Util { * if the operation fails. */ public static void deleteContentsRecursive(@Nonnull File file) throws IOException { - newPathRemover().forceRemoveDirectoryContents(fileToPath(file)); + deleteContentsRecursive(fileToPath(file), path -> true); + } + + /** + * Deletes the given directory contents (but not the directory itself) recursively using a filter. + * @param path a directory to delete + * @param pathFilter a predicate that when evaluated to true will delete that path; when false, will ignore that path + * @throws IOException if the operation fails + */ + @Restricted(NoExternalUse.class) + public static void deleteContentsRecursive(@Nonnull Path path, @Nonnull Predicate pathFilter) throws IOException { + newPathRemover(pathFilter).forceRemoveDirectoryContents(path); } /** @@ -252,7 +264,7 @@ public class Util { * @throws IOException if it exists but could not be successfully deleted */ public static void deleteFile(@Nonnull File f) throws IOException { - newPathRemover().forceRemoveFile(fileToPath(f)); + newPathRemover(path -> true).forceRemoveFile(fileToPath(f)); } /** @@ -264,7 +276,18 @@ public class Util { * if the operation fails. */ public static void deleteRecursive(@Nonnull File dir) throws IOException { - newPathRemover().forceRemoveRecursive(fileToPath(dir)); + deleteRecursive(fileToPath(dir), path -> true); + } + + /** + * Deletes the given directory and contents recursively using a filter. + * @param dir a directory to delete + * @param pathFilter a predicate that when evaluated to true will delete that path; when false, will ignore that path + * @throws IOException if the operation fails + */ + @Restricted(NoExternalUse.class) + public static void deleteRecursive(@Nonnull Path dir, @Nonnull Predicate pathFilter) throws IOException { + newPathRemover(pathFilter).forceRemoveRecursive(dir); } /* @@ -1560,8 +1583,8 @@ public class Util { @Restricted(value = NoExternalUse.class) static boolean GC_AFTER_FAILED_DELETE = SystemProperties.getBoolean(Util.class.getName() + ".performGCOnFailedDelete"); - private static PathRemover newPathRemover() { - return PathRemover.newRobustRemover(DELETION_MAX - 1, GC_AFTER_FAILED_DELETE, WAIT_BETWEEN_DELETION_RETRIES); + private static PathRemover newPathRemover(@Nonnull Predicate pathFilter) { + return PathRemover.newFilteredRobustRemover(pathFilter, DELETION_MAX - 1, GC_AFTER_FAILED_DELETE, WAIT_BETWEEN_DELETION_RETRIES); } /** diff --git a/core/src/main/java/jenkins/util/io/PathRemover.java b/core/src/main/java/jenkins/util/io/PathRemover.java index 7474399277..789c9b4d79 100644 --- a/core/src/main/java/jenkins/util/io/PathRemover.java +++ b/core/src/main/java/jenkins/util/io/PathRemover.java @@ -42,6 +42,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -49,21 +50,23 @@ import java.util.stream.Stream; public class PathRemover { public static PathRemover newSimpleRemover() { - return new PathRemover(ignored -> false); + return new PathRemover(ignored -> false, ignored -> true); } public static PathRemover newRemoverWithStrategy(@Nonnull RetryStrategy retryStrategy) { - return new PathRemover(retryStrategy); + return new PathRemover(retryStrategy, ignored -> true); } - public static PathRemover newRobustRemover(int maxRetries, boolean gcAfterFailedRemove, long waitBetweenRetries) { - return new PathRemover(new PausingGCRetryStrategy(maxRetries < 1 ? 1 : maxRetries, gcAfterFailedRemove, waitBetweenRetries)); + public static PathRemover newFilteredRobustRemover(@Nonnull Predicate pathFilter, int maxRetries, boolean gcAfterFailedRemove, long waitBetweenRetries) { + return new PathRemover(new PausingGCRetryStrategy(maxRetries < 1 ? 1 : maxRetries, gcAfterFailedRemove, waitBetweenRetries), pathFilter); } private final RetryStrategy retryStrategy; + private final Predicate pathFilter; - private PathRemover(@Nonnull RetryStrategy retryStrategy) { + private PathRemover(@Nonnull RetryStrategy retryStrategy, @Nonnull Predicate pathFilter) { this.retryStrategy = retryStrategy; + this.pathFilter = pathFilter; } public void forceRemoveFile(@Nonnull Path path) throws IOException { @@ -186,7 +189,7 @@ public class PathRemover { } } - private static Optional tryRemoveFile(@Nonnull Path path) { + private Optional tryRemoveFile(@Nonnull Path path) { try { removeOrMakeRemovableThenRemove(path.normalize()); return Optional.empty(); @@ -195,7 +198,7 @@ public class PathRemover { } } - private static List tryRemoveRecursive(@Nonnull Path path) { + private List tryRemoveRecursive(@Nonnull Path path) { Path normalized = path.normalize(); List accumulatedErrors = Util.isSymlink(normalized) ? new ArrayList<>() : tryRemoveDirectoryContents(normalized); @@ -203,7 +206,7 @@ public class PathRemover { return accumulatedErrors; } - private static List tryRemoveDirectoryContents(@Nonnull Path path) { + private List tryRemoveDirectoryContents(@Nonnull Path path) { Path normalized = path.normalize(); List accumulatedErrors = new ArrayList<>(); if (!Files.isDirectory(normalized)) return accumulatedErrors; @@ -217,7 +220,8 @@ public class PathRemover { return accumulatedErrors; } - private static void removeOrMakeRemovableThenRemove(@Nonnull Path path) throws IOException { + private void removeOrMakeRemovableThenRemove(@Nonnull Path path) throws IOException { + if (!pathFilter.test(path)) return; try { Files.deleteIfExists(path); } catch (IOException e) { @@ -256,9 +260,9 @@ public class PathRemover { $ rm x rm: x not removed: Permission denied */ - Path parent = path.getParent().normalize(); - if (parent != null && !Files.isWritable(parent)) { - makeWritable(parent); + Optional maybeParent = Optional.ofNullable(path.getParent()).map(Path::normalize).filter(p -> !Files.isWritable(p)); + if (maybeParent.isPresent()) { + makeWritable(maybeParent.get()); } } diff --git a/core/src/test/java/jenkins/util/io/PathRemoverTest.java b/core/src/test/java/jenkins/util/io/PathRemoverTest.java index 747775531d..fbfdd45a56 100644 --- a/core/src/test/java/jenkins/util/io/PathRemoverTest.java +++ b/core/src/test/java/jenkins/util/io/PathRemoverTest.java @@ -91,6 +91,7 @@ public class PathRemoverTest { given(path.toString()).willReturn(filename); given(path.toFile()).willReturn(file); given(path.getFileSystem()).willReturn(fs); + given(path.normalize()).willReturn(path); given(fs.provider()).willReturn(fsProvider); given(fsProvider.deleteIfExists(path)).willThrow(new FileSystemException(filename)); given(fsProvider.readAttributes(path, BasicFileAttributes.class)).willReturn(attributes); From 88b515d4bfa9f8b2654edb4fabc2c1d2754e1052 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 10 Jan 2019 13:54:14 -0600 Subject: [PATCH 06/14] Revert Util.isSymlink behavior The simplified form that this replaces has an issue where a file contained somewhere inside a symbolic link is also considered a symbolic link on Windows. Signed-off-by: Matt Sicker --- core/src/main/java/hudson/Util.java | 20 ++++++++++++++----- .../java/jenkins/util/io/PathRemoverTest.java | 19 ++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 3e857f481c..fefb1171ca 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -60,8 +60,11 @@ import java.nio.file.FileSystemException; import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.InvalidPathException; +import java.nio.file.LinkOption; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.BasicFileAttributes; +import java.nio.file.attribute.DosFileAttributes; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; import java.security.DigestInputStream; @@ -316,14 +319,21 @@ public class Util { @Restricted(NoExternalUse.class) public static boolean isSymlink(@Nonnull Path path) { - if (Files.isSymbolicLink(path)) return true; /* - In Windows, a directory junction is not considered a symbolic link despite being nearly the same feature. - To avoid relying directly on Windows-specific filesystem API calls, we can instead determine if a path is - logically a symbolic link by comparing its absolute path to its real path. + * Windows Directory Junctions are effectively the same as Linux symlinks to directories. + * Unfortunately, the Java 7 NIO2 API function isSymbolicLink does not treat them as such. + * It thinks of them as normal directories. To use the NIO2 API & treat it like a symlink, + * you have to go through BasicFileAttributes and do the following check: + * isSymbolicLink() || isOther() + * The isOther() call will include Windows reparse points, of which a directory junction is. + * It also includes includes devices, but reading the attributes of a device with NIO fails + * or returns false for isOther(). (i.e. named pipes such as \\.\pipe\JenkinsTestPipe return + * false for isOther(), and drives such as \\.\PhysicalDrive0 throw an exception when + * calling readAttributes. */ try { - return path.toAbsolutePath().compareTo(path.toRealPath()) != 0; + BasicFileAttributes attrs = Files.readAttributes(path, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS); + return attrs.isSymbolicLink() || (attrs instanceof DosFileAttributes && attrs.isOther()); } catch (IOException ignored) { return false; } diff --git a/core/src/test/java/jenkins/util/io/PathRemoverTest.java b/core/src/test/java/jenkins/util/io/PathRemoverTest.java index fbfdd45a56..4a40684c0d 100644 --- a/core/src/test/java/jenkins/util/io/PathRemoverTest.java +++ b/core/src/test/java/jenkins/util/io/PathRemoverTest.java @@ -366,6 +366,25 @@ public class PathRemoverTest { assertTrue("Unable to delete directory: " + folder, Files.notExists(path)); } + @Test + @Issue("JENKINS-55448") + public void testForceRemoveRecursive_ParentIsSymbolicLink() throws IOException { + File folder = tmp.newFolder(); + File d1 = new File(folder, "d1"); + File d1f1 = new File(d1, "d1f1"); + File f2 = new File(folder, "f2"); + mkdirs(d1); + touchWithFileName(d1f1, f2); + Path symlink = Files.createSymbolicLink(tmp.getRoot().toPath().resolve("linked"), folder.toPath()); + Path d1p = symlink.resolve("d1"); + + PathRemover remover = PathRemover.newSimpleRemover(); + remover.forceRemoveRecursive(d1p); + + assertTrue("Unable to delete directory: " + d1p, Files.notExists(d1p)); + assertFalse(d1.exists()); + } + private static void mkdirs(File... dirs) { for (File dir : dirs) { assertTrue("Could not mkdir " + dir, dir.mkdir()); From 84b1193378d399d1e32017c80b4afabd316c6c3f Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 10 Jan 2019 14:10:32 -0600 Subject: [PATCH 07/14] Use explicit functional interface for path checking Signed-off-by: Matt Sicker --- core/src/main/java/hudson/FilePath.java | 4 +-- core/src/main/java/hudson/Util.java | 24 ++++++++--------- .../java/jenkins/util/io/PathRemover.java | 27 ++++++++++++------- 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index fb6a09a2d5..15d87c2b11 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -1271,7 +1271,7 @@ public final class FilePath implements Serializable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { - Util.deleteRecursive(fileToPath(f), path -> deleting(path.toFile()).delete()); + Util.deleteRecursive(fileToPath(f), path -> deleting(path.toFile())); return null; } } @@ -1286,7 +1286,7 @@ public final class FilePath implements Serializable { private static final long serialVersionUID = 1L; @Override public Void invoke(File f, VirtualChannel channel) throws IOException { - Util.deleteContentsRecursive(fileToPath(f), path -> deleting(path.toFile()).delete()); + Util.deleteContentsRecursive(fileToPath(f), path -> deleting(path.toFile())); return null; } } diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index fefb1171ca..45d8f3f5e2 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -245,18 +245,18 @@ public class Util { * if the operation fails. */ public static void deleteContentsRecursive(@Nonnull File file) throws IOException { - deleteContentsRecursive(fileToPath(file), path -> true); + deleteContentsRecursive(fileToPath(file), PathRemover.PathChecker.ALLOW_ALL); } /** - * Deletes the given directory contents (but not the directory itself) recursively using a filter. + * Deletes the given directory contents (but not the directory itself) recursively using a PathChecker. * @param path a directory to delete - * @param pathFilter a predicate that when evaluated to true will delete that path; when false, will ignore that path + * @param pathChecker a security check to validate a path before deleting * @throws IOException if the operation fails */ @Restricted(NoExternalUse.class) - public static void deleteContentsRecursive(@Nonnull Path path, @Nonnull Predicate pathFilter) throws IOException { - newPathRemover(pathFilter).forceRemoveDirectoryContents(path); + public static void deleteContentsRecursive(@Nonnull Path path, @Nonnull PathRemover.PathChecker pathChecker) throws IOException { + newPathRemover(pathChecker).forceRemoveDirectoryContents(path); } /** @@ -267,7 +267,7 @@ public class Util { * @throws IOException if it exists but could not be successfully deleted */ public static void deleteFile(@Nonnull File f) throws IOException { - newPathRemover(path -> true).forceRemoveFile(fileToPath(f)); + newPathRemover(PathRemover.PathChecker.ALLOW_ALL).forceRemoveFile(fileToPath(f)); } /** @@ -279,18 +279,18 @@ public class Util { * if the operation fails. */ public static void deleteRecursive(@Nonnull File dir) throws IOException { - deleteRecursive(fileToPath(dir), path -> true); + deleteRecursive(fileToPath(dir), PathRemover.PathChecker.ALLOW_ALL); } /** * Deletes the given directory and contents recursively using a filter. * @param dir a directory to delete - * @param pathFilter a predicate that when evaluated to true will delete that path; when false, will ignore that path + * @param pathChecker a security check to validate a path before deleting * @throws IOException if the operation fails */ @Restricted(NoExternalUse.class) - public static void deleteRecursive(@Nonnull Path dir, @Nonnull Predicate pathFilter) throws IOException { - newPathRemover(pathFilter).forceRemoveRecursive(dir); + public static void deleteRecursive(@Nonnull Path dir, @Nonnull PathRemover.PathChecker pathChecker) throws IOException { + newPathRemover(pathChecker).forceRemoveRecursive(dir); } /* @@ -1593,8 +1593,8 @@ public class Util { @Restricted(value = NoExternalUse.class) static boolean GC_AFTER_FAILED_DELETE = SystemProperties.getBoolean(Util.class.getName() + ".performGCOnFailedDelete"); - private static PathRemover newPathRemover(@Nonnull Predicate pathFilter) { - return PathRemover.newFilteredRobustRemover(pathFilter, DELETION_MAX - 1, GC_AFTER_FAILED_DELETE, WAIT_BETWEEN_DELETION_RETRIES); + private static PathRemover newPathRemover(@Nonnull PathRemover.PathChecker pathChecker) { + return PathRemover.newFilteredRobustRemover(pathChecker, DELETION_MAX - 1, GC_AFTER_FAILED_DELETE, WAIT_BETWEEN_DELETION_RETRIES); } /** diff --git a/core/src/main/java/jenkins/util/io/PathRemover.java b/core/src/main/java/jenkins/util/io/PathRemover.java index 789c9b4d79..ece2e04bbf 100644 --- a/core/src/main/java/jenkins/util/io/PathRemover.java +++ b/core/src/main/java/jenkins/util/io/PathRemover.java @@ -42,7 +42,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -50,23 +49,23 @@ import java.util.stream.Stream; public class PathRemover { public static PathRemover newSimpleRemover() { - return new PathRemover(ignored -> false, ignored -> true); + return new PathRemover(ignored -> false, PathChecker.ALLOW_ALL); } public static PathRemover newRemoverWithStrategy(@Nonnull RetryStrategy retryStrategy) { - return new PathRemover(retryStrategy, ignored -> true); + return new PathRemover(retryStrategy, PathChecker.ALLOW_ALL); } - public static PathRemover newFilteredRobustRemover(@Nonnull Predicate pathFilter, int maxRetries, boolean gcAfterFailedRemove, long waitBetweenRetries) { - return new PathRemover(new PausingGCRetryStrategy(maxRetries < 1 ? 1 : maxRetries, gcAfterFailedRemove, waitBetweenRetries), pathFilter); + public static PathRemover newFilteredRobustRemover(@Nonnull PathChecker pathChecker, int maxRetries, boolean gcAfterFailedRemove, long waitBetweenRetries) { + return new PathRemover(new PausingGCRetryStrategy(maxRetries < 1 ? 1 : maxRetries, gcAfterFailedRemove, waitBetweenRetries), pathChecker); } private final RetryStrategy retryStrategy; - private final Predicate pathFilter; + private final PathChecker pathChecker; - private PathRemover(@Nonnull RetryStrategy retryStrategy, @Nonnull Predicate pathFilter) { + private PathRemover(@Nonnull RetryStrategy retryStrategy, @Nonnull PathChecker pathChecker) { this.retryStrategy = retryStrategy; - this.pathFilter = pathFilter; + this.pathChecker = pathChecker; } public void forceRemoveFile(@Nonnull Path path) throws IOException { @@ -97,6 +96,14 @@ public class PathRemover { } } + @Restricted(NoExternalUse.class) + @FunctionalInterface + public interface PathChecker { + void check(@Nonnull Path path) throws SecurityException; + + PathChecker ALLOW_ALL = path -> {}; + } + @Restricted(NoExternalUse.class) @FunctionalInterface public interface RetryStrategy { @@ -188,7 +195,7 @@ public class PathRemover { return sb.toString(); } } - + private Optional tryRemoveFile(@Nonnull Path path) { try { removeOrMakeRemovableThenRemove(path.normalize()); @@ -221,7 +228,7 @@ public class PathRemover { } private void removeOrMakeRemovableThenRemove(@Nonnull Path path) throws IOException { - if (!pathFilter.test(path)) return; + pathChecker.check(path); try { Files.deleteIfExists(path); } catch (IOException e) { From c35f3ba75ce8a5a33b7efc8f760ec7e5bf8c9228 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 11 Jan 2019 10:35:57 -0600 Subject: [PATCH 08/14] Add more tests for JENKINS-55448 This also refactors some test calls to mklink into a package-private Util method due to additional tests needing to create junction points. Signed-off-by: Matt Sicker --- core/src/main/java/hudson/Util.java | 33 +++++++++++++ core/src/test/java/hudson/FilePathTest.java | 7 +-- .../RemoveWindowsDirectoryJunctionTest.java | 12 +---- core/src/test/java/hudson/UtilTest.java | 46 ++++++++++++++++--- .../java/jenkins/util/io/PathRemoverTest.java | 33 +++++++++++++ 5 files changed, 108 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 45d8f3f5e2..a55b90d05e 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -23,6 +23,7 @@ */ package hudson; +import com.google.common.annotations.VisibleForTesting; import hudson.model.TaskListener; import jenkins.util.MemoryReductionUtil; import hudson.util.QuotedStringTokenizer; @@ -1277,6 +1278,38 @@ public class Util { } } + /** + * Creates an NTFS junction point if supported. Similar to symbolic links, NTFS provides junction points which + * provide different features than symbolic links. This method is primarily useful for unit testing on Windows. + * @param junction NTFS junction point to create + * @param target target directory to junction + * @return the newly created junction point + * @throws IOException if the call to mklink exits with a non-zero status code + * @throws InterruptedException if the call to mklink is interrupted before completing + * @throws AssertionError if this method is called on a non-Windows platform + */ + static @Nonnull File createJunction(@Nonnull File junction, @Nonnull File target) throws IOException, InterruptedException { + if (!Functions.isWindows()) throw new AssertionError("Cannot create junctions on non-Windows platforms"); + String command = "mklink /J " + escapeCmdArgument(junction.getAbsolutePath()) + ' ' + escapeCmdArgument(target.getAbsolutePath()); + Process process = new ProcessBuilder("cmd.exe", "/C", command).start(); + int result = process.waitFor(); + if (result != 0) throw new IOException("Command `" + command + "` failed with status code " + result); + return junction; + } + + private static final Pattern CMD_METACHARS = Pattern.compile("[()%!^\"<>&|]"); + + /** + * Escapes a Windows cmd argument. Cmd supports escaping characters with carats which simplifies nested escapes. + * @param arg argument to escape + * @return the quoted argument for use in cmd.exe + * @see Date: Fri, 11 Jan 2019 11:27:03 -0600 Subject: [PATCH 09/14] Fix typo in link Co-Authored-By: jvz --- core/src/main/java/hudson/Util.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index a55b90d05e..6aa703db9a 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1303,7 +1303,7 @@ public class Util { * Escapes a Windows cmd argument. Cmd supports escaping characters with carats which simplifies nested escapes. * @param arg argument to escape * @return the quoted argument for use in cmd.exe - * @see Everyone quotes command line arguments the wrong way */ @VisibleForTesting static String escapeCmdArgument(String arg) { From b747845f168449af58ddd9c5a9be791ca1b29565 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 11 Jan 2019 15:05:50 -0600 Subject: [PATCH 10/14] Fix Windows command line argument quoting in createJunction Signed-off-by: Matt Sicker --- core/src/main/java/hudson/Util.java | 23 ++--- .../windows/WindowsCommandLineFormatter.java | 86 +++++++++++++++++++ core/src/test/java/hudson/UtilTest.java | 8 -- .../WindowsCommandLineFormatterTest.java | 48 +++++++++++ 4 files changed, 140 insertions(+), 25 deletions(-) create mode 100644 core/src/main/java/jenkins/util/os/windows/WindowsCommandLineFormatter.java create mode 100644 core/src/test/java/jenkins/util/os/windows/WindowsCommandLineFormatterTest.java diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 6aa703db9a..d754fb10fe 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -23,7 +23,6 @@ */ package hudson; -import com.google.common.annotations.VisibleForTesting; import hudson.model.TaskListener; import jenkins.util.MemoryReductionUtil; import hudson.util.QuotedStringTokenizer; @@ -31,6 +30,7 @@ import hudson.util.VariableResolver; import jenkins.util.SystemProperties; import jenkins.util.io.PathRemover; +import jenkins.util.os.windows.WindowsCommandLineFormatter; import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.io.IOUtils; import org.apache.commons.io.output.NullOutputStream; @@ -79,7 +79,6 @@ import java.time.temporal.ChronoUnit; import java.util.*; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Predicate; import java.util.logging.Level; import java.util.logging.LogRecord; import java.util.logging.Logger; @@ -1290,26 +1289,16 @@ public class Util { */ static @Nonnull File createJunction(@Nonnull File junction, @Nonnull File target) throws IOException, InterruptedException { if (!Functions.isWindows()) throw new AssertionError("Cannot create junctions on non-Windows platforms"); - String command = "mklink /J " + escapeCmdArgument(junction.getAbsolutePath()) + ' ' + escapeCmdArgument(target.getAbsolutePath()); - Process process = new ProcessBuilder("cmd.exe", "/C", command).start(); + String command = "cmd.exe /C mklink /J " + + WindowsCommandLineFormatter.quoteArgumentForCmd(junction.getAbsolutePath()) + + ' ' + + WindowsCommandLineFormatter.quoteArgumentForCmd(target.getAbsolutePath()); + Process process = Runtime.getRuntime().exec(command); int result = process.waitFor(); if (result != 0) throw new IOException("Command `" + command + "` failed with status code " + result); return junction; } - private static final Pattern CMD_METACHARS = Pattern.compile("[()%!^\"<>&|]"); - - /** - * Escapes a Windows cmd argument. Cmd supports escaping characters with carats which simplifies nested escapes. - * @param arg argument to escape - * @return the quoted argument for use in cmd.exe - * @see Everyone quotes command line arguments the wrong way - */ - @VisibleForTesting - static String escapeCmdArgument(String arg) { - return CMD_METACHARS.matcher('"' + arg + '"').replaceAll("^$0"); - } - /** * Encodes the URL by RFC 2396. * diff --git a/core/src/main/java/jenkins/util/os/windows/WindowsCommandLineFormatter.java b/core/src/main/java/jenkins/util/os/windows/WindowsCommandLineFormatter.java new file mode 100644 index 0000000000..4958ff2b12 --- /dev/null +++ b/core/src/main/java/jenkins/util/os/windows/WindowsCommandLineFormatter.java @@ -0,0 +1,86 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package jenkins.util.os.windows; + +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import javax.annotation.Nonnull; +import java.util.regex.Pattern; + +/** + * Provides formatting utilities for Windows command line processes. + * + * @see Everyone quotes command line arguments the wrong way + */ +@Restricted(NoExternalUse.class) +public final class WindowsCommandLineFormatter { + private WindowsCommandLineFormatter() { + } + + /** + * Quotes an argument while escaping special characters interpreted by CreateProcess. + */ + public static @Nonnull String quoteArgument(@Nonnull String argument) { + StringBuilder sb = new StringBuilder(); + sb.append('"'); + int end = argument.length(); + for (int i = 0; i < end; i++) { + int nrBackslashes = 0; + while (i < end && argument.charAt(i) == '\\') { + i++; + nrBackslashes++; + } + + if (i == end) { + // backslashes at the end of the argument must be escaped so the terminate quote isn't + nrBackslashes = nrBackslashes * 2; + } else if (argument.charAt(i) == '"') { + // backslashes preceding a quote all need to be escaped along with the quote + nrBackslashes = nrBackslashes * 2 + 1; + } + // else backslashes have no special meaning and don't need to be escaped here + + for (int j = 0; j < nrBackslashes; j++) { + sb.append('\\'); + } + + if (i < end) { + sb.append(argument.charAt(i)); + } + } + return sb.append('"').toString(); + } + + private static final Pattern CMD_METACHARS = Pattern.compile("[()%!^\"<>&|]"); + + /** + * Quotes an argument while escaping special characters suitable for use as an argument to {@code cmd.exe}. + */ + public static @Nonnull String quoteArgumentForCmd(@Nonnull String argument) { + return CMD_METACHARS.matcher(quoteArgument(argument)).replaceAll("^$0"); + } + +} diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index aeb7aaf9e8..cb19aa7484 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -257,14 +257,6 @@ public class UtilTest { } } - @Test - public void testEscapeCmdArgument() { - String raw = "hello \"\\world&"; - String expected = "^\"hello ^\"\\world^&^\""; - String actual = Util.escapeCmdArgument(raw); - assertEquals(expected, actual); - } - @Test public void testIsSymlink_onWindows_junction() throws Exception { Assume.assumeTrue("Uses Windows-specific features", Functions.isWindows()); diff --git a/core/src/test/java/jenkins/util/os/windows/WindowsCommandLineFormatterTest.java b/core/src/test/java/jenkins/util/os/windows/WindowsCommandLineFormatterTest.java new file mode 100644 index 0000000000..d715fb2870 --- /dev/null +++ b/core/src/test/java/jenkins/util/os/windows/WindowsCommandLineFormatterTest.java @@ -0,0 +1,48 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package jenkins.util.os.windows; + +import org.junit.Test; + +import static org.junit.Assert.*; + +public class WindowsCommandLineFormatterTest { + + @Test + public void testQuoteArgument() { + String input = "C:\\Programs and \"Settings\"\\System32\\\\"; + String expected = "\"C:\\Programs and \\\"Settings\\\"\\System32\\\\\\\\\""; + String actual = WindowsCommandLineFormatter.quoteArgument(input); + assertEquals(expected, actual); + } + + @Test + public void testQuoteArgumentForCmd() { + String input = "hello \"\\world&"; + String expected = "^\"hello \\^\"\\world^&^\""; + String actual = WindowsCommandLineFormatter.quoteArgumentForCmd(input); + assertEquals(expected, actual); + } +} \ No newline at end of file From a489cc4ee7269e45b3939b3d5601aadee5876140 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 11 Jan 2019 15:35:02 -0600 Subject: [PATCH 11/14] Move Windows util code into test sources Signed-off-by: Matt Sicker --- core/src/main/java/hudson/Util.java | 23 --------- core/src/test/java/hudson/FilePathTest.java | 3 +- .../RemoveWindowsDirectoryJunctionTest.java | 4 +- core/src/test/java/hudson/UtilTest.java | 5 +- .../java/hudson/os/WindowsUtil.java} | 50 ++++++++++++++----- .../os/WindowsUtilTest.java} | 12 +++-- 6 files changed, 53 insertions(+), 44 deletions(-) rename core/src/{main/java/jenkins/util/os/windows/WindowsCommandLineFormatter.java => test/java/hudson/os/WindowsUtil.java} (60%) rename core/src/test/java/{jenkins/util/os/windows/WindowsCommandLineFormatterTest.java => hudson/os/WindowsUtilTest.java} (87%) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index d754fb10fe..df80ca2ff3 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -30,7 +30,6 @@ import hudson.util.VariableResolver; import jenkins.util.SystemProperties; import jenkins.util.io.PathRemover; -import jenkins.util.os.windows.WindowsCommandLineFormatter; import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.io.IOUtils; import org.apache.commons.io.output.NullOutputStream; @@ -1277,28 +1276,6 @@ public class Util { } } - /** - * Creates an NTFS junction point if supported. Similar to symbolic links, NTFS provides junction points which - * provide different features than symbolic links. This method is primarily useful for unit testing on Windows. - * @param junction NTFS junction point to create - * @param target target directory to junction - * @return the newly created junction point - * @throws IOException if the call to mklink exits with a non-zero status code - * @throws InterruptedException if the call to mklink is interrupted before completing - * @throws AssertionError if this method is called on a non-Windows platform - */ - static @Nonnull File createJunction(@Nonnull File junction, @Nonnull File target) throws IOException, InterruptedException { - if (!Functions.isWindows()) throw new AssertionError("Cannot create junctions on non-Windows platforms"); - String command = "cmd.exe /C mklink /J " + - WindowsCommandLineFormatter.quoteArgumentForCmd(junction.getAbsolutePath()) + - ' ' + - WindowsCommandLineFormatter.quoteArgumentForCmd(target.getAbsolutePath()); - Process process = Runtime.getRuntime().exec(command); - int result = process.waitFor(); - if (result != 0) throw new IOException("Command `" + command + "` failed with status code " + result); - return junction; - } - /** * Encodes the URL by RFC 2396. * diff --git a/core/src/test/java/hudson/FilePathTest.java b/core/src/test/java/hudson/FilePathTest.java index cf4e163b9d..b40d0a37d4 100644 --- a/core/src/test/java/hudson/FilePathTest.java +++ b/core/src/test/java/hudson/FilePathTest.java @@ -26,6 +26,7 @@ package hudson; import hudson.FilePath.TarCompression; import hudson.model.TaskListener; import hudson.os.PosixAPI; +import hudson.os.WindowsUtil; import hudson.remoting.VirtualChannel; import hudson.util.NullStream; import hudson.util.StreamTaskListener; @@ -836,7 +837,7 @@ public class FilePathTest { Path targetDir = temp.newFolder("targetDir").toPath(); Path targetContents = Files.createFile(targetDir.resolve("contents.txt")); Path toDelete = temp.newFolder("toDelete").toPath(); - File junction = Util.createJunction(toDelete.resolve("junction").toFile(), targetDir.toFile()); + File junction = WindowsUtil.createJunction(toDelete.resolve("junction").toFile(), targetDir.toFile()); Files.createFile(toDelete.resolve("foo")); Files.createFile(toDelete.resolve("bar")); FilePath f = new FilePath(toDelete.toFile()); diff --git a/core/src/test/java/hudson/RemoveWindowsDirectoryJunctionTest.java b/core/src/test/java/hudson/RemoveWindowsDirectoryJunctionTest.java index dc44db7974..de296e634b 100644 --- a/core/src/test/java/hudson/RemoveWindowsDirectoryJunctionTest.java +++ b/core/src/test/java/hudson/RemoveWindowsDirectoryJunctionTest.java @@ -8,6 +8,8 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeTrue; import java.io.File; + +import hudson.os.WindowsUtil; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -32,7 +34,7 @@ public class RemoveWindowsDirectoryJunctionTest { File subdir1 = tmp.newFolder("notJunction"); File f1 = new File(subdir1, "testfile1.txt"); assertTrue("Unable to create temporary file in notJunction directory", f1.createNewFile()); - File j1 = Util.createJunction(tmp.getRoot(), subdir1); + File j1 = WindowsUtil.createJunction(tmp.getRoot(), subdir1); Util.deleteRecursive(j1); assertFalse("Windows Junction should have been removed", j1.exists()); assertTrue("Contents of Windows Junction should not be removed", f1.exists()); diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index cb19aa7484..8662fc5631 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -43,6 +43,7 @@ import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.startsWith; import static org.junit.Assert.*; +import hudson.os.WindowsUtil; import org.hamcrest.BaseMatcher; import org.hamcrest.Description; import org.junit.Assume; @@ -262,7 +263,7 @@ public class UtilTest { Assume.assumeTrue("Uses Windows-specific features", Functions.isWindows()); File targetDir = tmp.newFolder("targetDir"); File d = tmp.newFolder("dir"); - File junction = Util.createJunction(new File(d, "junction"), targetDir); + File junction = WindowsUtil.createJunction(new File(d, "junction"), targetDir); assertTrue(Util.isSymlink(junction)); } @@ -274,7 +275,7 @@ public class UtilTest { File file = new File(targetDir, "test-file"); new FilePath(file).touch(System.currentTimeMillis()); File dir = tmp.newFolder(); - File junction = Util.createJunction(new File(dir, "junction"), targetDir); + File junction = WindowsUtil.createJunction(new File(dir, "junction"), targetDir); assertTrue(Util.isSymlink(junction)); assertFalse(Util.isSymlink(file)); diff --git a/core/src/main/java/jenkins/util/os/windows/WindowsCommandLineFormatter.java b/core/src/test/java/hudson/os/WindowsUtil.java similarity index 60% rename from core/src/main/java/jenkins/util/os/windows/WindowsCommandLineFormatter.java rename to core/src/test/java/hudson/os/WindowsUtil.java index 4958ff2b12..56a1dd13d4 100644 --- a/core/src/main/java/jenkins/util/os/windows/WindowsCommandLineFormatter.java +++ b/core/src/test/java/hudson/os/WindowsUtil.java @@ -22,24 +22,21 @@ * THE SOFTWARE. */ -package jenkins.util.os.windows; +package hudson.os; -import org.kohsuke.accmod.Restricted; -import org.kohsuke.accmod.restrictions.NoExternalUse; +import hudson.Functions; +import org.apache.commons.io.IOUtils; import javax.annotation.Nonnull; +import java.io.File; +import java.io.IOException; +import java.util.Arrays; import java.util.regex.Pattern; +import java.util.stream.Collectors; -/** - * Provides formatting utilities for Windows command line processes. - * - * @see Everyone quotes command line arguments the wrong way - */ -@Restricted(NoExternalUse.class) -public final class WindowsCommandLineFormatter { - private WindowsCommandLineFormatter() { - } +import static org.junit.Assert.assertTrue; +public class WindowsUtil { /** * Quotes an argument while escaping special characters interpreted by CreateProcess. */ @@ -83,4 +80,33 @@ public final class WindowsCommandLineFormatter { return CMD_METACHARS.matcher(quoteArgument(argument)).replaceAll("^$0"); } + /** + * Executes a command and arguments using {@code cmd.exe /C ...}. + */ + public static @Nonnull Process execCmd(String... argv) throws IOException { + String command = Arrays.stream(argv).map(WindowsUtil::quoteArgumentForCmd).collect(Collectors.joining(" ")); + return Runtime.getRuntime().exec(new String[]{"cmd.exe", "/C", command}); + } + + /** + * Creates an NTFS junction point if supported. Similar to symbolic links, NTFS provides junction points which + * provide different features than symbolic links. + * @param junction NTFS junction point to create + * @param target target directory to junction + * @return the newly created junction point + * @throws IOException if the call to mklink exits with a non-zero status code + * @throws InterruptedException if the call to mklink is interrupted before completing + * @throws AssertionError if this method is called on a non-Windows platform + */ + public static @Nonnull File createJunction(@Nonnull File junction, @Nonnull File target) throws IOException, InterruptedException { + assertTrue(Functions.isWindows()); + Process mklink = execCmd("mklink", "/J", junction.getAbsolutePath(), target.getAbsolutePath()); + int result = mklink.waitFor(); + if (result != 0) { + String stderr = IOUtils.toString(mklink.getErrorStream()); + String stdout = IOUtils.toString(mklink.getInputStream()); + throw new IOException("Process exited with " + result + "\nStandard Output:\n" + stdout + "\nError Output:\n" + stderr); + } + return junction; + } } diff --git a/core/src/test/java/jenkins/util/os/windows/WindowsCommandLineFormatterTest.java b/core/src/test/java/hudson/os/WindowsUtilTest.java similarity index 87% rename from core/src/test/java/jenkins/util/os/windows/WindowsCommandLineFormatterTest.java rename to core/src/test/java/hudson/os/WindowsUtilTest.java index d715fb2870..4fd9bb45d2 100644 --- a/core/src/test/java/jenkins/util/os/windows/WindowsCommandLineFormatterTest.java +++ b/core/src/test/java/hudson/os/WindowsUtilTest.java @@ -22,19 +22,21 @@ * THE SOFTWARE. */ -package jenkins.util.os.windows; +package hudson.os; import org.junit.Test; import static org.junit.Assert.*; -public class WindowsCommandLineFormatterTest { - +/** + * Test the test utility code, too! + */ +public class WindowsUtilTest { @Test public void testQuoteArgument() { String input = "C:\\Programs and \"Settings\"\\System32\\\\"; String expected = "\"C:\\Programs and \\\"Settings\\\"\\System32\\\\\\\\\""; - String actual = WindowsCommandLineFormatter.quoteArgument(input); + String actual = WindowsUtil.quoteArgument(input); assertEquals(expected, actual); } @@ -42,7 +44,7 @@ public class WindowsCommandLineFormatterTest { public void testQuoteArgumentForCmd() { String input = "hello \"\\world&"; String expected = "^\"hello \\^\"\\world^&^\""; - String actual = WindowsCommandLineFormatter.quoteArgumentForCmd(input); + String actual = WindowsUtil.quoteArgumentForCmd(input); assertEquals(expected, actual); } } \ No newline at end of file From 637ae87e95d281e9a9764a2f6f7e046ee195a318 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 11 Jan 2019 15:39:55 -0600 Subject: [PATCH 12/14] Add link to blog post Signed-off-by: Matt Sicker --- core/src/test/java/hudson/os/WindowsUtil.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/test/java/hudson/os/WindowsUtil.java b/core/src/test/java/hudson/os/WindowsUtil.java index 56a1dd13d4..831d500144 100644 --- a/core/src/test/java/hudson/os/WindowsUtil.java +++ b/core/src/test/java/hudson/os/WindowsUtil.java @@ -36,6 +36,8 @@ import java.util.stream.Collectors; import static org.junit.Assert.assertTrue; +// adapted from: +// https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ public class WindowsUtil { /** * Quotes an argument while escaping special characters interpreted by CreateProcess. From e4e37836c5fb07b866ffc5a82dbf4350e9ef43eb Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 11 Jan 2019 18:20:22 -0600 Subject: [PATCH 13/14] Improve cmd quoting algorithm Signed-off-by: Matt Sicker --- core/src/test/java/hudson/os/WindowsUtil.java | 3 +++ .../src/test/java/hudson/os/WindowsUtilTest.java | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/core/src/test/java/hudson/os/WindowsUtil.java b/core/src/test/java/hudson/os/WindowsUtil.java index 831d500144..6e528827a3 100644 --- a/core/src/test/java/hudson/os/WindowsUtil.java +++ b/core/src/test/java/hudson/os/WindowsUtil.java @@ -39,10 +39,13 @@ import static org.junit.Assert.assertTrue; // adapted from: // https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ public class WindowsUtil { + private static final Pattern NEEDS_QUOTING = Pattern.compile("[\\s\"]"); + /** * Quotes an argument while escaping special characters interpreted by CreateProcess. */ public static @Nonnull String quoteArgument(@Nonnull String argument) { + if (!NEEDS_QUOTING.matcher(argument).find()) return argument; StringBuilder sb = new StringBuilder(); sb.append('"'); int end = argument.length(); diff --git a/core/src/test/java/hudson/os/WindowsUtilTest.java b/core/src/test/java/hudson/os/WindowsUtilTest.java index 4fd9bb45d2..c8b4dbb4bf 100644 --- a/core/src/test/java/hudson/os/WindowsUtilTest.java +++ b/core/src/test/java/hudson/os/WindowsUtilTest.java @@ -26,6 +26,8 @@ package hudson.os; import org.junit.Test; +import java.util.Arrays; + import static org.junit.Assert.*; /** @@ -40,6 +42,13 @@ public class WindowsUtilTest { assertEquals(expected, actual); } + @Test + public void testQuoteArgument_OnlyQuotesWhenNecessary() { + for (String arg : Arrays.asList("", "foo", "foo-bar", "C:\\test\\path", "http://www.example.com/")) { + assertEquals(arg, WindowsUtil.quoteArgument(arg)); + } + } + @Test public void testQuoteArgumentForCmd() { String input = "hello \"\\world&"; @@ -47,4 +56,11 @@ public class WindowsUtilTest { String actual = WindowsUtil.quoteArgumentForCmd(input); assertEquals(expected, actual); } + + @Test + public void testQuoteArgumentForCmd_OnlyQuotesWhenNecessary() { + for (String arg : Arrays.asList("", "foo", "foo-bar", "C:\\test\\path", "http://www.example.com/")) { + assertEquals(arg, WindowsUtil.quoteArgumentForCmd(arg)); + } + } } \ No newline at end of file From 95ada0a6721b704f832d772d4b9865101c7f7cb3 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Mon, 14 Jan 2019 13:28:25 -0600 Subject: [PATCH 14/14] Fix test failure Signed-off-by: Matt Sicker --- .../test/java/hudson/RemoveWindowsDirectoryJunctionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/hudson/RemoveWindowsDirectoryJunctionTest.java b/core/src/test/java/hudson/RemoveWindowsDirectoryJunctionTest.java index de296e634b..d0255f6f37 100644 --- a/core/src/test/java/hudson/RemoveWindowsDirectoryJunctionTest.java +++ b/core/src/test/java/hudson/RemoveWindowsDirectoryJunctionTest.java @@ -34,7 +34,7 @@ public class RemoveWindowsDirectoryJunctionTest { File subdir1 = tmp.newFolder("notJunction"); File f1 = new File(subdir1, "testfile1.txt"); assertTrue("Unable to create temporary file in notJunction directory", f1.createNewFile()); - File j1 = WindowsUtil.createJunction(tmp.getRoot(), subdir1); + File j1 = WindowsUtil.createJunction(new File(tmp.getRoot(), "test junction"), subdir1); Util.deleteRecursive(j1); assertFalse("Windows Junction should have been removed", j1.exists()); assertTrue("Contents of Windows Junction should not be removed", f1.exists());