Skip to content

Commit fdbf77d

Browse files
cpovirkcpovirk
committed
Fix Files.createTempDir and FileBackedOutputStream under Windows.
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: 538888769
1 parent 29a9c7c commit fdbf77d

File tree

10 files changed

+178
-90
lines changed

10 files changed

+178
-90
lines changed

‎android/guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
package com.google.common.io;
1818

19-
import static com.google.common.base.StandardSystemProperty.OS_NAME;
2019
import static com.google.common.io.FileBackedOutputStreamTest.write;
2120

2221
import com.google.common.testing.GcFinalization;
@@ -31,9 +30,6 @@
3130
public class FileBackedOutputStreamAndroidIncompatibleTest extends IoTestCase {
3231

3332
public void testFinalizeDeletesFile() throws Exception {
34-
if (isWindows()) {
35-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
36-
}
3733
byte[] data = newPreFilledByteArray(100);
3834
FileBackedOutputStream out = new FileBackedOutputStream(0, true);
3935

@@ -55,8 +51,4 @@ public boolean isDone() {
5551
}
5652
});
5753
}
58-
59-
private static boolean isWindows() {
60-
return OS_NAME.value().startsWith("Windows");
61-
}
6254
}

‎android/guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,6 @@ public class FileBackedOutputStreamTest extends IoTestCase {
4141

4242

4343
public void testThreshold() throws Exception {
44-
if (isWindows()) {
45-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
46-
}
4744
testThreshold(0, 100, true, false);
4845
testThreshold(10, 100, true, false);
4946
testThreshold(100, 100, true, false);
@@ -82,7 +79,7 @@ private void testThreshold(
8279
assertEquals(dataSize, file.length());
8380
assertTrue(file.exists());
8481
assertThat(file.getName()).contains("FileBackedOutputStream");
85-
if (!isAndroid()) {
82+
if (!isAndroid() && !isWindows()) {
8683
PosixFileAttributes attributes =
8784
java.nio.file.Files.getFileAttributeView(file.toPath(), PosixFileAttributeView.class)
8885
.readAttributes();
@@ -103,9 +100,6 @@ private void testThreshold(
103100

104101

105102
public void testThreshold_resetOnFinalize() throws Exception {
106-
if (isWindows()) {
107-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
108-
}
109103
testThreshold(0, 100, true, true);
110104
testThreshold(10, 100, true, true);
111105
testThreshold(100, 100, true, true);
@@ -131,9 +125,6 @@ static void write(OutputStream out, byte[] b, int off, int len, boolean singleBy
131125
// TODO(chrisn): only works if we ensure we have crossed file threshold
132126

133127
public void testWriteErrorAfterClose() throws Exception {
134-
if (isWindows()) {
135-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
136-
}
137128
byte[] data = newPreFilledByteArray(100);
138129
FileBackedOutputStream out = new FileBackedOutputStream(50);
139130
ByteSource source = out.asByteSource();

‎android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,28 +39,28 @@
3939
@SuppressWarnings("deprecation") // tests of a deprecated method
4040
public class FilesCreateTempDirTest extends TestCase {
4141
public void testCreateTempDir() throws IOException {
42-
if (isWindows()) {
43-
return; // TODO: b/285742623 - Fix Files.createTempDir under Windows.
44-
}
4542
if (JAVA_IO_TMPDIR.value().equals("/sdcard")) {
4643
assertThrows(IllegalStateException.class, Files::createTempDir);
4744
return;
4845
}
4946
File temp = Files.createTempDir();
5047
try {
51-
assertTrue(temp.exists());
52-
assertTrue(temp.isDirectory());
48+
assertThat(temp.exists()).isTrue();
49+
assertThat(temp.isDirectory()).isTrue();
5350
assertThat(temp.listFiles()).isEmpty();
51+
File child = new File(temp, "child");
52+
assertThat(child.createNewFile()).isTrue();
53+
assertThat(child.delete()).isTrue();
5454

55-
if (isAndroid()) {
56-
return;
55+
if (!isAndroid() && !isWindows()) {
56+
PosixFileAttributes attributes =
57+
java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class)
58+
.readAttributes();
59+
assertThat(attributes.permissions())
60+
.containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE);
5761
}
58-
PosixFileAttributes attributes =
59-
java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class)
60-
.readAttributes();
61-
assertThat(attributes.permissions()).containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE);
6262
} finally {
63-
assertTrue(temp.delete());
63+
assertThat(temp.delete()).isTrue();
6464
}
6565
}
6666

‎android/guava-tests/test/com/google/common/reflect/ClassPathTest.java‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -174,13 +174,6 @@ public void testToFile_AndroidIncompatible() throws Exception {
174174
@AndroidIncompatible // Android forbids null parent ClassLoader
175175
// https://github.com/google/guava/issues/2152
176176
public void testJarFileWithSpaces() throws Exception {
177-
if (isWindows()) {
178-
/*
179-
* TODO: b/285742623 - Fix c.g.c.io.Files.createTempDir under Windows. Or use java.nio.files
180-
* instead?
181-
*/
182-
return;
183-
}
184177
URL url = makeJarUrlWithName("To test unescaped spaces in jar file name.jar");
185178
URLClassLoader classloader = new URLClassLoader(new URL[] {url}, null);
186179
assertThat(ClassPath.from(classloader).getTopLevelClasses()).isNotEmpty();
@@ -570,6 +563,10 @@ private static File fullpath(String path) {
570563
}
571564

572565
private static URL makeJarUrlWithName(String name) throws IOException {
566+
/*
567+
* TODO: cpovirk - Use java.nio.file.Files.createTempDirectory instead of
568+
* c.g.c.io.Files.createTempDir?
569+
*/
573570
File fullPath = new File(Files.createTempDir(), name);
574571
File jarFile = pickAnyJarFile();
575572
Files.copy(jarFile, fullPath);

‎android/guava/src/com/google/common/io/TempFileCreator.java‎

Lines changed: 72 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,26 @@
1515
package com.google.common.io;
1616

1717
import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
18+
import static com.google.common.base.StandardSystemProperty.USER_NAME;
19+
import static java.nio.file.attribute.AclEntryFlag.DIRECTORY_INHERIT;
20+
import static java.nio.file.attribute.AclEntryFlag.FILE_INHERIT;
21+
import static java.nio.file.attribute.AclEntryType.ALLOW;
22+
import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute;
1823

1924
import com.google.common.annotations.GwtIncompatible;
2025
import com.google.common.annotations.J2ktIncompatible;
26+
import com.google.common.collect.ImmutableList;
2127
import com.google.j2objc.annotations.J2ObjCIncompatible;
2228
import java.io.File;
2329
import java.io.IOException;
30+
import java.nio.file.FileSystems;
2431
import java.nio.file.Paths;
32+
import java.nio.file.attribute.AclEntry;
33+
import java.nio.file.attribute.AclEntryPermission;
2534
import java.nio.file.attribute.FileAttribute;
26-
import java.nio.file.attribute.PosixFilePermission;
2735
import java.nio.file.attribute.PosixFilePermissions;
36+
import java.nio.file.attribute.UserPrincipal;
37+
import java.util.EnumSet;
2838
import java.util.Set;
2939

3040
/**
@@ -90,16 +100,11 @@ private static TempFileCreator pickSecureCreator() {
90100

91101
@IgnoreJRERequirement // used only when Path is available
92102
private static final class JavaNioCreator extends TempFileCreator {
93-
private static final FileAttribute<Set<PosixFilePermission>> RWX_USER_ONLY =
94-
PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"));
95-
private static final FileAttribute<Set<PosixFilePermission>> RW_USER_ONLY =
96-
PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-------"));
97-
98103
@Override
99104
File createTempDir() {
100105
try {
101106
return java.nio.file.Files.createTempDirectory(
102-
Paths.get(JAVA_IO_TMPDIR.value()), /* prefix= */ null, RWX_USER_ONLY)
107+
Paths.get(JAVA_IO_TMPDIR.value()), /* prefix= */ null, directoryPermissions.get())
103108
.toFile();
104109
} catch (IOException e) {
105110
throw new IllegalStateException("Failed to create directory", e);
@@ -112,9 +117,68 @@ File createTempFile(String prefix) throws IOException {
112117
Paths.get(JAVA_IO_TMPDIR.value()),
113118
/* prefix= */ prefix,
114119
/* suffix= */ null,
115-
RW_USER_ONLY)
120+
filePermissions.get())
116121
.toFile();
117122
}
123+
124+
@IgnoreJRERequirement // see enclosing class (whose annotation Animal Sniffer ignores here...)
125+
private interface PermissionSupplier {
126+
FileAttribute<?> get() throws IOException;
127+
}
128+
129+
private static final PermissionSupplier filePermissions;
130+
private static final PermissionSupplier directoryPermissions;
131+
132+
static {
133+
Set<String> views = FileSystems.getDefault().supportedFileAttributeViews();
134+
if (views.contains("posix")) {
135+
filePermissions = () -> asFileAttribute(PosixFilePermissions.fromString("rw-------"));
136+
directoryPermissions = () -> asFileAttribute(PosixFilePermissions.fromString("rwx------"));
137+
} else if (views.contains("acl")) {
138+
filePermissions = directoryPermissions = userPermissions();
139+
} else {
140+
filePermissions =
141+
directoryPermissions =
142+
() -> {
143+
throw new IOException("unrecognized FileSystem type " + FileSystems.getDefault());
144+
};
145+
}
146+
}
147+
148+
private static PermissionSupplier userPermissions() {
149+
try {
150+
UserPrincipal user =
151+
FileSystems.getDefault()
152+
.getUserPrincipalLookupService()
153+
.lookupPrincipalByName(USER_NAME.value());
154+
ImmutableList<AclEntry> acl =
155+
ImmutableList.of(
156+
AclEntry.newBuilder()
157+
.setType(ALLOW)
158+
.setPrincipal(user)
159+
.setPermissions(EnumSet.allOf(AclEntryPermission.class))
160+
.setFlags(DIRECTORY_INHERIT, FILE_INHERIT)
161+
.build());
162+
FileAttribute<ImmutableList<AclEntry>> attribute =
163+
new FileAttribute<ImmutableList<AclEntry>>() {
164+
@Override
165+
public String name() {
166+
return "acl:acl";
167+
}
168+
169+
@Override
170+
public ImmutableList<AclEntry> value() {
171+
return acl;
172+
}
173+
};
174+
return () -> attribute;
175+
} catch (IOException e) {
176+
// We throw a new exception each time so that the stack trace is right.
177+
return () -> {
178+
throw new IOException("Could not find user", e);
179+
};
180+
}
181+
}
118182
}
119183

120184
private static final class JavaIoCreator extends TempFileCreator {

‎guava-tests/test/com/google/common/io/FileBackedOutputStreamAndroidIncompatibleTest.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
package com.google.common.io;
1818

19-
import static com.google.common.base.StandardSystemProperty.OS_NAME;
2019
import static com.google.common.io.FileBackedOutputStreamTest.write;
2120

2221
import com.google.common.testing.GcFinalization;
@@ -31,9 +30,6 @@
3130
public class FileBackedOutputStreamAndroidIncompatibleTest extends IoTestCase {
3231

3332
public void testFinalizeDeletesFile() throws Exception {
34-
if (isWindows()) {
35-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
36-
}
3733
byte[] data = newPreFilledByteArray(100);
3834
FileBackedOutputStream out = new FileBackedOutputStream(0, true);
3935

@@ -55,8 +51,4 @@ public boolean isDone() {
5551
}
5652
});
5753
}
58-
59-
private static boolean isWindows() {
60-
return OS_NAME.value().startsWith("Windows");
61-
}
6254
}

‎guava-tests/test/com/google/common/io/FileBackedOutputStreamTest.java‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,6 @@ public class FileBackedOutputStreamTest extends IoTestCase {
4141

4242

4343
public void testThreshold() throws Exception {
44-
if (isWindows()) {
45-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
46-
}
4744
testThreshold(0, 100, true, false);
4845
testThreshold(10, 100, true, false);
4946
testThreshold(100, 100, true, false);
@@ -82,7 +79,7 @@ private void testThreshold(
8279
assertEquals(dataSize, file.length());
8380
assertTrue(file.exists());
8481
assertThat(file.getName()).contains("FileBackedOutputStream");
85-
if (!isAndroid()) {
82+
if (!isAndroid() && !isWindows()) {
8683
PosixFileAttributes attributes =
8784
java.nio.file.Files.getFileAttributeView(file.toPath(), PosixFileAttributeView.class)
8885
.readAttributes();
@@ -103,9 +100,6 @@ private void testThreshold(
103100

104101

105102
public void testThreshold_resetOnFinalize() throws Exception {
106-
if (isWindows()) {
107-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
108-
}
109103
testThreshold(0, 100, true, true);
110104
testThreshold(10, 100, true, true);
111105
testThreshold(100, 100, true, true);
@@ -131,9 +125,6 @@ static void write(OutputStream out, byte[] b, int off, int len, boolean singleBy
131125
// TODO(chrisn): only works if we ensure we have crossed file threshold
132126

133127
public void testWriteErrorAfterClose() throws Exception {
134-
if (isWindows()) {
135-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
136-
}
137128
byte[] data = newPreFilledByteArray(100);
138129
FileBackedOutputStream out = new FileBackedOutputStream(50);
139130
ByteSource source = out.asByteSource();

‎guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,28 +39,28 @@
3939
@SuppressWarnings("deprecation") // tests of a deprecated method
4040
public class FilesCreateTempDirTest extends TestCase {
4141
public void testCreateTempDir() throws IOException {
42-
if (isWindows()) {
43-
return; // TODO: b/285742623 - Fix Files.createTempDir under Windows.
44-
}
4542
if (JAVA_IO_TMPDIR.value().equals("/sdcard")) {
4643
assertThrows(IllegalStateException.class, Files::createTempDir);
4744
return;
4845
}
4946
File temp = Files.createTempDir();
5047
try {
51-
assertTrue(temp.exists());
52-
assertTrue(temp.isDirectory());
48+
assertThat(temp.exists()).isTrue();
49+
assertThat(temp.isDirectory()).isTrue();
5350
assertThat(temp.listFiles()).isEmpty();
51+
File child = new File(temp, "child");
52+
assertThat(child.createNewFile()).isTrue();
53+
assertThat(child.delete()).isTrue();
5454

55-
if (isAndroid()) {
56-
return;
55+
if (!isAndroid() && !isWindows()) {
56+
PosixFileAttributes attributes =
57+
java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class)
58+
.readAttributes();
59+
assertThat(attributes.permissions())
60+
.containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE);
5761
}
58-
PosixFileAttributes attributes =
59-
java.nio.file.Files.getFileAttributeView(temp.toPath(), PosixFileAttributeView.class)
60-
.readAttributes();
61-
assertThat(attributes.permissions()).containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE);
6262
} finally {
63-
assertTrue(temp.delete());
63+
assertThat(temp.delete()).isTrue();
6464
}
6565
}
6666

‎guava-tests/test/com/google/common/reflect/ClassPathTest.java‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -180,13 +180,6 @@ public void testToFile_AndroidIncompatible() throws Exception {
180180
@AndroidIncompatible // Android forbids null parent ClassLoader
181181
// https://github.com/google/guava/issues/2152
182182
public void testJarFileWithSpaces() throws Exception {
183-
if (isWindows()) {
184-
/*
185-
* TODO: b/285742623 - Fix c.g.c.io.Files.createTempDir under Windows. Or use java.nio.files
186-
* instead?
187-
*/
188-
return;
189-
}
190183
URL url = makeJarUrlWithName("To test unescaped spaces in jar file name.jar");
191184
URLClassLoader classloader = new URLClassLoader(new URL[] {url}, null);
192185
assertThat(ClassPath.from(classloader).getTopLevelClasses()).isNotEmpty();
@@ -635,6 +628,10 @@ private static File fullpath(String path) {
635628
}
636629

637630
private static URL makeJarUrlWithName(String name) throws IOException {
631+
/*
632+
* TODO: cpovirk - Use java.nio.file.Files.createTempDirectory instead of
633+
* c.g.c.io.Files.createTempDir?
634+
*/
638635
File fullPath = new File(Files.createTempDir(), name);
639636
File jarFile = pickAnyJarFile();
640637
Files.copy(jarFile, fullPath);

0 commit comments

Comments
 (0)