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 7dc83d1
Show file tree
Hide file tree
Showing 10 changed files with 196 additions and 96 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 @@ -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");
}
}
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");
}
}
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
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<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 @@ -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 7dc83d1

Please sign in to comment.