From f87f68cd3e032a082a7702f3143862a9b27422fe Mon Sep 17 00:00:00 2001 From: cpovirk Date: Fri, 6 Oct 2023 10:09:07 -0700 Subject: [PATCH] Fix `Files.createTempDir` and `FileBackedOutputStream` under Windows _services_ (at least under JDK 9+), a rare use case. Fixes https://github.com/google/guava/issues/6634 Relevant to https://github.com/google/guava/issues/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](https://github.com/google/guava/issues/6634). (The fix actually covers only Java 9+ because Java 8 would require an additional approach. Let us know if you need support under Java 8.) PiperOrigin-RevId: 571368120 --- .../common/io/FilesCreateTempDirTest.java | 43 +++++++++++ .../com/google/common/io/TempFileCreator.java | 77 ++++++++++++++++++- .../common/io/FilesCreateTempDirTest.java | 43 +++++++++++ .../com/google/common/io/TempFileCreator.java | 77 ++++++++++++++++++- 4 files changed, 238 insertions(+), 2 deletions(-) diff --git a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index a31c43770e3b..381eef258425 100644 --- a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -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; @@ -64,6 +65,44 @@ 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. + */ + + 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"); } @@ -71,4 +110,8 @@ private static boolean isAndroid() { private static boolean isWindows() { return OS_NAME.value().startsWith("Windows"); } + + private static boolean isJava8() { + return JAVA_SPECIFICATION_VERSION.value().equals("1.8"); + } } diff --git a/android/guava/src/com/google/common/io/TempFileCreator.java b/android/guava/src/com/google/common/io/TempFileCreator.java index 96632359906e..769761280b27 100644 --- a/android/guava/src/com/google/common/io/TempFileCreator.java +++ b/android/guava/src/com/google/common/io/TempFileCreator.java @@ -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; @@ -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. + * + *

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 @@ -150,7 +169,7 @@ private static PermissionSupplier userPermissions() { UserPrincipal user = FileSystems.getDefault() .getUserPrincipalLookupService() - .lookupPrincipalByName(USER_NAME.value()); + .lookupPrincipalByName(getUsername()); ImmutableList acl = ImmutableList.of( AclEntry.newBuilder() @@ -179,6 +198,62 @@ public ImmutableList 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 { diff --git a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index a31c43770e3b..381eef258425 100644 --- a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -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; @@ -64,6 +65,44 @@ 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. + */ + + 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"); } @@ -71,4 +110,8 @@ private static boolean isAndroid() { private static boolean isWindows() { return OS_NAME.value().startsWith("Windows"); } + + private static boolean isJava8() { + return JAVA_SPECIFICATION_VERSION.value().equals("1.8"); + } } diff --git a/guava/src/com/google/common/io/TempFileCreator.java b/guava/src/com/google/common/io/TempFileCreator.java index 96632359906e..769761280b27 100644 --- a/guava/src/com/google/common/io/TempFileCreator.java +++ b/guava/src/com/google/common/io/TempFileCreator.java @@ -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; @@ -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. + * + *

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 @@ -150,7 +169,7 @@ private static PermissionSupplier userPermissions() { UserPrincipal user = FileSystems.getDefault() .getUserPrincipalLookupService() - .lookupPrincipalByName(USER_NAME.value()); + .lookupPrincipalByName(getUsername()); ImmutableList acl = ImmutableList.of( AclEntry.newBuilder() @@ -179,6 +198,62 @@ public ImmutableList 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 {