Skip to content

Commit 0cd2b3a

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 0cd2b3a

File tree

4 files changed

+210
-2
lines changed

4 files changed

+210
-2
lines changed

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

+32
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,37 @@ 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+
try {
90+
TempFileCreator.testMakingUserPermissionsFromScratch();
91+
assertThat(isJava8OnWindows).isFalse();
92+
} catch (IOException expectedIfJavaWindows8) {
93+
assertThat(isJava8OnWindows).isTrue();
94+
} finally {
95+
System.setProperty("user.name", save);
96+
}
97+
}
98+
6799
private static boolean isAndroid() {
68100
return System.getProperty("java.runtime.name", "").contains("Android");
69101
}

android/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 {

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

+32
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,37 @@ 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+
try {
90+
TempFileCreator.testMakingUserPermissionsFromScratch();
91+
assertThat(isJava8OnWindows).isFalse();
92+
} catch (IOException expectedIfJavaWindows8) {
93+
assertThat(isJava8OnWindows).isTrue();
94+
} finally {
95+
System.setProperty("user.name", save);
96+
}
97+
}
98+
6799
private static boolean isAndroid() {
68100
return System.getProperty("java.runtime.name", "").contains("Android");
69101
}

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)