Skip to content

Commit 6b1b6e1

Browse files
cpovirkGoogle Java Core Libraries
cpovirk
authored and
Google Java Core Libraries
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: 538492945
1 parent d7c43ab commit 6b1b6e1

File tree

10 files changed

+198
-96
lines changed

10 files changed

+198
-96
lines changed

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

-8
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
}

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

-14
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package com.google.common.io;
1818

1919
import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
20-
import static com.google.common.base.StandardSystemProperty.OS_NAME;
2120
import static com.google.common.truth.Truth.assertThat;
2221
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
2322
import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
@@ -41,9 +40,6 @@ public class FileBackedOutputStreamTest extends IoTestCase {
4140

4241

4342
public void testThreshold() throws Exception {
44-
if (isWindows()) {
45-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
46-
}
4743
testThreshold(0, 100, true, false);
4844
testThreshold(10, 100, true, false);
4945
testThreshold(100, 100, true, false);
@@ -103,9 +99,6 @@ private void testThreshold(
10399

104100

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

133126
public void testWriteErrorAfterClose() throws Exception {
134-
if (isWindows()) {
135-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
136-
}
137127
byte[] data = newPreFilledByteArray(100);
138128
FileBackedOutputStream out = new FileBackedOutputStream(50);
139129
ByteSource source = out.asByteSource();
@@ -177,8 +167,4 @@ public void testReset() throws Exception {
177167
private static boolean isAndroid() {
178168
return System.getProperty("java.runtime.name", "").contains("Android");
179169
}
180-
181-
private static boolean isWindows() {
182-
return OS_NAME.value().startsWith("Windows");
183-
}
184170
}

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

+6-11
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package com.google.common.io;
1818

1919
import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
20-
import static com.google.common.base.StandardSystemProperty.OS_NAME;
2120
import static com.google.common.truth.Truth.assertThat;
2221
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
2322
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
@@ -39,18 +38,18 @@
3938
@SuppressWarnings("deprecation") // tests of a deprecated method
4039
public class FilesCreateTempDirTest extends TestCase {
4140
public void testCreateTempDir() throws IOException {
42-
if (isWindows()) {
43-
return; // TODO: b/285742623 - Fix Files.createTempDir under Windows.
44-
}
4541
if (JAVA_IO_TMPDIR.value().equals("/sdcard")) {
4642
assertThrows(IllegalStateException.class, Files::createTempDir);
4743
return;
4844
}
4945
File temp = Files.createTempDir();
5046
try {
51-
assertTrue(temp.exists());
52-
assertTrue(temp.isDirectory());
47+
assertThat(temp.exists()).isTrue();
48+
assertThat(temp.isDirectory()).isTrue();
5349
assertThat(temp.listFiles()).isEmpty();
50+
File child = new File(temp, "child");
51+
assertThat(child.createNewFile()).isTrue();
52+
assertThat(child.delete()).isTrue();
5453

5554
if (isAndroid()) {
5655
return;
@@ -60,15 +59,11 @@ public void testCreateTempDir() throws IOException {
6059
.readAttributes();
6160
assertThat(attributes.permissions()).containsExactly(OWNER_READ, OWNER_WRITE, OWNER_EXECUTE);
6261
} finally {
63-
assertTrue(temp.delete());
62+
assertThat(temp.delete()).isTrue();
6463
}
6564
}
6665

6766
private static boolean isAndroid() {
6867
return System.getProperty("java.runtime.name", "").contains("Android");
6968
}
70-
71-
private static boolean isWindows() {
72-
return OS_NAME.value().startsWith("Windows");
73-
}
7469
}

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

+4-7
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);

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

+89-8
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,13 @@ 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()),
108+
/* prefix= */ null,
109+
PermissionSupplier.INSTANCE.directoryPermissions.get())
103110
.toFile();
104111
} catch (IOException e) {
105112
throw new IllegalStateException("Failed to create directory", e);
@@ -112,9 +119,83 @@ File createTempFile(String prefix) throws IOException {
112119
Paths.get(JAVA_IO_TMPDIR.value()),
113120
/* prefix= */ prefix,
114121
/* suffix= */ null,
115-
RW_USER_ONLY)
122+
PermissionSupplier.INSTANCE.filePermissions.get())
116123
.toFile();
117124
}
125+
126+
private interface IoSupplier<T> {
127+
T get() throws IOException;
128+
}
129+
130+
@IgnoreJRERequirement // see enclosing class (whose annotation Animal Sniffer ignores here...)
131+
private static final class PermissionSupplier {
132+
static final PermissionSupplier INSTANCE = pickSupplier();
133+
134+
final IoSupplier<FileAttribute<?>> filePermissions;
135+
final IoSupplier<FileAttribute<?>> directoryPermissions;
136+
137+
private PermissionSupplier(
138+
IoSupplier<FileAttribute<?>> filePermissions,
139+
IoSupplier<FileAttribute<?>> directoryPermissions) {
140+
this.filePermissions = filePermissions;
141+
this.directoryPermissions = directoryPermissions;
142+
}
143+
144+
private PermissionSupplier(IoSupplier<FileAttribute<?>> filePermissions) {
145+
this(filePermissions, filePermissions);
146+
}
147+
148+
private static PermissionSupplier pickSupplier() {
149+
Set<String> views = FileSystems.getDefault().supportedFileAttributeViews();
150+
if (views.contains("posix")) {
151+
return new PermissionSupplier(
152+
() -> asFileAttribute(PosixFilePermissions.fromString("rw-------")),
153+
() -> asFileAttribute(PosixFilePermissions.fromString("rwx------")));
154+
} else if (views.contains("acl")) {
155+
return new PermissionSupplier(makeAclSupplier());
156+
} else {
157+
return new PermissionSupplier(
158+
() -> {
159+
throw new IOException("unrecognized FileSystem type " + FileSystems.getDefault());
160+
});
161+
}
162+
}
163+
164+
private static IoSupplier<FileAttribute<?>> makeAclSupplier() {
165+
try {
166+
UserPrincipal user =
167+
FileSystems.getDefault()
168+
.getUserPrincipalLookupService()
169+
.lookupPrincipalByName(USER_NAME.value());
170+
ImmutableList<AclEntry> acl =
171+
ImmutableList.of(
172+
AclEntry.newBuilder()
173+
.setType(ALLOW)
174+
.setPrincipal(user)
175+
.setPermissions(EnumSet.allOf(AclEntryPermission.class))
176+
.setFlags(DIRECTORY_INHERIT, FILE_INHERIT)
177+
.build());
178+
FileAttribute<ImmutableList<AclEntry>> attribute =
179+
new FileAttribute<ImmutableList<AclEntry>>() {
180+
@Override
181+
public String name() {
182+
return "acl:acl";
183+
}
184+
185+
@Override
186+
public ImmutableList<AclEntry> value() {
187+
return acl;
188+
}
189+
};
190+
return () -> attribute;
191+
} catch (IOException e) {
192+
// We throw a new exception each time so that the stack trace is right.
193+
return () -> {
194+
throw new IOException("Could not find user", e);
195+
};
196+
}
197+
}
198+
}
118199
}
119200

120201
private static final class JavaIoCreator extends TempFileCreator {

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

-8
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
}

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

-14
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package com.google.common.io;
1818

1919
import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
20-
import static com.google.common.base.StandardSystemProperty.OS_NAME;
2120
import static com.google.common.truth.Truth.assertThat;
2221
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
2322
import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
@@ -41,9 +40,6 @@ public class FileBackedOutputStreamTest extends IoTestCase {
4140

4241

4342
public void testThreshold() throws Exception {
44-
if (isWindows()) {
45-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
46-
}
4743
testThreshold(0, 100, true, false);
4844
testThreshold(10, 100, true, false);
4945
testThreshold(100, 100, true, false);
@@ -103,9 +99,6 @@ private void testThreshold(
10399

104100

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

133126
public void testWriteErrorAfterClose() throws Exception {
134-
if (isWindows()) {
135-
return; // TODO: b/285742623 - Fix FileBackedOutputStream under Windows.
136-
}
137127
byte[] data = newPreFilledByteArray(100);
138128
FileBackedOutputStream out = new FileBackedOutputStream(50);
139129
ByteSource source = out.asByteSource();
@@ -177,8 +167,4 @@ public void testReset() throws Exception {
177167
private static boolean isAndroid() {
178168
return System.getProperty("java.runtime.name", "").contains("Android");
179169
}
180-
181-
private static boolean isWindows() {
182-
return OS_NAME.value().startsWith("Windows");
183-
}
184170
}

0 commit comments

Comments
 (0)