From fdbf77d3f2b826fc0a70b1f9b9994b140ddf3bd8 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Thu, 8 Jun 2023 14:16:17 -0700 Subject: [PATCH] Fix `Files.createTempDir` and `FileBackedOutputStream` under Windows. Based on [some discussions in GitHub](https://github.com/google/guava/issues/6535#issuecomment-1579806211), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer. Fixes https://github.com/google/guava/issues/6535 Relates to: - https://github.com/google/guava/issues/4011 - https://github.com/google/guava/issues/2575 - https://github.com/google/guava/issues/2686 (Yay, we have Windows CI set up!) - https://github.com/google/guava/issues/2130 RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows. PiperOrigin-RevId: 538888769 --- ...edOutputStreamAndroidIncompatibleTest.java | 8 -- .../common/io/FileBackedOutputStreamTest.java | 11 +-- .../common/io/FilesCreateTempDirTest.java | 24 +++--- .../google/common/reflect/ClassPathTest.java | 11 +-- .../com/google/common/io/TempFileCreator.java | 80 +++++++++++++++++-- ...edOutputStreamAndroidIncompatibleTest.java | 8 -- .../common/io/FileBackedOutputStreamTest.java | 11 +-- .../common/io/FilesCreateTempDirTest.java | 24 +++--- .../google/common/reflect/ClassPathTest.java | 11 +-- .../com/google/common/io/TempFileCreator.java | 80 +++++++++++++++++-- 10 files changed, 178 insertions(+), 90 deletions(-) diff --git a/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java b/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java index 9d25e42affb3..4ecdcf8d5688 100644 --- a/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java +++ b/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java @@ -16,7 +16,6 @@ package com.google.common.io; -import static com.google.common.base.StandardSystemProperty.OS_NAME; import static com.google.common.io.FileBackedOutputStreamTest.write; import com.google.common.testing.GcFinalization; @@ -31,9 +30,6 @@ public class FileBackedOutputStreamAndroidIncompatibleTest extends IoTestCase { public void testFinalizeDeletesFile() throws Exception { - if (isWindows()) { - return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows. - } byte[] data = newPreFilledByteArray(100); FileBackedOutputStream out = new FileBackedOutputStream(0, true); @@ -55,8 +51,4 @@ public boolean isDone() { } }); } - - private static boolean isWindows() { - return OS_NAME.value().startsWith("Windows"); - } } diff --git a/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java b/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java index 2924cc081909..3d756a88bf9d 100644 --- a/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java +++ b/android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java @@ -41,9 +41,6 @@ public class FileBackedOutputStreamTest extends IoTestCase { public void testThreshold() throws Exception { - if (isWindows()) { - return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows. - } testThreshold(0, 100, true, false); testThreshold(10, 100, true, false); testThreshold(100, 100, true, false); @@ -82,7 +79,7 @@ private void testThreshold( assertEquals(dataSize, file.length()); assertTrue(file.exists()); assertThat(file.getName()).contains("FileBackedOutputStream"); - if (!isAndroid()) { + if (!isAndroid() && !isWindows()) { PosixFileAttributes attributes = java.nio.file.Files.getFileAttributeView(file.toPath(), PosixFileAttributeView.class) .readAttributes(); @@ -103,9 +100,6 @@ private void testThreshold( public void testThreshold_resetOnFinalize() throws Exception { - if (isWindows()) { - return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows. - } testThreshold(0, 100, true, true); testThreshold(10, 100, true, true); testThreshold(100, 100, true, true); @@ -131,9 +125,6 @@ static void write(OutputStream out, byte[] b, int off, int len, boolean singleBy // TODO(chrisn): only works if we ensure we have crossed file threshold public void testWriteErrorAfterClose() throws Exception { - if (isWindows()) { - return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows. - } byte[] data = newPreFilledByteArray(100); FileBackedOutputStream out = new FileBackedOutputStream(50); ByteSource source = out.asByteSource(); diff --git a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index 228891d27255..a31c43770e3b 100644 --- a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -39,28 +39,28 @@ @SuppressWarnings("deprecation") // tests of a deprecated method public class FilesCreateTempDirTest extends TestCase { public void testCreateTempDir() throws IOException { - if (isWindows()) { - return; // TODO: b/285742623 - Fix Files.createTempDir under Windows. - } if (JAVA_IO_TMPDIR.value().equals("/sdcard")) { assertThrows(IllegalStateException.class, Files::createTempDir); return; } File temp = Files.createTempDir(); try { - assertTrue(temp.exists()); - assertTrue(temp.isDirectory()); + assertThat(temp.exists()).isTrue(); + assertThat(temp.isDirectory()).isTrue(); assertThat(temp.listFiles()).isEmpty(); + File child = new File(temp, "child"); + assertThat(child.createNewFile()).isTrue(); + assertThat(child.delete()).isTrue(); - if (isAndroid()) { - return; + if (!isAndroid() && !isWindows()) { + PosixFileAttributes attributes = + java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class) + .readAttributes(); + assertThat(attributes.permissions()) + .containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE); } - PosixFileAttributes attributes = - java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class) - .readAttributes(); - assertThat(attributes.permissions()).containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE); } finally { - assertTrue(temp.delete()); + assertThat(temp.delete()).isTrue(); } } diff --git a/android/guava-tests/test/com/google/common/reflect/ClassPathTest.java b/android/guava-tests/test/com/google/common/reflect/ClassPathTest.java index f2de67c19b28..72e3cb5b7304 100644 --- a/android/guava-tests/test/com/google/common/reflect/ClassPathTest.java +++ b/android/guava-tests/test/com/google/common/reflect/ClassPathTest.java @@ -174,13 +174,6 @@ public void testToFile_AndroidIncompatible() throws Exception { @AndroidIncompatible // Android forbids null parent ClassLoader // https://github.com/google/guava/issues/2152 public void testJarFileWithSpaces() throws Exception { - if (isWindows()) { - /* - * TODO: b/285742623 - Fix c.g.c.io.Files.createTempDir under Windows. Or use java.nio.files - * instead? - */ - return; - } URL url = makeJarUrlWithName("To test unescaped spaces in jar file name.jar"); URLClassLoader classloader = new URLClassLoader(new URL[] {url}, null); assertThat(ClassPath.from(classloader).getTopLevelClasses()).isNotEmpty(); @@ -570,6 +563,10 @@ private static File fullpath(String path) { } private static URL makeJarUrlWithName(String name) throws IOException { + /* + * TODO: cpovirk - Use java.nio.file.Files.createTempDirectory instead of + * c.g.c.io.Files.createTempDir? + */ File fullPath = new File(Files.createTempDir(), name); File jarFile = pickAnyJarFile(); Files.copy(jarFile, fullPath); diff --git a/android/guava/src/com/google/common/io/TempFileCreator.java b/android/guava/src/com/google/common/io/TempFileCreator.java index 03c5ae99cec6..96632359906e 100644 --- a/android/guava/src/com/google/common/io/TempFileCreator.java +++ b/android/guava/src/com/google/common/io/TempFileCreator.java @@ -15,16 +15,26 @@ package com.google.common.io; import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR; +import static com.google.common.base.StandardSystemProperty.USER_NAME; +import static java.nio.file.attribute.AclEntryFlag.DIRECTORY_INHERIT; +import static java.nio.file.attribute.AclEntryFlag.FILE_INHERIT; +import static java.nio.file.attribute.AclEntryType.ALLOW; +import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute; import com.google.common.annotations.GwtIncompatible; import com.google.common.annotations.J2ktIncompatible; +import com.google.common.collect.ImmutableList; import com.google.j2objc.annotations.J2ObjCIncompatible; import java.io.File; import java.io.IOException; +import java.nio.file.FileSystems; import java.nio.file.Paths; +import java.nio.file.attribute.AclEntry; +import java.nio.file.attribute.AclEntryPermission; import java.nio.file.attribute.FileAttribute; -import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; +import java.nio.file.attribute.UserPrincipal; +import java.util.EnumSet; import java.util.Set; /** @@ -90,16 +100,11 @@ private static TempFileCreator pickSecureCreator() { @IgnoreJRERequirement // used only when Path is available private static final class JavaNioCreator extends TempFileCreator { - private static final FileAttribute> RWX_USER_ONLY = - PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------")); - private static final FileAttribute> RW_USER_ONLY = - PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-------")); - @Override File createTempDir() { try { return java.nio.file.Files.createTempDirectory( - Paths.get(JAVA_IO_TMPDIR.value()), /* prefix= */ null, RWX_USER_ONLY) + Paths.get(JAVA_IO_TMPDIR.value()), /* prefix= */ null, directoryPermissions.get()) .toFile(); } catch (IOException e) { throw new IllegalStateException("Failed to create directory", e); @@ -112,9 +117,68 @@ File createTempFile(String prefix) throws IOException { Paths.get(JAVA_IO_TMPDIR.value()), /* prefix= */ prefix, /* suffix= */ null, - RW_USER_ONLY) + filePermissions.get()) .toFile(); } + + @IgnoreJRERequirement // see enclosing class (whose annotation Animal Sniffer ignores here...) + private interface PermissionSupplier { + FileAttribute get() throws IOException; + } + + private static final PermissionSupplier filePermissions; + private static final PermissionSupplier directoryPermissions; + + static { + Set views = FileSystems.getDefault().supportedFileAttributeViews(); + if (views.contains("posix")) { + filePermissions = () -> asFileAttribute(PosixFilePermissions.fromString("rw-------")); + directoryPermissions = () -> asFileAttribute(PosixFilePermissions.fromString("rwx------")); + } else if (views.contains("acl")) { + filePermissions = directoryPermissions = userPermissions(); + } else { + filePermissions = + directoryPermissions = + () -> { + throw new IOException("unrecognized FileSystem type " + FileSystems.getDefault()); + }; + } + } + + private static PermissionSupplier userPermissions() { + try { + UserPrincipal user = + FileSystems.getDefault() + .getUserPrincipalLookupService() + .lookupPrincipalByName(USER_NAME.value()); + ImmutableList acl = + ImmutableList.of( + AclEntry.newBuilder() + .setType(ALLOW) + .setPrincipal(user) + .setPermissions(EnumSet.allOf(AclEntryPermission.class)) + .setFlags(DIRECTORY_INHERIT, FILE_INHERIT) + .build()); + FileAttribute> attribute = + new FileAttribute>() { + @Override + public String name() { + return "acl:acl"; + } + + @Override + public ImmutableList value() { + return acl; + } + }; + return () -> attribute; + } catch (IOException e) { + // We throw a new exception each time so that the stack trace is right. + return () -> { + throw new IOException("Could not find user", e); + }; + } + } } private static final class JavaIoCreator extends TempFileCreator { diff --git a/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java b/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java index 9d25e42affb3..4ecdcf8d5688 100644 --- a/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java +++ b/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java @@ -16,7 +16,6 @@ package com.google.common.io; -import static com.google.common.base.StandardSystemProperty.OS_NAME; import static com.google.common.io.FileBackedOutputStreamTest.write; import com.google.common.testing.GcFinalization; @@ -31,9 +30,6 @@ public class FileBackedOutputStreamAndroidIncompatibleTest extends IoTestCase { public void testFinalizeDeletesFile() throws Exception { - if (isWindows()) { - return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows. - } byte[] data = newPreFilledByteArray(100); FileBackedOutputStream out = new FileBackedOutputStream(0, true); @@ -55,8 +51,4 @@ public boolean isDone() { } }); } - - private static boolean isWindows() { - return OS_NAME.value().startsWith("Windows"); - } } diff --git a/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java b/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java index 2924cc081909..3d756a88bf9d 100644 --- a/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java +++ b/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java @@ -41,9 +41,6 @@ public class FileBackedOutputStreamTest extends IoTestCase { public void testThreshold() throws Exception { - if (isWindows()) { - return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows. - } testThreshold(0, 100, true, false); testThreshold(10, 100, true, false); testThreshold(100, 100, true, false); @@ -82,7 +79,7 @@ private void testThreshold( assertEquals(dataSize, file.length()); assertTrue(file.exists()); assertThat(file.getName()).contains("FileBackedOutputStream"); - if (!isAndroid()) { + if (!isAndroid() && !isWindows()) { PosixFileAttributes attributes = java.nio.file.Files.getFileAttributeView(file.toPath(), PosixFileAttributeView.class) .readAttributes(); @@ -103,9 +100,6 @@ private void testThreshold( public void testThreshold_resetOnFinalize() throws Exception { - if (isWindows()) { - return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows. - } testThreshold(0, 100, true, true); testThreshold(10, 100, true, true); testThreshold(100, 100, true, true); @@ -131,9 +125,6 @@ static void write(OutputStream out, byte[] b, int off, int len, boolean singleBy // TODO(chrisn): only works if we ensure we have crossed file threshold public void testWriteErrorAfterClose() throws Exception { - if (isWindows()) { - return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows. - } byte[] data = newPreFilledByteArray(100); FileBackedOutputStream out = new FileBackedOutputStream(50); ByteSource source = out.asByteSource(); diff --git a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index 228891d27255..a31c43770e3b 100644 --- a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -39,28 +39,28 @@ @SuppressWarnings("deprecation") // tests of a deprecated method public class FilesCreateTempDirTest extends TestCase { public void testCreateTempDir() throws IOException { - if (isWindows()) { - return; // TODO: b/285742623 - Fix Files.createTempDir under Windows. - } if (JAVA_IO_TMPDIR.value().equals("/sdcard")) { assertThrows(IllegalStateException.class, Files::createTempDir); return; } File temp = Files.createTempDir(); try { - assertTrue(temp.exists()); - assertTrue(temp.isDirectory()); + assertThat(temp.exists()).isTrue(); + assertThat(temp.isDirectory()).isTrue(); assertThat(temp.listFiles()).isEmpty(); + File child = new File(temp, "child"); + assertThat(child.createNewFile()).isTrue(); + assertThat(child.delete()).isTrue(); - if (isAndroid()) { - return; + if (!isAndroid() && !isWindows()) { + PosixFileAttributes attributes = + java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class) + .readAttributes(); + assertThat(attributes.permissions()) + .containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE); } - PosixFileAttributes attributes = - java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class) - .readAttributes(); - assertThat(attributes.permissions()).containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE); } finally { - assertTrue(temp.delete()); + assertThat(temp.delete()).isTrue(); } } diff --git a/guava-tests/test/com/google/common/reflect/ClassPathTest.java b/guava-tests/test/com/google/common/reflect/ClassPathTest.java index 587419576b72..9a95c4a6a4ea 100644 --- a/guava-tests/test/com/google/common/reflect/ClassPathTest.java +++ b/guava-tests/test/com/google/common/reflect/ClassPathTest.java @@ -180,13 +180,6 @@ public void testToFile_AndroidIncompatible() throws Exception { @AndroidIncompatible // Android forbids null parent ClassLoader // https://github.com/google/guava/issues/2152 public void testJarFileWithSpaces() throws Exception { - if (isWindows()) { - /* - * TODO: b/285742623 - Fix c.g.c.io.Files.createTempDir under Windows. Or use java.nio.files - * instead? - */ - return; - } URL url = makeJarUrlWithName("To test unescaped spaces in jar file name.jar"); URLClassLoader classloader = new URLClassLoader(new URL[] {url}, null); assertThat(ClassPath.from(classloader).getTopLevelClasses()).isNotEmpty(); @@ -635,6 +628,10 @@ private static File fullpath(String path) { } private static URL makeJarUrlWithName(String name) throws IOException { + /* + * TODO: cpovirk - Use java.nio.file.Files.createTempDirectory instead of + * c.g.c.io.Files.createTempDir? + */ File fullPath = new File(Files.createTempDir(), name); File jarFile = pickAnyJarFile(); Files.copy(jarFile, fullPath); diff --git a/guava/src/com/google/common/io/TempFileCreator.java b/guava/src/com/google/common/io/TempFileCreator.java index 03c5ae99cec6..96632359906e 100644 --- a/guava/src/com/google/common/io/TempFileCreator.java +++ b/guava/src/com/google/common/io/TempFileCreator.java @@ -15,16 +15,26 @@ package com.google.common.io; import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR; +import static com.google.common.base.StandardSystemProperty.USER_NAME; +import static java.nio.file.attribute.AclEntryFlag.DIRECTORY_INHERIT; +import static java.nio.file.attribute.AclEntryFlag.FILE_INHERIT; +import static java.nio.file.attribute.AclEntryType.ALLOW; +import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute; import com.google.common.annotations.GwtIncompatible; import com.google.common.annotations.J2ktIncompatible; +import com.google.common.collect.ImmutableList; import com.google.j2objc.annotations.J2ObjCIncompatible; import java.io.File; import java.io.IOException; +import java.nio.file.FileSystems; import java.nio.file.Paths; +import java.nio.file.attribute.AclEntry; +import java.nio.file.attribute.AclEntryPermission; import java.nio.file.attribute.FileAttribute; -import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; +import java.nio.file.attribute.UserPrincipal; +import java.util.EnumSet; import java.util.Set; /** @@ -90,16 +100,11 @@ private static TempFileCreator pickSecureCreator() { @IgnoreJRERequirement // used only when Path is available private static final class JavaNioCreator extends TempFileCreator { - private static final FileAttribute> RWX_USER_ONLY = - PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------")); - private static final FileAttribute> RW_USER_ONLY = - PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-------")); - @Override File createTempDir() { try { return java.nio.file.Files.createTempDirectory( - Paths.get(JAVA_IO_TMPDIR.value()), /* prefix= */ null, RWX_USER_ONLY) + Paths.get(JAVA_IO_TMPDIR.value()), /* prefix= */ null, directoryPermissions.get()) .toFile(); } catch (IOException e) { throw new IllegalStateException("Failed to create directory", e); @@ -112,9 +117,68 @@ File createTempFile(String prefix) throws IOException { Paths.get(JAVA_IO_TMPDIR.value()), /* prefix= */ prefix, /* suffix= */ null, - RW_USER_ONLY) + filePermissions.get()) .toFile(); } + + @IgnoreJRERequirement // see enclosing class (whose annotation Animal Sniffer ignores here...) + private interface PermissionSupplier { + FileAttribute get() throws IOException; + } + + private static final PermissionSupplier filePermissions; + private static final PermissionSupplier directoryPermissions; + + static { + Set views = FileSystems.getDefault().supportedFileAttributeViews(); + if (views.contains("posix")) { + filePermissions = () -> asFileAttribute(PosixFilePermissions.fromString("rw-------")); + directoryPermissions = () -> asFileAttribute(PosixFilePermissions.fromString("rwx------")); + } else if (views.contains("acl")) { + filePermissions = directoryPermissions = userPermissions(); + } else { + filePermissions = + directoryPermissions = + () -> { + throw new IOException("unrecognized FileSystem type " + FileSystems.getDefault()); + }; + } + } + + private static PermissionSupplier userPermissions() { + try { + UserPrincipal user = + FileSystems.getDefault() + .getUserPrincipalLookupService() + .lookupPrincipalByName(USER_NAME.value()); + ImmutableList acl = + ImmutableList.of( + AclEntry.newBuilder() + .setType(ALLOW) + .setPrincipal(user) + .setPermissions(EnumSet.allOf(AclEntryPermission.class)) + .setFlags(DIRECTORY_INHERIT, FILE_INHERIT) + .build()); + FileAttribute> attribute = + new FileAttribute>() { + @Override + public String name() { + return "acl:acl"; + } + + @Override + public ImmutableList value() { + return acl; + } + }; + return () -> attribute; + } catch (IOException e) { + // We throw a new exception each time so that the stack trace is right. + return () -> { + throw new IOException("Could not find user", e); + }; + } + } } private static final class JavaIoCreator extends TempFileCreator {