Skip to content

Commit

Permalink
Fix Files.createTempDir and FileBackedOutputStream under Windows.
Browse files Browse the repository at this point in the history 
Based on [some discussions in GitHub](#6535 (comment)), 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 #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed Jun 8, 2023
1 parent d7c43ab commit 4cd5a75
Show file tree
Hide file tree
Showing 10 changed files with 196 additions and 96 deletions.
8 changes: 0 additions & 8 deletions .../guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand All @@ -55,8 +51,4 @@ public boolean isDone() {
}
});
}

private static boolean isWindows() {
return OS_NAME.value().startsWith("Windows");
}
}
14 changes: 0 additions & 14 deletions android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.google.common.io;

import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
import static com.google.common.base.StandardSystemProperty.OS_NAME;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
Expand All @@ -41,9 +40,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);
Expand Down Expand Up @@ -103,9 +99,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);
Expand All @@ -131,9 +124,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();
Expand Down Expand Up @@ -177,8 +167,4 @@ public void testReset() throws Exception {
private static boolean isAndroid() {
return System.getProperty("java.runtime.name", "").contains("Android");
}

private static boolean isWindows() {
return OS_NAME.value().startsWith("Windows");
}
}
17 changes: 6 additions & 11 deletions android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.google.common.io;

import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
import static com.google.common.base.StandardSystemProperty.OS_NAME;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
Expand All @@ -39,18 +38,18 @@
@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;
Expand All @@ -60,15 +59,11 @@ public void testCreateTempDir() throws IOException {
.readAttributes();
assertThat(attributes.permissions()).containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE);
} finally {
assertTrue(temp.delete());
assertThat(temp.delete()).isTrue();
}
}

private static boolean isAndroid() {
return System.getProperty("java.runtime.name", "").contains("Android");
}

private static boolean isWindows() {
return OS_NAME.value().startsWith("Windows");
}
}
11 changes: 4 additions & 7 deletions android/guava-tests/test/com/google/common/reflect/ClassPathTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
96 changes: 88 additions & 8 deletions android/guava/src/com/google/common/io/TempFileCreator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -90,16 +100,13 @@ private static TempFileCreator pickSecureCreator() {

@IgnoreJRERequirement // used only when Path is available
private static final class JavaNioCreator extends TempFileCreator {
private static final FileAttribute<Set<PosixFilePermission>> RWX_USER_ONLY =
PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"));
private static final FileAttribute<Set<PosixFilePermission>> 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,
PermissionSupplier.INSTANCE.directoryPermissions.get())
.toFile();
} catch (IOException e) {
throw new IllegalStateException("Failed to create directory", e);
Expand All @@ -112,9 +119,82 @@ File createTempFile(String prefix) throws IOException {
Paths.get(JAVA_IO_TMPDIR.value()),
/* prefix= */ prefix,
/* suffix= */ null,
RW_USER_ONLY)
PermissionSupplier.INSTANCE.filePermissions.get())
.toFile();
}

private interface IoSupplier<T> {
T get() throws IOException;
}

private static final class PermissionSupplier {
static final PermissionSupplier INSTANCE = pickSupplier();

final IoSupplier<FileAttribute<?>> filePermissions;
final IoSupplier<FileAttribute<?>> directoryPermissions;

private PermissionSupplier(
IoSupplier<FileAttribute<?>> filePermissions,
IoSupplier<FileAttribute<?>> directoryPermissions) {
this.filePermissions = filePermissions;
this.directoryPermissions = directoryPermissions;
}

private PermissionSupplier(IoSupplier<FileAttribute<?>> filePermissions) {
this(filePermissions, filePermissions);
}

private static PermissionSupplier pickSupplier() {
Set<String> views = FileSystems.getDefault().supportedFileAttributeViews();
if (views.contains("posix")) {
return new PermissionSupplier(
() -> asFileAttribute(PosixFilePermissions.fromString("rw-------")),
() -> asFileAttribute(PosixFilePermissions.fromString("rwx------")));
} else if (views.contains("acl")) {
return new PermissionSupplier(makeAclSupplier());
} else {
return new PermissionSupplier(
() -> {
throw new IOException("unrecognized FileSystem type " + FileSystems.getDefault());
});
}
}

private static IoSupplier<FileAttribute<?>> makeAclSupplier() {
try {
UserPrincipal user =
FileSystems.getDefault()
.getUserPrincipalLookupService()
.lookupPrincipalByName(USER_NAME.value());
ImmutableList<AclEntry> acl =
ImmutableList.of(
AclEntry.newBuilder()
.setType(ALLOW)
.setPrincipal(user)
.setPermissions(EnumSet.allOf(AclEntryPermission.class))
.setFlags(DIRECTORY_INHERIT, FILE_INHERIT)
.build());
FileAttribute<ImmutableList<AclEntry>> attribute =
new FileAttribute<>() {
@Override
public String name() {
return "acl:acl";
}

@Override
public ImmutableList<AclEntry> 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 {
Expand Down
8 changes: 0 additions & 8 deletions guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand All @@ -55,8 +51,4 @@ public boolean isDone() {
}
});
}

private static boolean isWindows() {
return OS_NAME.value().startsWith("Windows");
}
}
14 changes: 0 additions & 14 deletions guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.google.common.io;

import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
import static com.google.common.base.StandardSystemProperty.OS_NAME;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
Expand All @@ -41,9 +40,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);
Expand Down Expand Up @@ -103,9 +99,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);
Expand All @@ -131,9 +124,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();
Expand Down Expand Up @@ -177,8 +167,4 @@ public void testReset() throws Exception {
private static boolean isAndroid() {
return System.getProperty("java.runtime.name", "").contains("Android");
}

private static boolean isWindows() {
return OS_NAME.value().startsWith("Windows");
}
}
Loading

0 comments on commit 4cd5a75

Please sign in to comment.