From 97a0e184896825bfce97ab81d8f4317f7704c3f7 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Tue, 26 Sep 2023 11:41:37 -0700 Subject: [PATCH 1/5] clone of https://github.com/google/guava/pull/6730 but without the prod change -- should fail Windows testing Fix `Files.createTempDir` and `FileBackedOutputStream` under Windows _services_, 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). PiperOrigin-RevId: 568604081 --- .github/workflows/ci.yml | 4 +-- .../common/io/FilesCreateTempDirTest.java | 36 +++++++++++++++++++ .../common/io/FilesCreateTempDirTest.java | 36 +++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 902dac31c755..766b0e5ca663 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,8 +20,8 @@ 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 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..c0c74d6640ec 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,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"); } diff --git a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index a31c43770e3b..c0c74d6640ec 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,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"); } From 4ce9ccd99acaef26fdd0ae710279cc10f619584e Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 26 Sep 2023 15:44:15 -0400 Subject: [PATCH 2/5] Try a more realistic username? --- .../test/com/google/common/io/FilesCreateTempDirTest.java | 2 +- .../test/com/google/common/io/FilesCreateTempDirTest.java | 2 +- 2 files changed, 2 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 c0c74d6640ec..e104204087d3 100644 --- a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -85,7 +85,7 @@ public void testBogusSystemPropertiesUsername() { 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//?"); + System.setProperty("user.name", "thisisabogususername"); File temp = null; try { temp = Files.createTempDir(); diff --git a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index c0c74d6640ec..e104204087d3 100644 --- a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -85,7 +85,7 @@ public void testBogusSystemPropertiesUsername() { 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//?"); + System.setProperty("user.name", "thisisabogususername"); File temp = null; try { temp = Files.createTempDir(); From 9c9a57b4f5543f579d05f4cb36dc6acf89a53882 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 26 Sep 2023 16:02:55 -0400 Subject: [PATCH 3/5] fixed test --- .../com/google/common/io/FilesCreateTempDirTest.java | 10 +++------- .../src/com/google/common/io/TempFileCreator.java | 11 +++++++++++ .../com/google/common/io/FilesCreateTempDirTest.java | 10 +++------- guava/src/com/google/common/io/TempFileCreator.java | 11 +++++++++++ 4 files changed, 28 insertions(+), 14 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 e104204087d3..d09b3f86555f 100644 --- a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -85,18 +85,14 @@ public void testBogusSystemPropertiesUsername() { boolean isJava8OnWindows = JAVA_SPECIFICATION_VERSION.value().equals("1.8") && isWindows(); String save = System.getProperty("user.name"); - System.setProperty("user.name", "thisisabogususername"); - File temp = null; + System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?"); try { - temp = Files.createTempDir(); + TempFileCreator.testMakingUserPermissionsFromScratch(); assertThat(isJava8OnWindows).isFalse(); - } catch (IllegalStateException expectedIfJavaWindows8) { + } catch (IOException expectedIfJavaWindows8) { assertThat(isJava8OnWindows).isTrue(); } finally { System.setProperty("user.name", save); - if (temp != null) { - assertThat(temp.delete()).isTrue(); - } } } diff --git a/android/guava/src/com/google/common/io/TempFileCreator.java b/android/guava/src/com/google/common/io/TempFileCreator.java index 96632359906e..66a01a53ca43 100644 --- a/android/guava/src/com/google/common/io/TempFileCreator.java +++ b/android/guava/src/com/google/common/io/TempFileCreator.java @@ -98,6 +98,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 diff --git a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index e104204087d3..d09b3f86555f 100644 --- a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -85,18 +85,14 @@ public void testBogusSystemPropertiesUsername() { boolean isJava8OnWindows = JAVA_SPECIFICATION_VERSION.value().equals("1.8") && isWindows(); String save = System.getProperty("user.name"); - System.setProperty("user.name", "thisisabogususername"); - File temp = null; + System.setProperty("user.name", "-this-is-definitely-not-the-username-we-are-running-as//?"); try { - temp = Files.createTempDir(); + TempFileCreator.testMakingUserPermissionsFromScratch(); assertThat(isJava8OnWindows).isFalse(); - } catch (IllegalStateException expectedIfJavaWindows8) { + } catch (IOException expectedIfJavaWindows8) { assertThat(isJava8OnWindows).isTrue(); } finally { System.setProperty("user.name", save); - if (temp != null) { - assertThat(temp.delete()).isTrue(); - } } } diff --git a/guava/src/com/google/common/io/TempFileCreator.java b/guava/src/com/google/common/io/TempFileCreator.java index 96632359906e..66a01a53ca43 100644 --- a/guava/src/com/google/common/io/TempFileCreator.java +++ b/guava/src/com/google/common/io/TempFileCreator.java @@ -98,6 +98,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 From 5c7247b96787a0c4de7fc40499f95724cdba6157 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 26 Sep 2023 16:06:28 -0400 Subject: [PATCH 4/5] add import --- android/guava/src/com/google/common/io/TempFileCreator.java | 1 + guava/src/com/google/common/io/TempFileCreator.java | 1 + 2 files changed, 2 insertions(+) diff --git a/android/guava/src/com/google/common/io/TempFileCreator.java b/android/guava/src/com/google/common/io/TempFileCreator.java index 66a01a53ca43..bdeab74a4def 100644 --- a/android/guava/src/com/google/common/io/TempFileCreator.java +++ b/android/guava/src/com/google/common/io/TempFileCreator.java @@ -23,6 +23,7 @@ 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; diff --git a/guava/src/com/google/common/io/TempFileCreator.java b/guava/src/com/google/common/io/TempFileCreator.java index 66a01a53ca43..bdeab74a4def 100644 --- a/guava/src/com/google/common/io/TempFileCreator.java +++ b/guava/src/com/google/common/io/TempFileCreator.java @@ -23,6 +23,7 @@ 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; From e2a4a52cbe61311b3b395657c53fff5b103ec306 Mon Sep 17 00:00:00 2001 From: Chris Povirk Date: Tue, 26 Sep 2023 16:26:49 -0400 Subject: [PATCH 5/5] "merge" --- .../common/io/FilesCreateTempDirTest.java | 28 ++++++++++++------- .../com/google/common/io/TempFileCreator.java | 5 +++- .../common/io/FilesCreateTempDirTest.java | 28 ++++++++++++------- 3 files changed, 40 insertions(+), 21 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 d09b3f86555f..d9312f1d7d7b 100644 --- a/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/android/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -66,31 +66,39 @@ 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) do we look up the username. Thus, this test doesn't test anything interesting - * under most environments. + * 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, we test that: + * 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. - * - * 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(); + 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(isJava8OnWindows).isFalse(); - } catch (IOException expectedIfJavaWindows8) { - assertThat(isJava8OnWindows).isTrue(); + assertThat(isJava8).isFalse(); + } catch (IOException expectedIfJava8) { + assertThat(isJava8).isTrue(); } finally { System.setProperty("user.name", save); } diff --git a/android/guava/src/com/google/common/io/TempFileCreator.java b/android/guava/src/com/google/common/io/TempFileCreator.java index bdeab74a4def..01dce4c24298 100644 --- a/android/guava/src/com/google/common/io/TempFileCreator.java +++ b/android/guava/src/com/google/common/io/TempFileCreator.java @@ -101,7 +101,10 @@ private static TempFileCreator pickSecureCreator() { /** * Creates the permissions normally used for Windows filesystems, looking up the user afresh, even - * if previous calls have initialized the PermissionSupplier fields. + * 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 diff --git a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java index d09b3f86555f..d9312f1d7d7b 100644 --- a/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java +++ b/guava-tests/test/com/google/common/io/FilesCreateTempDirTest.java @@ -66,31 +66,39 @@ 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) do we look up the username. Thus, this test doesn't test anything interesting - * under most environments. + * 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, we test that: + * 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. - * - * 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(); + 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(isJava8OnWindows).isFalse(); - } catch (IOException expectedIfJavaWindows8) { - assertThat(isJava8OnWindows).isTrue(); + assertThat(isJava8).isFalse(); + } catch (IOException expectedIfJava8) { + assertThat(isJava8).isTrue(); } finally { System.setProperty("user.name", save); }