Skip to content

Commit

Permalink
Fix Files.createTempDir and FileBackedOutputStream under Windows …
Browse files Browse the repository at this point in the history 
…_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 authored and Google Java Core Libraries committed Sep 26, 2023
1 parent 5a0af31 commit fffa5a1
Show file tree
Hide file tree
Showing 4 changed files with 229 additions and 2 deletions.
40 changes: 40 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,45 @@ public void testCreateTempDir() throws IOException {
}
}

public void testBogusSystemPropertiesUsername() {
if (isAndroid()) {
/*
* The test calls directly into the "ACL-based filesystem" code, which isn't available under
* old versions of Android. Since Android doesn't use that code path, anyway, there's no need
* to test it.
*/
return;
}

/*
* Only under Windows (or hypothetically when running with some other non-POSIX, ACL-based
* filesystem) does our prod code look up the username. Thus, this test doesn't necessarily test
* anything interesting under most environments. Still, we can run it (except for Android, at
* least old versions), so we mostly do. This is useful because we don't actually run our CI on
* Windows under Java 8, at least as of this writing.
*
* Under Windows in particular, we want to 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.
*/
boolean isJava8 = JAVA_SPECIFICATION_VERSION.value().equals("1.8");

String save = System.getProperty("user.name");
System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?");
try {
TempFileCreator.testMakingUserPermissionsFromScratch();
assertThat(isJava8).isFalse();
} catch (IOException expectedIfJava8) {
assertThat(isJava8).isTrue();
} finally {
System.setProperty("user.name", save);
}
}

private static boolean isAndroid() {
return System.getProperty("java.runtime.name", "").contains("Android");
}
Expand Down
77 changes: 76 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,22 @@

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.annotations.VisibleForTesting;
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 @@ -98,6 +103,20 @@ private static TempFileCreator pickSecureCreator() {
return new JavaIoCreator();
}

/**
* Creates the permissions normally used for Windows filesystems, looking up the user afresh, even
* if previous calls have initialized the {@code PermissionSupplier} fields.
*
* <p>This lets us test the effects of different values of the {@code user.name} system property
* without needing a separate VM or classloader.
*/
@IgnoreJRERequirement // used only when Path is available (and only from tests)
@VisibleForTesting
static void testMakingUserPermissionsFromScratch() throws IOException {
// All we're testing is whether it throws.
FileAttribute<?> unused = JavaNioCreator.userPermissions().get();
}

@IgnoreJRERequirement // used only when Path is available
private static final class JavaNioCreator extends TempFileCreator {
@Override
Expand Down Expand Up @@ -150,7 +169,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 +198,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
40 changes: 40 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,45 @@ public void testCreateTempDir() throws IOException {
}
}

public void testBogusSystemPropertiesUsername() {
if (isAndroid()) {
/*
* The test calls directly into the "ACL-based filesystem" code, which isn't available under
* old versions of Android. Since Android doesn't use that code path, anyway, there's no need
* to test it.
*/
return;
}

/*
* Only under Windows (or hypothetically when running with some other non-POSIX, ACL-based
* filesystem) does our prod code look up the username. Thus, this test doesn't necessarily test
* anything interesting under most environments. Still, we can run it (except for Android, at
* least old versions), so we mostly do. This is useful because we don't actually run our CI on
* Windows under Java 8, at least as of this writing.
*
* Under Windows in particular, we want to 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.
*/
boolean isJava8 = JAVA_SPECIFICATION_VERSION.value().equals("1.8");

String save = System.getProperty("user.name");
System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?");
try {
TempFileCreator.testMakingUserPermissionsFromScratch();
assertThat(isJava8).isFalse();
} catch (IOException expectedIfJava8) {
assertThat(isJava8).isTrue();
} finally {
System.setProperty("user.name", save);
}
}

private static boolean isAndroid() {
return System.getProperty("java.runtime.name", "").contains("Android");
}
Expand Down
74 changes: 73 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,22 @@

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.annotations.VisibleForTesting;
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 @@ -98,6 +103,17 @@ private static TempFileCreator pickSecureCreator() {
return new JavaIoCreator();
}

/**
* Creates the permissions normally used for Windows filesystems, looking up the user afresh, even
* if previous calls have initialized the PermissionSupplier fields.
*/
@IgnoreJRERequirement // used only when Path is available (and only from tests)
@VisibleForTesting
static void testMakingUserPermissionsFromScratch() throws IOException {
// All we're testing is whether it throws.
FileAttribute<?> unused = JavaNioCreator.userPermissions().get();
}

@IgnoreJRERequirement // used only when Path is available
private static final class JavaNioCreator extends TempFileCreator {
@Override
Expand Down Expand Up @@ -150,7 +166,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 +195,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 fffa5a1

Please sign in to comment.