Skip to content

Commit fffa5a1

Browse files
cpovirkGoogle Java Core Libraries
authored and
Google Java Core Libraries
committed
Fix Files.createTempDir and FileBackedOutputStream under Windows _services_, a rare use case.
Fixes #6634 Relevant to #2686 in that it shows that we would ideally run our Windows testing under both Java 8 *and* a newer version.... RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under [Windows _services_, a rare use case](#6634). PiperOrigin-RevId: 568604081
1 parent 5a0af31 commit fffa5a1

File tree

4 files changed

+229
-2
lines changed

4 files changed

+229
-2
lines changed

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

+40
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
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.JAVA_SPECIFICATION_VERSION;
2021
import static com.google.common.base.StandardSystemProperty.OS_NAME;
2122
import static com.google.common.truth.Truth.assertThat;
2223
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
@@ -64,6 +65,45 @@ public void testCreateTempDir() throws IOException {
6465
}
6566
}
6667

68+
public void testBogusSystemPropertiesUsername() {
69+
if (isAndroid()) {
70+
/*
71+
* The test calls directly into the "ACL-based filesystem" code, which isn't available under
72+
* old versions of Android. Since Android doesn't use that code path, anyway, there's no need
73+
* to test it.
74+
*/
75+
return;
76+
}
77+
78+
/*
79+
* Only under Windows (or hypothetically when running with some other non-POSIX, ACL-based
80+
* filesystem) does our prod code look up the username. Thus, this test doesn't necessarily test
81+
* anything interesting under most environments. Still, we can run it (except for Android, at
82+
* least old versions), so we mostly do. This is useful because we don't actually run our CI on
83+
* Windows under Java 8, at least as of this writing.
84+
*
85+
* Under Windows in particular, we want to test that:
86+
*
87+
* - Under Java 9+, createTempDir() succeeds because it can look up the *real* username, rather
88+
* than relying on the one from the system property.
89+
*
90+
* - Under Java 8, createTempDir() fails because it falls back to the bogus username from the
91+
* system property.
92+
*/
93+
boolean isJava8 = JAVA_SPECIFICATION_VERSION.value().equals("1.8");
94+
95+
String save = System.getProperty("user.name");
96+
System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?");
97+
try {
98+
TempFileCreator.testMakingUserPermissionsFromScratch();
99+
assertThat(isJava8).isFalse();
100+
} catch (IOException expectedIfJava8) {
101+
assertThat(isJava8).isTrue();
102+
} finally {
103+
System.setProperty("user.name", save);
104+
}
105+
}
106+
67107
private static boolean isAndroid() {
68108
return System.getProperty("java.runtime.name", "").contains("Android");
69109
}

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

+76-1
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,22 @@
1616

1717
import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
1818
import static com.google.common.base.StandardSystemProperty.USER_NAME;
19+
import static com.google.common.base.Throwables.throwIfUnchecked;
1920
import static java.nio.file.attribute.AclEntryFlag.DIRECTORY_INHERIT;
2021
import static java.nio.file.attribute.AclEntryFlag.FILE_INHERIT;
2122
import static java.nio.file.attribute.AclEntryType.ALLOW;
2223
import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute;
24+
import static java.util.Objects.requireNonNull;
2325

2426
import com.google.common.annotations.GwtIncompatible;
2527
import com.google.common.annotations.J2ktIncompatible;
28+
import com.google.common.annotations.VisibleForTesting;
2629
import com.google.common.collect.ImmutableList;
2730
import com.google.j2objc.annotations.J2ObjCIncompatible;
2831
import java.io.File;
2932
import java.io.IOException;
33+
import java.lang.reflect.InvocationTargetException;
34+
import java.lang.reflect.Method;
3035
import java.nio.file.FileSystems;
3136
import java.nio.file.Paths;
3237
import java.nio.file.attribute.AclEntry;
@@ -98,6 +103,20 @@ private static TempFileCreator pickSecureCreator() {
98103
return new JavaIoCreator();
99104
}
100105

106+
/**
107+
* Creates the permissions normally used for Windows filesystems, looking up the user afresh, even
108+
* if previous calls have initialized the {@code PermissionSupplier} fields.
109+
*
110+
* <p>This lets us test the effects of different values of the {@code user.name} system property
111+
* without needing a separate VM or classloader.
112+
*/
113+
@IgnoreJRERequirement // used only when Path is available (and only from tests)
114+
@VisibleForTesting
115+
static void testMakingUserPermissionsFromScratch() throws IOException {
116+
// All we're testing is whether it throws.
117+
FileAttribute<?> unused = JavaNioCreator.userPermissions().get();
118+
}
119+
101120
@IgnoreJRERequirement // used only when Path is available
102121
private static final class JavaNioCreator extends TempFileCreator {
103122
@Override
@@ -150,7 +169,7 @@ private static PermissionSupplier userPermissions() {
150169
UserPrincipal user =
151170
FileSystems.getDefault()
152171
.getUserPrincipalLookupService()
153-
.lookupPrincipalByName(USER_NAME.value());
172+
.lookupPrincipalByName(getUsername());
154173
ImmutableList<AclEntry> acl =
155174
ImmutableList.of(
156175
AclEntry.newBuilder()
@@ -179,6 +198,62 @@ public ImmutableList<AclEntry> value() {
179198
};
180199
}
181200
}
201+
202+
private static String getUsername() {
203+
/*
204+
* https://github.com/google/guava/issues/6634: ProcessHandle has more accurate information,
205+
* but that class isn't available under all environments that we support. We use it if
206+
* available and fall back if not.
207+
*/
208+
String fromSystemProperty = requireNonNull(USER_NAME.value());
209+
210+
try {
211+
Class<?> processHandleClass = Class.forName("java.lang.ProcessHandle");
212+
Class<?> processHandleInfoClass = Class.forName("java.lang.ProcessHandle$Info");
213+
Class<?> optionalClass = Class.forName("java.util.Optional");
214+
/*
215+
* We don't *need* to use reflection to access Optional: It's available on all JDKs we
216+
* support, and Android code won't get this far, anyway, because ProcessHandle is
217+
* unavailable. But given how much other reflection we're using, we might as well use it
218+
* here, too, so that we don't need to also suppress an AndroidApiChecker error.
219+
*/
220+
221+
Method currentMethod = processHandleClass.getMethod("current");
222+
Method infoMethod = processHandleClass.getMethod("info");
223+
Method userMethod = processHandleInfoClass.getMethod("user");
224+
Method orElseMethod = optionalClass.getMethod("orElse", Object.class);
225+
226+
Object current = currentMethod.invoke(null);
227+
Object info = infoMethod.invoke(current);
228+
Object user = userMethod.invoke(info);
229+
return (String) requireNonNull(orElseMethod.invoke(user, fromSystemProperty));
230+
} catch (ClassNotFoundException runningUnderAndroidOrJava8) {
231+
/*
232+
* I'm not sure that we could actually get here for *Android*: I would expect us to enter
233+
* the POSIX code path instead. And if we tried this code path, we'd have trouble unless we
234+
* were running under a new enough version of Android to support NIO.
235+
*
236+
* So this is probably just the "Windows Java 8" case. In that case, if we wanted *another*
237+
* layer of fallback before consulting the system property, we could try
238+
* com.sun.security.auth.module.NTSystem.
239+
*
240+
* But for now, we use the value from the system property as our best guess.
241+
*/
242+
return fromSystemProperty;
243+
} catch (InvocationTargetException e) {
244+
throwIfUnchecked(e.getCause()); // in case it's an Error or something
245+
return fromSystemProperty; // should be impossible
246+
} catch (NoSuchMethodException shouldBeImpossible) {
247+
return fromSystemProperty;
248+
} catch (IllegalAccessException shouldBeImpossible) {
249+
/*
250+
* We don't merge these into `catch (ReflectiveOperationException ...)` or an equivalent
251+
* multicatch because ReflectiveOperationException isn't available under Android:
252+
* b/124188803
253+
*/
254+
return fromSystemProperty;
255+
}
256+
}
182257
}
183258

184259
private static final class JavaIoCreator extends TempFileCreator {

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

+40
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
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.JAVA_SPECIFICATION_VERSION;
2021
import static com.google.common.base.StandardSystemProperty.OS_NAME;
2122
import static com.google.common.truth.Truth.assertThat;
2223
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
@@ -64,6 +65,45 @@ public void testCreateTempDir() throws IOException {
6465
}
6566
}
6667

68+
public void testBogusSystemPropertiesUsername() {
69+
if (isAndroid()) {
70+
/*
71+
* The test calls directly into the "ACL-based filesystem" code, which isn't available under
72+
* old versions of Android. Since Android doesn't use that code path, anyway, there's no need
73+
* to test it.
74+
*/
75+
return;
76+
}
77+
78+
/*
79+
* Only under Windows (or hypothetically when running with some other non-POSIX, ACL-based
80+
* filesystem) does our prod code look up the username. Thus, this test doesn't necessarily test
81+
* anything interesting under most environments. Still, we can run it (except for Android, at
82+
* least old versions), so we mostly do. This is useful because we don't actually run our CI on
83+
* Windows under Java 8, at least as of this writing.
84+
*
85+
* Under Windows in particular, we want to test that:
86+
*
87+
* - Under Java 9+, createTempDir() succeeds because it can look up the *real* username, rather
88+
* than relying on the one from the system property.
89+
*
90+
* - Under Java 8, createTempDir() fails because it falls back to the bogus username from the
91+
* system property.
92+
*/
93+
boolean isJava8 = JAVA_SPECIFICATION_VERSION.value().equals("1.8");
94+
95+
String save = System.getProperty("user.name");
96+
System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?");
97+
try {
98+
TempFileCreator.testMakingUserPermissionsFromScratch();
99+
assertThat(isJava8).isFalse();
100+
} catch (IOException expectedIfJava8) {
101+
assertThat(isJava8).isTrue();
102+
} finally {
103+
System.setProperty("user.name", save);
104+
}
105+
}
106+
67107
private static boolean isAndroid() {
68108
return System.getProperty("java.runtime.name", "").contains("Android");
69109
}

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

+73-1
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,22 @@
1616

1717
import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
1818
import static com.google.common.base.StandardSystemProperty.USER_NAME;
19+
import static com.google.common.base.Throwables.throwIfUnchecked;
1920
import static java.nio.file.attribute.AclEntryFlag.DIRECTORY_INHERIT;
2021
import static java.nio.file.attribute.AclEntryFlag.FILE_INHERIT;
2122
import static java.nio.file.attribute.AclEntryType.ALLOW;
2223
import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute;
24+
import static java.util.Objects.requireNonNull;
2325

2426
import com.google.common.annotations.GwtIncompatible;
2527
import com.google.common.annotations.J2ktIncompatible;
28+
import com.google.common.annotations.VisibleForTesting;
2629
import com.google.common.collect.ImmutableList;
2730
import com.google.j2objc.annotations.J2ObjCIncompatible;
2831
import java.io.File;
2932
import java.io.IOException;
33+
import java.lang.reflect.InvocationTargetException;
34+
import java.lang.reflect.Method;
3035
import java.nio.file.FileSystems;
3136
import java.nio.file.Paths;
3237
import java.nio.file.attribute.AclEntry;
@@ -98,6 +103,17 @@ private static TempFileCreator pickSecureCreator() {
98103
return new JavaIoCreator();
99104
}
100105

106+
/**
107+
* Creates the permissions normally used for Windows filesystems, looking up the user afresh, even
108+
* if previous calls have initialized the PermissionSupplier fields.
109+
*/
110+
@IgnoreJRERequirement // used only when Path is available (and only from tests)
111+
@VisibleForTesting
112+
static void testMakingUserPermissionsFromScratch() throws IOException {
113+
// All we're testing is whether it throws.
114+
FileAttribute<?> unused = JavaNioCreator.userPermissions().get();
115+
}
116+
101117
@IgnoreJRERequirement // used only when Path is available
102118
private static final class JavaNioCreator extends TempFileCreator {
103119
@Override
@@ -150,7 +166,7 @@ private static PermissionSupplier userPermissions() {
150166
UserPrincipal user =
151167
FileSystems.getDefault()
152168
.getUserPrincipalLookupService()
153-
.lookupPrincipalByName(USER_NAME.value());
169+
.lookupPrincipalByName(getUsername());
154170
ImmutableList<AclEntry> acl =
155171
ImmutableList.of(
156172
AclEntry.newBuilder()
@@ -179,6 +195,62 @@ public ImmutableList<AclEntry> value() {
179195
};
180196
}
181197
}
198+
199+
private static String getUsername() {
200+
/*
201+
* https://github.com/google/guava/issues/6634: ProcessHandle has more accurate information,
202+
* but that class isn't available under all environments that we support. We use it if
203+
* available and fall back if not.
204+
*/
205+
String fromSystemProperty = requireNonNull(USER_NAME.value());
206+
207+
try {
208+
Class<?> processHandleClass = Class.forName("java.lang.ProcessHandle");
209+
Class<?> processHandleInfoClass = Class.forName("java.lang.ProcessHandle$Info");
210+
Class<?> optionalClass = Class.forName("java.util.Optional");
211+
/*
212+
* We don't *need* to use reflection to access Optional: It's available on all JDKs we
213+
* support, and Android code won't get this far, anyway, because ProcessHandle is
214+
* unavailable. But given how much other reflection we're using, we might as well use it
215+
* here, too, so that we don't need to also suppress an AndroidApiChecker error.
216+
*/
217+
218+
Method currentMethod = processHandleClass.getMethod("current");
219+
Method infoMethod = processHandleClass.getMethod("info");
220+
Method userMethod = processHandleInfoClass.getMethod("user");
221+
Method orElseMethod = optionalClass.getMethod("orElse", Object.class);
222+
223+
Object current = currentMethod.invoke(null);
224+
Object info = infoMethod.invoke(current);
225+
Object user = userMethod.invoke(info);
226+
return (String) requireNonNull(orElseMethod.invoke(user, fromSystemProperty));
227+
} catch (ClassNotFoundException runningUnderAndroidOrJava8) {
228+
/*
229+
* I'm not sure that we could actually get here for *Android*: I would expect us to enter
230+
* the POSIX code path instead. And if we tried this code path, we'd have trouble unless we
231+
* were running under a new enough version of Android to support NIO.
232+
*
233+
* So this is probably just the "Windows Java 8" case. In that case, if we wanted *another*
234+
* layer of fallback before consulting the system property, we could try
235+
* com.sun.security.auth.module.NTSystem.
236+
*
237+
* But for now, we use the value from the system property as our best guess.
238+
*/
239+
return fromSystemProperty;
240+
} catch (InvocationTargetException e) {
241+
throwIfUnchecked(e.getCause()); // in case it's an Error or something
242+
return fromSystemProperty; // should be impossible
243+
} catch (NoSuchMethodException shouldBeImpossible) {
244+
return fromSystemProperty;
245+
} catch (IllegalAccessException shouldBeImpossible) {
246+
/*
247+
* We don't merge these into `catch (ReflectiveOperationException ...)` or an equivalent
248+
* multicatch because ReflectiveOperationException isn't available under Android:
249+
* b/124188803
250+
*/
251+
return fromSystemProperty;
252+
}
253+
}
182254
}
183255

184256
private static final class JavaIoCreator extends TempFileCreator {

0 commit comments

Comments
 (0)