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 authored and Google Java Core Libraries committed Jun 8, 2023
1 parent d7c43ab commit 8d8ff0d
Show file tree
Hide file tree
Showing 10 changed files with 178 additions and 90 deletions.
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");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand Down
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
80 changes: 72 additions & 8 deletions android/guava/src/com/google/common/io/TempFileCreator.java
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,11 @@ 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, directoryPermissions.get())
.toFile();
} catch (IOException e) {
throw new IllegalStateException("Failed to create directory", e);
Expand All @@ -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<String> 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 = makeAclSupplier();
} else {
filePermissions =
directoryPermissions =
() -> {
throw new IOException("unrecognized FileSystem type " + FileSystems.getDefault());
};
}
}

private static PermissionSupplier 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<ImmutableList<AclEntry>>() {
@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
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");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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();
Expand Down
24 changes: 12 additions & 12 deletions guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand Down
11 changes: 4 additions & 7 deletions guava-tests/test/com/google/common/reflect/ClassPathTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
Loading

0 comments on commit 8d8ff0d

Please sign in to comment.