Skip to content

Commit

Permalink
clone of #6730 but with Windows testing on JDK8
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@cpovirk
cpovirk committed Sep 26, 2023
1 parent 5a0af31 commit 61f135e
Show file tree
Hide file tree
Showing 5 changed files with 197 additions and 5 deletions.
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ jobs:
strategy:
matrix:
os: [ ubuntu-latest ]
java: [ 8, 11, 17 ]
root-pom: [ 'pom.xml', 'android/pom.xml' ]
java: [ 8 ]
root-pom: [ 'pom.xml' ]
include:
- os: windows-latest
java: 17
java: 8
root-pom: pom.xml
runs-on: ${{ matrix.os }}
env:
Expand Down
36 changes: 36 additions & 0 deletions android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.common.io;

import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;
import static com.google.common.base.StandardSystemProperty.OS_NAME;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
Expand Down Expand Up @@ -64,6 +65,41 @@ public void testCreateTempDir() throws IOException {
}
}

public void testBogusSystemPropertiesUsername() {
/*
* Only under Windows (or hypothetically when running with some other non-POSIX, ACL-based
* filesystem) do we look up the username. Thus, this test doesn't test anything interesting
* under most environments.
*
* Under Windows, we test that:
*
* - Under Java 9+, createTempDir() succeeds because it can look up the *real* username, rather
* than relying on the one from the system property.
*
* - Under Java 8, createTempDir() fails because it falls back to the bogus username from the
* system property.
*
* However: Note that we don't actually run our CI on Windows under Java 8, at least as of this
* writing.
*/
boolean isJava8OnWindows = JAVA_SPECIFICATION_VERSION.value().equals("1.8") && isWindows();

String save = System.getProperty("user.name");
System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?");
File temp = null;
try {
temp = Files.createTempDir();
assertThat(isJava8OnWindows).isFalse();
} catch (IllegalStateException expectedIfJavaWindows8) {
assertThat(isJava8OnWindows).isTrue();
} finally {
System.setProperty("user.name", save);
if (temp != null) {
assertThat(temp.delete()).isTrue();
}
}
}

private static boolean isAndroid() {
return System.getProperty("java.runtime.name", "").contains("Android");
}
Expand Down
62 changes: 61 additions & 1 deletion android/guava/src/com/google/common/io/TempFileCreator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,21 @@

import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
import static com.google.common.base.StandardSystemProperty.USER_NAME;
import static com.google.common.base.Throwables.throwIfUnchecked;
import static java.nio.file.attribute.AclEntryFlag.DIRECTORY_INHERIT;
import static java.nio.file.attribute.AclEntryFlag.FILE_INHERIT;
import static java.nio.file.attribute.AclEntryType.ALLOW;
import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute;
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
import com.google.common.collect.ImmutableList;
import com.google.j2objc.annotations.J2ObjCIncompatible;
import java.io.File;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.file.FileSystems;
import java.nio.file.Paths;
import java.nio.file.attribute.AclEntry;
Expand Down Expand Up @@ -150,7 +154,7 @@ private static PermissionSupplier userPermissions() {
UserPrincipal user =
FileSystems.getDefault()
.getUserPrincipalLookupService()
.lookupPrincipalByName(USER_NAME.value());
.lookupPrincipalByName(getUsername());
ImmutableList<AclEntry> acl =
ImmutableList.of(
AclEntry.newBuilder()
Expand Down Expand Up @@ -179,6 +183,62 @@ public ImmutableList<AclEntry> value() {
};
}
}

private static String getUsername() {
/*
* https://github.com/google/guava/issues/6634: ProcessHandle has more accurate information,
* but that class isn't available under all environments that we support. We use it if
* available and fall back if not.
*/
String fromSystemProperty = requireNonNull(USER_NAME.value());

try {
Class<?> processHandleClass = Class.forName("java.lang.ProcessHandle");
Class<?> processHandleInfoClass = Class.forName("java.lang.ProcessHandle$Info");
Class<?> optionalClass = Class.forName("java.util.Optional");
/*
* We don't *need* to use reflection to access Optional: It's available on all JDKs we
* support, and Android code won't get this far, anyway, because ProcessHandle is
* unavailable. But given how much other reflection we're using, we might as well use it
* here, too, so that we don't need to also suppress an AndroidApiChecker error.
*/

Method currentMethod = processHandleClass.getMethod("current");
Method infoMethod = processHandleClass.getMethod("info");
Method userMethod = processHandleInfoClass.getMethod("user");
Method orElseMethod = optionalClass.getMethod("orElse", Object.class);

Object current = currentMethod.invoke(null);
Object info = infoMethod.invoke(current);
Object user = userMethod.invoke(info);
return (String) requireNonNull(orElseMethod.invoke(user, fromSystemProperty));
} catch (ClassNotFoundException runningUnderAndroidOrJava8) {
/*
* I'm not sure that we could actually get here for *Android*: I would expect us to enter
* the POSIX code path instead. And if we tried this code path, we'd have trouble unless we
* were running under a new enough version of Android to support NIO.
*
* So this is probably just the "Windows Java 8" case. In that case, if we wanted *another*
* layer of fallback before consulting the system property, we could try
* com.sun.security.auth.module.NTSystem.
*
* But for now, we use the value from the system property as our best guess.
*/
return fromSystemProperty;
} catch (InvocationTargetException e) {
throwIfUnchecked(e.getCause()); // in case it's an Error or something
return fromSystemProperty; // should be impossible
} catch (NoSuchMethodException shouldBeImpossible) {
return fromSystemProperty;
} catch (IllegalAccessException shouldBeImpossible) {
/*
* We don't merge these into `catch (ReflectiveOperationException ...)` or an equivalent
* multicatch because ReflectiveOperationException isn't available under Android:
* b/124188803
*/
return fromSystemProperty;
}
}
}

private static final class JavaIoCreator extends TempFileCreator {
Expand Down
36 changes: 36 additions & 0 deletions guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.common.io;

import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;
import static com.google.common.base.StandardSystemProperty.OS_NAME;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
Expand Down Expand Up @@ -64,6 +65,41 @@ public void testCreateTempDir() throws IOException {
}
}

public void testBogusSystemPropertiesUsername() {
/*
* Only under Windows (or hypothetically when running with some other non-POSIX, ACL-based
* filesystem) do we look up the username. Thus, this test doesn't test anything interesting
* under most environments.
*
* Under Windows, we test that:
*
* - Under Java 9+, createTempDir() succeeds because it can look up the *real* username, rather
* than relying on the one from the system property.
*
* - Under Java 8, createTempDir() fails because it falls back to the bogus username from the
* system property.
*
* However: Note that we don't actually run our CI on Windows under Java 8, at least as of this
* writing.
*/
boolean isJava8OnWindows = JAVA_SPECIFICATION_VERSION.value().equals("1.8") && isWindows();

String save = System.getProperty("user.name");
System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?");
File temp = null;
try {
temp = Files.createTempDir();
assertThat(isJava8OnWindows).isFalse();
} catch (IllegalStateException expectedIfJavaWindows8) {
assertThat(isJava8OnWindows).isTrue();
} finally {
System.setProperty("user.name", save);
if (temp != null) {
assertThat(temp.delete()).isTrue();
}
}
}

private static boolean isAndroid() {
return System.getProperty("java.runtime.name", "").contains("Android");
}
Expand Down
62 changes: 61 additions & 1 deletion guava/src/com/google/common/io/TempFileCreator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,21 @@

import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
import static com.google.common.base.StandardSystemProperty.USER_NAME;
import static com.google.common.base.Throwables.throwIfUnchecked;
import static java.nio.file.attribute.AclEntryFlag.DIRECTORY_INHERIT;
import static java.nio.file.attribute.AclEntryFlag.FILE_INHERIT;
import static java.nio.file.attribute.AclEntryType.ALLOW;
import static java.nio.file.attribute.PosixFilePermissions.asFileAttribute;
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
import com.google.common.collect.ImmutableList;
import com.google.j2objc.annotations.J2ObjCIncompatible;
import java.io.File;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.file.FileSystems;
import java.nio.file.Paths;
import java.nio.file.attribute.AclEntry;
Expand Down Expand Up @@ -150,7 +154,7 @@ private static PermissionSupplier userPermissions() {
UserPrincipal user =
FileSystems.getDefault()
.getUserPrincipalLookupService()
.lookupPrincipalByName(USER_NAME.value());
.lookupPrincipalByName(getUsername());
ImmutableList<AclEntry> acl =
ImmutableList.of(
AclEntry.newBuilder()
Expand Down Expand Up @@ -179,6 +183,62 @@ public ImmutableList<AclEntry> value() {
};
}
}

private static String getUsername() {
/*
* https://github.com/google/guava/issues/6634: ProcessHandle has more accurate information,
* but that class isn't available under all environments that we support. We use it if
* available and fall back if not.
*/
String fromSystemProperty = requireNonNull(USER_NAME.value());

try {
Class<?> processHandleClass = Class.forName("java.lang.ProcessHandle");
Class<?> processHandleInfoClass = Class.forName("java.lang.ProcessHandle$Info");
Class<?> optionalClass = Class.forName("java.util.Optional");
/*
* We don't *need* to use reflection to access Optional: It's available on all JDKs we
* support, and Android code won't get this far, anyway, because ProcessHandle is
* unavailable. But given how much other reflection we're using, we might as well use it
* here, too, so that we don't need to also suppress an AndroidApiChecker error.
*/

Method currentMethod = processHandleClass.getMethod("current");
Method infoMethod = processHandleClass.getMethod("info");
Method userMethod = processHandleInfoClass.getMethod("user");
Method orElseMethod = optionalClass.getMethod("orElse", Object.class);

Object current = currentMethod.invoke(null);
Object info = infoMethod.invoke(current);
Object user = userMethod.invoke(info);
return (String) requireNonNull(orElseMethod.invoke(user, fromSystemProperty));
} catch (ClassNotFoundException runningUnderAndroidOrJava8) {
/*
* I'm not sure that we could actually get here for *Android*: I would expect us to enter
* the POSIX code path instead. And if we tried this code path, we'd have trouble unless we
* were running under a new enough version of Android to support NIO.
*
* So this is probably just the "Windows Java 8" case. In that case, if we wanted *another*
* layer of fallback before consulting the system property, we could try
* com.sun.security.auth.module.NTSystem.
*
* But for now, we use the value from the system property as our best guess.
*/
return fromSystemProperty;
} catch (InvocationTargetException e) {
throwIfUnchecked(e.getCause()); // in case it's an Error or something
return fromSystemProperty; // should be impossible
} catch (NoSuchMethodException shouldBeImpossible) {
return fromSystemProperty;
} catch (IllegalAccessException shouldBeImpossible) {
/*
* We don't merge these into `catch (ReflectiveOperationException ...)` or an equivalent
* multicatch because ReflectiveOperationException isn't available under Android:
* b/124188803
*/
return fromSystemProperty;
}
}
}

private static final class JavaIoCreator extends TempFileCreator {
Expand Down

0 comments on commit 61f135e

Please sign in to comment.