Skip to content

Commit 61f135e

Browse files
cpovirkcpovirk
committed
clone of #6730 but with Windows testing on JDK8
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 61f135e

File tree

5 files changed

+197
-5
lines changed

5 files changed

+197
-5
lines changed

Diff for: ‎.github/workflows/ci.yml

+3-3
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ jobs:
2020
strategy:
2121
matrix:
2222
os: [ ubuntu-latest ]
23-
java: [ 8, 11, 17 ]
24-
root-pom: [ 'pom.xml', 'android/pom.xml' ]
23+
java: [ 8 ]
24+
root-pom: [ 'pom.xml' ]
2525
include:
2626
- os: windows-latest
27-
java: 17
27+
java: 8
2828
root-pom: pom.xml
2929
runs-on: ${{ matrix.os }}
3030
env:

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

+36
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,41 @@ public void testCreateTempDir() throws IOException {
6465
}
6566
}
6667

68+
public void testBogusSystemPropertiesUsername() {
69+
/*
70+
* Only under Windows (or hypothetically when running with some other non-POSIX, ACL-based
71+
* filesystem) do we look up the username. Thus, this test doesn't test anything interesting
72+
* under most environments.
73+
*
74+
* Under Windows, we test that:
75+
*
76+
* - Under Java 9+, createTempDir() succeeds because it can look up the *real* username, rather
77+
* than relying on the one from the system property.
78+
*
79+
* - Under Java 8, createTempDir() fails because it falls back to the bogus username from the
80+
* system property.
81+
*
82+
* However: Note that we don't actually run our CI on Windows under Java 8, at least as of this
83+
* writing.
84+
*/
85+
boolean isJava8OnWindows = JAVA_SPECIFICATION_VERSION.value().equals("1.8") && isWindows();
86+
87+
String save = System.getProperty("user.name");
88+
System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?");
89+
File temp = null;
90+
try {
91+
temp = Files.createTempDir();
92+
assertThat(isJava8OnWindows).isFalse();
93+
} catch (IllegalStateException expectedIfJavaWindows8) {
94+
assertThat(isJava8OnWindows).isTrue();
95+
} finally {
96+
System.setProperty("user.name", save);
97+
if (temp != null) {
98+
assertThat(temp.delete()).isTrue();
99+
}
100+
}
101+
}
102+
67103
private static boolean isAndroid() {
68104
return System.getProperty("java.runtime.name", "").contains("Android");
69105
}

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

+61-1
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,21 @@
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;
2628
import com.google.common.collect.ImmutableList;
2729
import com.google.j2objc.annotations.J2ObjCIncompatible;
2830
import java.io.File;
2931
import java.io.IOException;
32+
import java.lang.reflect.InvocationTargetException;
33+
import java.lang.reflect.Method;
3034
import java.nio.file.FileSystems;
3135
import java.nio.file.Paths;
3236
import java.nio.file.attribute.AclEntry;
@@ -150,7 +154,7 @@ private static PermissionSupplier userPermissions() {
150154
UserPrincipal user =
151155
FileSystems.getDefault()
152156
.getUserPrincipalLookupService()
153-
.lookupPrincipalByName(USER_NAME.value());
157+
.lookupPrincipalByName(getUsername());
154158
ImmutableList<AclEntry> acl =
155159
ImmutableList.of(
156160
AclEntry.newBuilder()
@@ -179,6 +183,62 @@ public ImmutableList<AclEntry> value() {
179183
};
180184
}
181185
}
186+
187+
private static String getUsername() {
188+
/*
189+
* https://github.com/google/guava/issues/6634: ProcessHandle has more accurate information,
190+
* but that class isn't available under all environments that we support. We use it if
191+
* available and fall back if not.
192+
*/
193+
String fromSystemProperty = requireNonNull(USER_NAME.value());
194+
195+
try {
196+
Class<?> processHandleClass = Class.forName("java.lang.ProcessHandle");
197+
Class<?> processHandleInfoClass = Class.forName("java.lang.ProcessHandle$Info");
198+
Class<?> optionalClass = Class.forName("java.util.Optional");
199+
/*
200+
* We don't *need* to use reflection to access Optional: It's available on all JDKs we
201+
* support, and Android code won't get this far, anyway, because ProcessHandle is
202+
* unavailable. But given how much other reflection we're using, we might as well use it
203+
* here, too, so that we don't need to also suppress an AndroidApiChecker error.
204+
*/
205+
206+
Method currentMethod = processHandleClass.getMethod("current");
207+
Method infoMethod = processHandleClass.getMethod("info");
208+
Method userMethod = processHandleInfoClass.getMethod("user");
209+
Method orElseMethod = optionalClass.getMethod("orElse", Object.class);
210+
211+
Object current = currentMethod.invoke(null);
212+
Object info = infoMethod.invoke(current);
213+
Object user = userMethod.invoke(info);
214+
return (String) requireNonNull(orElseMethod.invoke(user, fromSystemProperty));
215+
} catch (ClassNotFoundException runningUnderAndroidOrJava8) {
216+
/*
217+
* I'm not sure that we could actually get here for *Android*: I would expect us to enter
218+
* the POSIX code path instead. And if we tried this code path, we'd have trouble unless we
219+
* were running under a new enough version of Android to support NIO.
220+
*
221+
* So this is probably just the "Windows Java 8" case. In that case, if we wanted *another*
222+
* layer of fallback before consulting the system property, we could try
223+
* com.sun.security.auth.module.NTSystem.
224+
*
225+
* But for now, we use the value from the system property as our best guess.
226+
*/
227+
return fromSystemProperty;
228+
} catch (InvocationTargetException e) {
229+
throwIfUnchecked(e.getCause()); // in case it's an Error or something
230+
return fromSystemProperty; // should be impossible
231+
} catch (NoSuchMethodException shouldBeImpossible) {
232+
return fromSystemProperty;
233+
} catch (IllegalAccessException shouldBeImpossible) {
234+
/*
235+
* We don't merge these into `catch (ReflectiveOperationException ...)` or an equivalent
236+
* multicatch because ReflectiveOperationException isn't available under Android:
237+
* b/124188803
238+
*/
239+
return fromSystemProperty;
240+
}
241+
}
182242
}
183243

184244
private static final class JavaIoCreator extends TempFileCreator {

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

+36
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,41 @@ public void testCreateTempDir() throws IOException {
6465
}
6566
}
6667

68+
public void testBogusSystemPropertiesUsername() {
69+
/*
70+
* Only under Windows (or hypothetically when running with some other non-POSIX, ACL-based
71+
* filesystem) do we look up the username. Thus, this test doesn't test anything interesting
72+
* under most environments.
73+
*
74+
* Under Windows, we test that:
75+
*
76+
* - Under Java 9+, createTempDir() succeeds because it can look up the *real* username, rather
77+
* than relying on the one from the system property.
78+
*
79+
* - Under Java 8, createTempDir() fails because it falls back to the bogus username from the
80+
* system property.
81+
*
82+
* However: Note that we don't actually run our CI on Windows under Java 8, at least as of this
83+
* writing.
84+
*/
85+
boolean isJava8OnWindows = JAVA_SPECIFICATION_VERSION.value().equals("1.8") && isWindows();
86+
87+
String save = System.getProperty("user.name");
88+
System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?");
89+
File temp = null;
90+
try {
91+
temp = Files.createTempDir();
92+
assertThat(isJava8OnWindows).isFalse();
93+
} catch (IllegalStateException expectedIfJavaWindows8) {
94+
assertThat(isJava8OnWindows).isTrue();
95+
} finally {
96+
System.setProperty("user.name", save);
97+
if (temp != null) {
98+
assertThat(temp.delete()).isTrue();
99+
}
100+
}
101+
}
102+
67103
private static boolean isAndroid() {
68104
return System.getProperty("java.runtime.name", "").contains("Android");
69105
}

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

+61-1
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,21 @@
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;
2628
import com.google.common.collect.ImmutableList;
2729
import com.google.j2objc.annotations.J2ObjCIncompatible;
2830
import java.io.File;
2931
import java.io.IOException;
32+
import java.lang.reflect.InvocationTargetException;
33+
import java.lang.reflect.Method;
3034
import java.nio.file.FileSystems;
3135
import java.nio.file.Paths;
3236
import java.nio.file.attribute.AclEntry;
@@ -150,7 +154,7 @@ private static PermissionSupplier userPermissions() {
150154
UserPrincipal user =
151155
FileSystems.getDefault()
152156
.getUserPrincipalLookupService()
153-
.lookupPrincipalByName(USER_NAME.value());
157+
.lookupPrincipalByName(getUsername());
154158
ImmutableList<AclEntry> acl =
155159
ImmutableList.of(
156160
AclEntry.newBuilder()
@@ -179,6 +183,62 @@ public ImmutableList<AclEntry> value() {
179183
};
180184
}
181185
}
186+
187+
private static String getUsername() {
188+
/*
189+
* https://github.com/google/guava/issues/6634: ProcessHandle has more accurate information,
190+
* but that class isn't available under all environments that we support. We use it if
191+
* available and fall back if not.
192+
*/
193+
String fromSystemProperty = requireNonNull(USER_NAME.value());
194+
195+
try {
196+
Class<?> processHandleClass = Class.forName("java.lang.ProcessHandle");
197+
Class<?> processHandleInfoClass = Class.forName("java.lang.ProcessHandle$Info");
198+
Class<?> optionalClass = Class.forName("java.util.Optional");
199+
/*
200+
* We don't *need* to use reflection to access Optional: It's available on all JDKs we
201+
* support, and Android code won't get this far, anyway, because ProcessHandle is
202+
* unavailable. But given how much other reflection we're using, we might as well use it
203+
* here, too, so that we don't need to also suppress an AndroidApiChecker error.
204+
*/
205+
206+
Method currentMethod = processHandleClass.getMethod("current");
207+
Method infoMethod = processHandleClass.getMethod("info");
208+
Method userMethod = processHandleInfoClass.getMethod("user");
209+
Method orElseMethod = optionalClass.getMethod("orElse", Object.class);
210+
211+
Object current = currentMethod.invoke(null);
212+
Object info = infoMethod.invoke(current);
213+
Object user = userMethod.invoke(info);
214+
return (String) requireNonNull(orElseMethod.invoke(user, fromSystemProperty));
215+
} catch (ClassNotFoundException runningUnderAndroidOrJava8) {
216+
/*
217+
* I'm not sure that we could actually get here for *Android*: I would expect us to enter
218+
* the POSIX code path instead. And if we tried this code path, we'd have trouble unless we
219+
* were running under a new enough version of Android to support NIO.
220+
*
221+
* So this is probably just the "Windows Java 8" case. In that case, if we wanted *another*
222+
* layer of fallback before consulting the system property, we could try
223+
* com.sun.security.auth.module.NTSystem.
224+
*
225+
* But for now, we use the value from the system property as our best guess.
226+
*/
227+
return fromSystemProperty;
228+
} catch (InvocationTargetException e) {
229+
throwIfUnchecked(e.getCause()); // in case it's an Error or something
230+
return fromSystemProperty; // should be impossible
231+
} catch (NoSuchMethodException shouldBeImpossible) {
232+
return fromSystemProperty;
233+
} catch (IllegalAccessException shouldBeImpossible) {
234+
/*
235+
* We don't merge these into `catch (ReflectiveOperationException ...)` or an equivalent
236+
* multicatch because ReflectiveOperationException isn't available under Android:
237+
* b/124188803
238+
*/
239+
return fromSystemProperty;
240+
}
241+
}
182242
}
183243

184244
private static final class JavaIoCreator extends TempFileCreator {

0 commit comments

Comments
 (0)