From fdccc0e8384370684e25063e95f4a704773c53dd Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Fri, 1 Dec 2017 02:01:34 -0500 Subject: [PATCH] [JENKINS-36088] Use NIO implementations of chmod and mode by default (#3135) * Use NIO for FilePath#chmod and IOUtils#mode * Add tests for NIO mode and chmod implementations * Add test, remove new method, and update JavaDoc * Provide system property to use native implementations of chmod and mode * Revert unrelated whitespace modification * Don't remove exception from throws and put imports in original location * Fix broken JavaDoc links * Ignore file type bits (above 0o7777) in Util#modeToPermission * Use octal for constants and don't include file type bits * Revert unnecessary changes to TarArchiverTest * Add assertion that non-permission bits are ignored by chmod * Use NIO copy with StandardCopyOption.COPY_ATTRIBUTES in copyToWithPermissions where possible * Catch InvalidPathException and convert it to IOException * Create utility method for File#toPath and use File#createDirectories after review * Remove useless calls to toAbsolutePath and getAbsoluteFile * Fix typos and use octal for constant after review * Add test for behavior of copyToWithPermission with special bits --- core/src/main/java/hudson/FilePath.java | 30 +++++++++- core/src/main/java/hudson/Util.java | 57 +++++++++++++++++++ core/src/main/java/hudson/util/IOUtils.java | 19 ++++++- core/src/test/java/hudson/FilePathTest.java | 53 ++++++++++++++++- core/src/test/java/hudson/UtilTest.java | 39 +++++++++++++ .../java/hudson/util/io/TarArchiverTest.java | 8 +-- 6 files changed, 196 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index ad13edf5f3..197a27f60b 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -79,6 +79,7 @@ import java.net.URL; import java.net.URLConnection; import java.nio.file.Files; import java.nio.file.InvalidPathException; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; @@ -1582,6 +1583,11 @@ public final class FilePath implements Serializable { *

* please note mask is expected to be an octal if you use chmod command line values, * so preceded by a '0' in java notation, ie chmod(0644) + *

+ * Only supports setting read, write, or execute permissions for the + * owner, group, or others, so the largest permissible value is 0777. + * Attempting to set larger values (i.e. the setgid, setuid, or sticky + * bits) will cause an IOException to be thrown. * * @since 1.303 * @see #mode() @@ -1591,7 +1597,6 @@ public final class FilePath implements Serializable { act(new SecureFileCallable() { private static final long serialVersionUID = 1L; public Void invoke(File f, VirtualChannel channel) throws IOException { - // TODO first check for Java 7+ and use PosixFileAttributeView _chmod(writing(f), mask); return null; @@ -1600,14 +1605,18 @@ public final class FilePath implements Serializable { } /** - * Run chmod via jnr-posix + * Change permissions via NIO. */ private static void _chmod(File f, int mask) throws IOException { // TODO WindowsPosix actually does something here (WindowsLibC._wchmod); should we let it? // Anyway the existing calls already skip this method if on Windows. if (File.pathSeparatorChar==';') return; // noop - PosixAPI.jnr().chmod(f.getAbsolutePath(),mask); + if (Util.NATIVE_CHMOD_MODE) { + PosixAPI.jnr().chmod(f.getAbsolutePath(), mask); + } else { + Files.setPosixFilePermissions(Util.fileToPath(f), Util.modeToPermissions(mask)); + } } private static boolean CHMOD_WARNED = false; @@ -2008,6 +2017,21 @@ public final class FilePath implements Serializable { * @since 1.311 */ public void copyToWithPermission(FilePath target) throws IOException, InterruptedException { + // Use NIO copy with StandardCopyOption.COPY_ATTRIBUTES when copying on the same machine. + if (this.channel == target.channel) { + act(new SecureFileCallable() { + public Void invoke(File f, VirtualChannel channel) throws IOException { + File targetFile = new File(target.remote); + File targetDir = targetFile.getParentFile(); + filterNonNull().mkdirs(targetDir); + Files.createDirectories(Util.fileToPath(targetDir)); + Files.copy(Util.fileToPath(reading(f)), Util.fileToPath(writing(targetFile)), StandardCopyOption.COPY_ATTRIBUTES, StandardCopyOption.REPLACE_EXISTING); + return null; + } + }); + return; + } + copyTo(target); // copy file permission target.chmod(mode()); diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index fc0afb1505..8ee5c7390a 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -68,6 +68,7 @@ import java.nio.file.LinkOption; import java.nio.file.NoSuchFileException; 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.DosFileAttributes; import java.security.MessageDigest; @@ -1597,6 +1598,51 @@ public class Util { } } + @Restricted(NoExternalUse.class) + public static int permissionsToMode(Set permissions) { + PosixFilePermission[] allPermissions = PosixFilePermission.values(); + int result = 0; + for (int i = 0; i < allPermissions.length; i++) { + result <<= 1; + result |= permissions.contains(allPermissions[i]) ? 1 : 0; + } + return result; + } + + @Restricted(NoExternalUse.class) + public static Set modeToPermissions(int mode) throws IOException { + // Anything larger is a file type, not a permission. + int PERMISSIONS_MASK = 07777; + // setgid/setuid/sticky are not supported. + int MAX_SUPPORTED_MODE = 0777; + mode = mode & PERMISSIONS_MASK; + if ((mode & MAX_SUPPORTED_MODE) != mode) { + throw new IOException("Invalid mode: " + mode); + } + PosixFilePermission[] allPermissions = PosixFilePermission.values(); + Set result = EnumSet.noneOf(PosixFilePermission.class); + for (int i = 0; i < allPermissions.length; i++) { + if ((mode & 1) == 1) { + result.add(allPermissions[allPermissions.length - i - 1]); + } + mode >>= 1; + } + return result; + } + + /** + * Converts a {@link File} into a {@link Path} and checks runtime exceptions. + * @throws IOException if {@code f.toPath()} throws {@link InvalidPathException}. + */ + @Restricted(NoExternalUse.class) + public static @Nonnull Path fileToPath(@Nonnull File file) throws IOException { + try { + return file.toPath(); + } catch (InvalidPathException e) { + throw new IOException(e); + } + } + public static final FastDateFormat XS_DATETIME_FORMATTER = FastDateFormat.getInstance("yyyy-MM-dd'T'HH:mm:ss'Z'",new SimpleTimeZone(0,"GMT")); // Note: RFC822 dates must not be localized! @@ -1664,4 +1710,15 @@ public class Util { */ @Restricted(value = NoExternalUse.class) static boolean GC_AFTER_FAILED_DELETE = SystemProperties.getBoolean(Util.class.getName() + ".performGCOnFailedDelete"); + + /** + * If this flag is true, native implementations of {@link FilePath#chmod} + * and {@link hudson.util.IOUtils#mode} are used instead of NIO. + *

+ * This should only be enabled if the setgid/setuid/sticky bits are + * intentionally set on the Jenkins installation and they are being + * overwritten by Jenkins erroneously. + */ + @Restricted(value = NoExternalUse.class) + public static boolean NATIVE_CHMOD_MODE = SystemProperties.getBoolean(Util.class.getName() + ".useNativeChmodAndMode"); } diff --git a/core/src/main/java/hudson/util/IOUtils.java b/core/src/main/java/hudson/util/IOUtils.java index d726b43df5..4168379405 100644 --- a/core/src/main/java/hudson/util/IOUtils.java +++ b/core/src/main/java/hudson/util/IOUtils.java @@ -1,6 +1,7 @@ package hudson.util; import hudson.Functions; +import hudson.Util; import hudson.os.PosixAPI; import hudson.os.PosixException; import java.nio.file.Files; @@ -119,13 +120,27 @@ public class IOUtils { /** - * Gets the mode of a file/directory, if appropriate. + * Gets the mode of a file/directory, if appropriate. Only includes read, write, and + * execute permissions for the owner, group, and others, i.e. the max return value + * is 0777. Consider using {@link Files#getPosixFilePermissions} instead if you only + * care about access permissions. + * * @return a file mode, or -1 if not on Unix * @throws PosixException if the file could not be statted, e.g. broken symlink */ public static int mode(File f) throws PosixException { if(Functions.isWindows()) return -1; - return PosixAPI.jnr().stat(f.getPath()).mode(); + try { + if (Util.NATIVE_CHMOD_MODE) { + return PosixAPI.jnr().stat(f.getPath()).mode(); + } else { + return Util.permissionsToMode(Files.getPosixFilePermissions(Util.fileToPath(f))); + } + } catch (IOException cause) { + PosixException e = new PosixException("Unable to get file permissions", null); + e.initCause(cause); + throw e; + } } /** diff --git a/core/src/test/java/hudson/FilePathTest.java b/core/src/test/java/hudson/FilePathTest.java index 84ca31d889..846900424e 100644 --- a/core/src/test/java/hudson/FilePathTest.java +++ b/core/src/test/java/hudson/FilePathTest.java @@ -25,6 +25,7 @@ package hudson; import hudson.FilePath.TarCompression; import hudson.model.TaskListener; +import hudson.os.PosixAPI; import hudson.remoting.VirtualChannel; import hudson.util.NullStream; import hudson.util.StreamTaskListener; @@ -245,7 +246,7 @@ public class FilePathTest { throw x; } } finally { - toF.chmod(700); + toF.chmod(0700); } } @@ -471,6 +472,25 @@ public class FilePathTest { } } + @Test public void copyToWithPermissionSpecialPermissions() throws IOException, InterruptedException { + assumeFalse("Test uses POSIX-specific features", Functions.isWindows()); + File tmp = temp.getRoot(); + File original = new File(tmp,"original"); + FilePath originalP = new FilePath(channels.french, original.getPath()); + originalP.touch(0); + PosixAPI.jnr().chmod(original.getAbsolutePath(), 02777); // Read/write/execute for everyone and setuid. + + File sameChannelCopy = new File(tmp,"sameChannelCopy"); + FilePath sameChannelCopyP = new FilePath(channels.french, sameChannelCopy.getPath()); + originalP.copyToWithPermission(sameChannelCopyP); + assertEquals("Special permissions should be copied on the same machine", 02777, PosixAPI.jnr().stat(sameChannelCopy.getAbsolutePath()).mode() & 07777); + + File diffChannelCopy = new File(tmp,"diffChannelCopy"); + FilePath diffChannelCopyP = new FilePath(channels.british, diffChannelCopy.getPath()); + originalP.copyToWithPermission(diffChannelCopyP); + assertEquals("Special permissions should not be copied across machines", 00777, PosixAPI.jnr().stat(diffChannelCopy.getAbsolutePath()).mode() & 07777); + } + @Test public void symlinkInTar() throws Exception { assumeFalse("can't test on Windows", Functions.isWindows()); @@ -739,7 +759,38 @@ public class FilePathTest { // and now fail when flush is bad! tmpDirPath.child("../" + archive.getName()).untar(outDir, TarCompression.NONE); } + + @Test + public void chmod() throws Exception { + assumeFalse(Functions.isWindows()); + File f = temp.newFile("file"); + FilePath fp = new FilePath(f); + int prevMode = fp.mode(); + assertEquals(0400, chmodAndMode(fp, 0400)); + assertEquals(0412, chmodAndMode(fp, 0412)); + assertEquals(0777, chmodAndMode(fp, 0777)); + assertEquals(prevMode, chmodAndMode(fp, prevMode)); + } + @Test + public void chmodInvalidPermissions() throws Exception { + assumeFalse(Functions.isWindows()); + File f = temp.newFolder("folder"); + FilePath fp = new FilePath(f); + int invalidMode = 01770; // Full permissions for owner and group plus sticky bit. + try { + chmodAndMode(fp, invalidMode); + fail("Setting sticky bit should fail"); + } catch (IOException e) { + assertEquals("Invalid mode: " + invalidMode, e.getMessage()); + } + } + + private int chmodAndMode(FilePath path, int mode) throws Exception { + path.chmod(mode); + return path.mode(); + } + @Test public void deleteRecursiveOnUnix() throws Exception { assumeFalse("Uses Unix-specific features", Functions.isWindows()); Path targetDir = temp.newFolder("target").toPath(); diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index 3cef81aae9..21a8993de4 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -36,6 +36,7 @@ import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.file.attribute.PosixFilePermissions; import static org.hamcrest.CoreMatchers.allOf; import static org.hamcrest.CoreMatchers.containsString; @@ -43,6 +44,7 @@ import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.startsWith; import static org.junit.Assert.*; import org.apache.commons.io.FileUtils; @@ -724,4 +726,41 @@ public class UtilTest { assertTrue(Util.isDescendant(new File(root, "."), new File(new File(root, "child"), "."))); } + @Test + public void testModeToPermissions() throws Exception { + assertEquals(PosixFilePermissions.fromString("rwxrwxrwx"), Util.modeToPermissions(0777)); + assertEquals(PosixFilePermissions.fromString("rwxr-xrwx"), Util.modeToPermissions(0757)); + assertEquals(PosixFilePermissions.fromString("rwxr-x---"), Util.modeToPermissions(0750)); + assertEquals(PosixFilePermissions.fromString("r-xr-x---"), Util.modeToPermissions(0550)); + assertEquals(PosixFilePermissions.fromString("r-xr-----"), Util.modeToPermissions(0540)); + assertEquals(PosixFilePermissions.fromString("--xr-----"), Util.modeToPermissions(0140)); + assertEquals(PosixFilePermissions.fromString("--xr---w-"), Util.modeToPermissions(0142)); + assertEquals(PosixFilePermissions.fromString("--xr--rw-"), Util.modeToPermissions(0146)); + assertEquals(PosixFilePermissions.fromString("-wxr--rw-"), Util.modeToPermissions(0346)); + assertEquals(PosixFilePermissions.fromString("---------"), Util.modeToPermissions(0000)); + + assertEquals("Non-permission bits should be ignored", PosixFilePermissions.fromString("r-xr-----"), Util.modeToPermissions(0100540)); + + try { + Util.modeToPermissions(01777); + fail("Did not detect invalid mode"); + } catch (IOException e) { + assertThat(e.getMessage(), startsWith("Invalid mode")); + } + } + + @Test + public void testPermissionsToMode() throws Exception { + assertEquals(0777, Util.permissionsToMode(PosixFilePermissions.fromString("rwxrwxrwx"))); + assertEquals(0757, Util.permissionsToMode(PosixFilePermissions.fromString("rwxr-xrwx"))); + assertEquals(0750, Util.permissionsToMode(PosixFilePermissions.fromString("rwxr-x---"))); + assertEquals(0550, Util.permissionsToMode(PosixFilePermissions.fromString("r-xr-x---"))); + assertEquals(0540, Util.permissionsToMode(PosixFilePermissions.fromString("r-xr-----"))); + assertEquals(0140, Util.permissionsToMode(PosixFilePermissions.fromString("--xr-----"))); + assertEquals(0142, Util.permissionsToMode(PosixFilePermissions.fromString("--xr---w-"))); + assertEquals(0146, Util.permissionsToMode(PosixFilePermissions.fromString("--xr--rw-"))); + assertEquals(0346, Util.permissionsToMode(PosixFilePermissions.fromString("-wxr--rw-"))); + assertEquals(0000, Util.permissionsToMode(PosixFilePermissions.fromString("---------"))); + } + } diff --git a/core/src/test/java/hudson/util/io/TarArchiverTest.java b/core/src/test/java/hudson/util/io/TarArchiverTest.java index 144be06c55..7635ccc97b 100644 --- a/core/src/test/java/hudson/util/io/TarArchiverTest.java +++ b/core/src/test/java/hudson/util/io/TarArchiverTest.java @@ -83,9 +83,9 @@ public class TarArchiverTest { // extract via the tar command run(e, "tar", "xvpf", tar.getAbsolutePath()); - assertEquals(0100755,e.child("a.txt").mode()); + assertEquals(0755,e.child("a.txt").mode()); assertEquals(dirMode,e.child("subdir").mode()); - assertEquals(0100644,e.child("subdir/b.txt").mode()); + assertEquals(0644,e.child("subdir/b.txt").mode()); // extract via the zip command @@ -93,9 +93,9 @@ public class TarArchiverTest { run(e, "unzip", zip.getAbsolutePath()); e = e.listDirectories().get(0); - assertEquals(0100755, e.child("a.txt").mode()); + assertEquals(0755, e.child("a.txt").mode()); assertEquals(dirMode,e.child("subdir").mode()); - assertEquals(0100644,e.child("subdir/b.txt").mode()); + assertEquals(0644,e.child("subdir/b.txt").mode()); } finally { tar.delete(); zip.delete(); -- GitLab