From 550bd4afe7599da340ff04f56f3fb226cf203f92 Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Tue, 18 Feb 2025 13:42:27 +0100 Subject: [PATCH 01/12] Work in progress. --- .../NoOverwriteJarClassFilesByDefault.java | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java diff --git a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java b/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java new file mode 100644 index 0000000000000..7ec95c7994e83 --- /dev/null +++ b/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java @@ -0,0 +1,80 @@ +/* + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +package T8338675; + +import toolbox.JarTask; +import toolbox.JavacTask; +import toolbox.ToolBox; + +import java.io.IOException; + +/* + * @test + * @bug 8338675 + * @summary javac shouldn't silently change .jar files on the classpath + * @library /tools/lib + * @modules jdk.compiler/com.sun.tools.javac.api + * jdk.compiler/com.sun.tools.javac.main + * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask + * @run main NoOverwriteJarClassFilesByDefault + */ +public class NoOverwriteJarClassFilesByDefault { + + private static final String OLD_JAR_SOURCE = """ + class InJarFile { + static final String X = "ABCD"; + } + """; + + private static final String NEW_JAR_SOURCE = """ + class InJarFile { + static final String X = "XYZ"; + } + """; + + private static final String TARGET_SOURCE = """ + class TargetClass { + static final String Y = InJarFile.X; + } + """; + + public static void main(String[] args) throws IOException { + ToolBox tb = new ToolBox(); + + new JavacTask(tb) + .sources(OLD_JAR_SOURCE) + .run(); + + // Jar file has the "new" source in (which doesn't match the compiled class file). + tb.writeFile("InJarFile.java", NEW_JAR_SOURCE); + new JarTask(tb, "lib.jar") + .files("InJarFile.java", "InJarFile.class") + .run(); + + new JavacTask(tb) + .sources(TARGET_SOURCE) + .classpath("lib.jar") + .run(); + } +} From 06b6ee57703b74e896f627397b0c4a8d75a7d35b Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Tue, 18 Feb 2025 17:47:34 +0100 Subject: [PATCH 02/12] 8338675: javac shouldn't silently change .jar files on the classpath --- .../NoOverwriteJarClassFilesByDefault.java | 110 ++++++++++++++---- 1 file changed, 89 insertions(+), 21 deletions(-) diff --git a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java b/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java index 7ec95c7994e83..d25a2d9bbd7c1 100644 --- a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java +++ b/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java @@ -21,14 +21,6 @@ * questions. */ -package T8338675; - -import toolbox.JarTask; -import toolbox.JavacTask; -import toolbox.ToolBox; - -import java.io.IOException; - /* * @test * @bug 8338675 @@ -39,42 +31,118 @@ * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask * @run main NoOverwriteJarClassFilesByDefault */ + +import toolbox.JavacTask; +import toolbox.ToolBox; + +import java.io.IOException; +import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.time.Instant; +import java.time.ZoneId; +import java.util.jar.JarOutputStream; +import java.util.zip.ZipEntry; +import java.util.zip.ZipFile; + public class NoOverwriteJarClassFilesByDefault { - private static final String OLD_JAR_SOURCE = """ - class InJarFile { - static final String X = "ABCD"; + private static final String OLD_LIB_SOURCE = """ + class LibClass { + static final String OLD_FIELD = "This will not compile with Target"; } """; - private static final String NEW_JAR_SOURCE = """ - class InJarFile { - static final String X = "XYZ"; + private static final String NEW_LIB_SOURCE = """ + class LibClass { + static final String NEW_FIELD = "Only this will compile with Target"; } """; + // Target source references the field only available in the new source. private static final String TARGET_SOURCE = """ class TargetClass { - static final String Y = InJarFile.X; + static final String VALUE = LibClass.NEW_FIELD; } """; + private static final String LIB_SOURCE_NAME = "LibClass.java"; + private static final String LIB_CLASS_NAME = "LibClass.class"; + public static void main(String[] args) throws IOException { ToolBox tb = new ToolBox(); + // Compile the old (broken) source and then store the class file in the JAR. new JavacTask(tb) - .sources(OLD_JAR_SOURCE) + .sources(OLD_LIB_SOURCE) .run(); - // Jar file has the "new" source in (which doesn't match the compiled class file). - tb.writeFile("InJarFile.java", NEW_JAR_SOURCE); - new JarTask(tb, "lib.jar") - .files("InJarFile.java", "InJarFile.class") - .run(); + // The new (fixed) source is never written to disk, so if compilation works + // it proves it's getting it from the source file in the JAR. + Instant olderTime = Instant.now(); + Instant newerTime = olderTime.plusSeconds(1); + try (OutputStream jos = Files.newOutputStream(Path.of("lib.jar"))) { + JarOutputStream jar = new JarOutputStream(jos); + // Important: The JAR file entry order *matters* if the timestamps are + // the same (the latter entry is used for compilation, regardless of + // whether it's a source file or a class file). So in this test, we + // put the one we want to use *first* with a newer timestamp, to show + // that it's definitely the timestamp being used to select the source. + // TODO: Should this be UTF-8, or platform default for byte encoding? + writeEntry(jar, LIB_SOURCE_NAME, NEW_LIB_SOURCE.getBytes(), newerTime); + // Source is newer than the (broken) compiled class, so should compile + // from the source file in the JAR. If timestamps were not set, or set + // equal, the test would use this class file, and fail. + writeEntry(jar, LIB_CLASS_NAME, Files.readAllBytes(Path.of(LIB_CLASS_NAME)), olderTime); + jar.close(); + } + long originalLibCrc = getLibCrc(); + // This compilation only succeeds if 'NEW_FIELD' exists, which is only in + // the source file written to the JAR, and nowhere on disk. + // Before the fix, adding '.options("-implicit:none")' makes the test pass. new JavacTask(tb) .sources(TARGET_SOURCE) .classpath("lib.jar") .run(); + + // Since compilation succeeded, we know it used NEW_LIB_SOURCE, and if it + // wrote the class file back to the JAR (bad) then that should now have + // different contents. Note that the modification time of the class file + // is NOT modified, even if the JAR is updated, so we cannot test that. + long actualLibCrc = getLibCrc(); + if (actualLibCrc != originalLibCrc) { + throw new AssertionError("Class library contents were modified in the JAR file."); + } + } + + // Note: JarOutputStream only writes modification time, not creation time, but + // that's what Javac uses to determine "newness" so it's fine. + private static void writeEntry(JarOutputStream jar, String name, byte[] bytes, Instant timestamp) throws IOException { + ZipEntry e = new ZipEntry(name); + e.setLastModifiedTime(FileTime.from(timestamp)); + jar.putNextEntry(e); + jar.write(bytes); + jar.closeEntry(); + } + + private static long getLibCrc() throws IOException { + try (ZipFile zipFile = new ZipFile("lib.jar")) { + // zipFile.stream().map(NoOverwriteJarClassFilesByDefault::format).forEach(System.err::println); + return zipFile.getEntry(LIB_CLASS_NAME).getCrc(); + } + } + + // ---- Debug helper methods ---- + private static String format(ZipEntry e) { + return String.format("name: %s, size: %s, modified: %s\n", + e.getName(), + e.getSize(), + toLocalTime(e.getLastModifiedTime())); + } + + private static String toLocalTime(FileTime t) { + return t != null ? t.toInstant().atZone(ZoneId.systemDefault()).toString() : ""; } } From 0131a99d0790e20d4b82700f2d853c25ca84021f Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Wed, 19 Feb 2025 16:41:22 +0100 Subject: [PATCH 03/12] 8338675: javac shouldn't silently change .jar files on the classpath --- .../tools/javac/file/JavacFileManager.java | 16 +++- .../sun/tools/javac/file/PathFileObject.java | 7 ++ .../NoOverwriteJarClassFilesByDefault.java | 78 ++++++++++++++----- 3 files changed, 76 insertions(+), 25 deletions(-) diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java index 9b61398ba9250..49abbfec6e189 100644 --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java @@ -913,13 +913,21 @@ private JavaFileObject getFileForOutput(Location location, if (getClassOutDir() != null) { dir = getClassOutDir(); } else { + // Sibling is the associated source of the class file (e.g. x/y/Foo.java). + // The base name for class output is the class file name (e.g. "Foo.class"). String baseName = fileName.basename(); - if (sibling != null && sibling instanceof PathFileObject pathFileObject) { + // Use the sibling to determine the output location where possible, unless + // it is in a JAR/ZIP file (we don't attempt to write class files back into + // archives). See JDK-8338675. + if (sibling instanceof PathFileObject pathFileObject && !pathFileObject.isJarFile()) { return pathFileObject.getSibling(baseName); } else { - Path p = getPath(baseName); - Path real = fsInfo.getCanonicalFile(p); - return PathFileObject.forSimplePath(this, real, p); + // Without the sibling present, we just create an output path in the + // current working directory (this isn't great, but it is what older + // versions of the JDK did). + Path userPath = getPath(baseName); + Path realPath = fsInfo.getCanonicalFile(userPath); + return PathFileObject.forSimplePath(this, realPath, userPath); } } } else if (location == SOURCE_OUTPUT) { diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/PathFileObject.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/PathFileObject.java index 9ca2aee71adab..a1c40163e9fe1 100644 --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/PathFileObject.java +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/PathFileObject.java @@ -354,6 +354,13 @@ protected PathFileObject(BaseFileManager fileManager, Path path) { */ abstract PathFileObject getSibling(String basename); + /** + * Returns whether this file object represents a file in a JAR archive. + */ + boolean isJarFile() { + return this instanceof JarFileObject; + } + /** * Return the Path for this object. * @return the Path for this object. diff --git a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java b/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java index d25a2d9bbd7c1..ce769c5c2423b 100644 --- a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java +++ b/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java @@ -39,44 +39,68 @@ import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.attribute.FileTime; import java.time.Instant; import java.time.ZoneId; import java.util.jar.JarOutputStream; +import java.util.zip.CRC32; +import java.util.zip.CRC32C; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; +/** + * This test makes two specific assertions about javac behaviour when source + * files are found in the classpath. + * + *
    + *
  1. Source files found in classpath JAR files are not overwritten. + *
  2. Class files generated during compilation which are associated with any + * sources found in JAR files are written (flat) to the current directory. + *
+ * + *

Note that this behaviour is not obviously well-defined, and should not + * be relied upon, but it matches previous JDK behaviour and so is tested here. + * + *

Specifically, the behaviour in (2) means library classes with the same base + * class name may be overwritten, and the resulting set of class files in the + * current directory may not be usable. + */ public class NoOverwriteJarClassFilesByDefault { private static final String OLD_LIB_SOURCE = """ - class LibClass { - static final String OLD_FIELD = "This will not compile with Target"; + package lib; + public class LibClass { + public static final String OLD_FIELD = "This will not compile with Target"; } """; private static final String NEW_LIB_SOURCE = """ - class LibClass { - static final String NEW_FIELD = "Only this will compile with Target"; + package lib; + public class LibClass { + public static final String NEW_FIELD = "Only this will compile with Target"; } """; // Target source references the field only available in the new source. private static final String TARGET_SOURCE = """ class TargetClass { - static final String VALUE = LibClass.NEW_FIELD; + static final String VALUE = lib.LibClass.NEW_FIELD; } """; - private static final String LIB_SOURCE_NAME = "LibClass.java"; - private static final String LIB_CLASS_NAME = "LibClass.class"; + private static final String LIB_SOURCE_NAME = "lib/LibClass.java"; + private static final String LIB_CLASS_NAME = "lib/LibClass.class"; public static void main(String[] args) throws IOException { ToolBox tb = new ToolBox(); + tb.createDirectories("lib"); + tb.writeFile(LIB_SOURCE_NAME, OLD_LIB_SOURCE); // Compile the old (broken) source and then store the class file in the JAR. - new JavacTask(tb) - .sources(OLD_LIB_SOURCE) - .run(); + // The class file generated he is in the lib/ directory, which we delete + // after making the JAR (just to be sure). + new JavacTask(tb).files(LIB_SOURCE_NAME).run(); // The new (fixed) source is never written to disk, so if compilation works // it proves it's getting it from the source file in the JAR. @@ -89,24 +113,34 @@ public static void main(String[] args) throws IOException { // whether it's a source file or a class file). So in this test, we // put the one we want to use *first* with a newer timestamp, to show // that it's definitely the timestamp being used to select the source. - // TODO: Should this be UTF-8, or platform default for byte encoding? writeEntry(jar, LIB_SOURCE_NAME, NEW_LIB_SOURCE.getBytes(), newerTime); // Source is newer than the (broken) compiled class, so should compile // from the source file in the JAR. If timestamps were not set, or set - // equal, the test would use this class file, and fail. + // equal, the test would use this (broken) class file, and fail. writeEntry(jar, LIB_CLASS_NAME, Files.readAllBytes(Path.of(LIB_CLASS_NAME)), olderTime); jar.close(); } + // Check there's no output file present and delete the original library files. + Path outputClassFile = Path.of("LibClass.class"); + if (Files.exists(outputClassFile)) { + throw new IllegalStateException("Output class file should not exist (yet)."); + } + Path libDir = Path.of("lib"); + tb.cleanDirectory(libDir); + tb.deleteFiles(libDir); + + // Before running the test itself, get the CRC of the class file in the JAR. long originalLibCrc = getLibCrc(); + // Code under test: + // Compile the target class with new library source only available in the JAR. + // // This compilation only succeeds if 'NEW_FIELD' exists, which is only in // the source file written to the JAR, and nowhere on disk. - // Before the fix, adding '.options("-implicit:none")' makes the test pass. - new JavacTask(tb) - .sources(TARGET_SOURCE) - .classpath("lib.jar") - .run(); + new JavacTask(tb).sources(TARGET_SOURCE).classpath("lib.jar").run(); + // Assertion 1: The class file in the JAR is unchanged. + // // Since compilation succeeded, we know it used NEW_LIB_SOURCE, and if it // wrote the class file back to the JAR (bad) then that should now have // different contents. Note that the modification time of the class file @@ -115,6 +149,11 @@ public static void main(String[] args) throws IOException { if (actualLibCrc != originalLibCrc) { throw new AssertionError("Class library contents were modified in the JAR file."); } + + // Assertion 2: An output class file was written to the current directory. + if (!Files.exists(outputClassFile)) { + throw new AssertionError("Output class file was not written to the current directory."); + } } // Note: JarOutputStream only writes modification time, not creation time, but @@ -136,10 +175,7 @@ private static long getLibCrc() throws IOException { // ---- Debug helper methods ---- private static String format(ZipEntry e) { - return String.format("name: %s, size: %s, modified: %s\n", - e.getName(), - e.getSize(), - toLocalTime(e.getLastModifiedTime())); + return String.format("name: %s, size: %s, modified: %s\n", e.getName(), e.getSize(), toLocalTime(e.getLastModifiedTime())); } private static String toLocalTime(FileTime t) { From 5d47fdf113556580c37e1fd6808d8e01f07c69d5 Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Wed, 19 Feb 2025 17:12:14 +0100 Subject: [PATCH 04/12] 8338675: javac shouldn't silently change .jar files on the classpath --- .../javac/T8338675/NoOverwriteJarClassFilesByDefault.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java b/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java index ce769c5c2423b..b951a150922c4 100644 --- a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java +++ b/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java @@ -39,13 +39,10 @@ import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.attribute.FileTime; import java.time.Instant; import java.time.ZoneId; import java.util.jar.JarOutputStream; -import java.util.zip.CRC32; -import java.util.zip.CRC32C; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; From 39d24c9a243d2239b6e559d3e9c8d2b877cfa4cd Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Fri, 21 Feb 2025 12:20:41 +0100 Subject: [PATCH 05/12] Changes based on PR feedback. --- .../NoOverwriteJarClassFilesByDefault.java | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) rename test/langtools/tools/javac/{T8338675 => }/NoOverwriteJarClassFilesByDefault.java (89%) diff --git a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java b/test/langtools/tools/javac/NoOverwriteJarClassFilesByDefault.java similarity index 89% rename from test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java rename to test/langtools/tools/javac/NoOverwriteJarClassFilesByDefault.java index b951a150922c4..d64818f0bbd82 100644 --- a/test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java +++ b/test/langtools/tools/javac/NoOverwriteJarClassFilesByDefault.java @@ -42,6 +42,7 @@ import java.nio.file.attribute.FileTime; import java.time.Instant; import java.time.ZoneId; +import java.util.Arrays; import java.util.jar.JarOutputStream; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; @@ -65,14 +66,16 @@ */ public class NoOverwriteJarClassFilesByDefault { - private static final String OLD_LIB_SOURCE = """ + private static final String OLD_LIB_SOURCE = + """ package lib; public class LibClass { public static final String OLD_FIELD = "This will not compile with Target"; } """; - private static final String NEW_LIB_SOURCE = """ + private static final String NEW_LIB_SOURCE = + """ package lib; public class LibClass { public static final String NEW_FIELD = "Only this will compile with Target"; @@ -80,7 +83,8 @@ public class LibClass { """; // Target source references the field only available in the new source. - private static final String TARGET_SOURCE = """ + private static final String TARGET_SOURCE = + """ class TargetClass { static final String VALUE = lib.LibClass.NEW_FIELD; } @@ -88,6 +92,7 @@ class TargetClass { private static final String LIB_SOURCE_NAME = "lib/LibClass.java"; private static final String LIB_CLASS_NAME = "lib/LibClass.class"; + private static final Path LIB_JAR = Path.of("lib.jar"); public static void main(String[] args) throws IOException { ToolBox tb = new ToolBox(); @@ -97,7 +102,10 @@ public static void main(String[] args) throws IOException { // Compile the old (broken) source and then store the class file in the JAR. // The class file generated he is in the lib/ directory, which we delete // after making the JAR (just to be sure). - new JavacTask(tb).files(LIB_SOURCE_NAME).run(); + new JavacTask(tb) + .files(LIB_SOURCE_NAME) + .run() + .writeAll(); // The new (fixed) source is never written to disk, so if compilation works // it proves it's getting it from the source file in the JAR. @@ -128,13 +136,19 @@ public static void main(String[] args) throws IOException { // Before running the test itself, get the CRC of the class file in the JAR. long originalLibCrc = getLibCrc(); + // And read the JAR file completely for comparison later. + byte[] originalJarContents = Files.readAllBytes(LIB_JAR); // Code under test: // Compile the target class with new library source only available in the JAR. // // This compilation only succeeds if 'NEW_FIELD' exists, which is only in // the source file written to the JAR, and nowhere on disk. - new JavacTask(tb).sources(TARGET_SOURCE).classpath("lib.jar").run(); + new JavacTask(tb) + .sources(TARGET_SOURCE) + .classpath(LIB_JAR) + .run() + .writeAll(); // Assertion 1: The class file in the JAR is unchanged. // @@ -142,10 +156,12 @@ public static void main(String[] args) throws IOException { // wrote the class file back to the JAR (bad) then that should now have // different contents. Note that the modification time of the class file // is NOT modified, even if the JAR is updated, so we cannot test that. - long actualLibCrc = getLibCrc(); - if (actualLibCrc != originalLibCrc) { + if (getLibCrc() != originalLibCrc) { throw new AssertionError("Class library contents were modified in the JAR file."); } + if (!Arrays.equals(Files.readAllBytes(LIB_JAR), originalJarContents)) { + throw new AssertionError("Jar file was modified."); + } // Assertion 2: An output class file was written to the current directory. if (!Files.exists(outputClassFile)) { From 8b9134844bf45230d61d36e7023b1c941fdc9b20 Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Fri, 21 Feb 2025 13:11:49 +0100 Subject: [PATCH 06/12] Changes based on PR feedback. --- ...java => NoOverwriteJarClassFilesTest.java} | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) rename test/langtools/tools/javac/{NoOverwriteJarClassFilesByDefault.java => NoOverwriteJarClassFilesTest.java} (90%) diff --git a/test/langtools/tools/javac/NoOverwriteJarClassFilesByDefault.java b/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java similarity index 90% rename from test/langtools/tools/javac/NoOverwriteJarClassFilesByDefault.java rename to test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java index d64818f0bbd82..b11b8d7faaa01 100644 --- a/test/langtools/tools/javac/NoOverwriteJarClassFilesByDefault.java +++ b/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java @@ -29,7 +29,7 @@ * @modules jdk.compiler/com.sun.tools.javac.api * jdk.compiler/com.sun.tools.javac.main * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask - * @run main NoOverwriteJarClassFilesByDefault + * @run junit NoOverwriteJarClassFilesTest */ import toolbox.JavacTask; @@ -42,11 +42,16 @@ import java.nio.file.attribute.FileTime; import java.time.Instant; import java.time.ZoneId; -import java.util.Arrays; import java.util.jar.JarOutputStream; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + /** * This test makes two specific assertions about javac behaviour when source * files are found in the classpath. @@ -64,7 +69,7 @@ * class name may be overwritten, and the resulting set of class files in the * current directory may not be usable. */ -public class NoOverwriteJarClassFilesByDefault { +public class NoOverwriteJarClassFilesTest { private static final String OLD_LIB_SOURCE = """ @@ -94,7 +99,8 @@ class TargetClass { private static final String LIB_CLASS_NAME = "lib/LibClass.class"; private static final Path LIB_JAR = Path.of("lib.jar"); - public static void main(String[] args) throws IOException { + @Test + public void jarFileNotModified() throws IOException { ToolBox tb = new ToolBox(); tb.createDirectories("lib"); tb.writeFile(LIB_SOURCE_NAME, OLD_LIB_SOURCE); @@ -156,17 +162,11 @@ public static void main(String[] args) throws IOException { // wrote the class file back to the JAR (bad) then that should now have // different contents. Note that the modification time of the class file // is NOT modified, even if the JAR is updated, so we cannot test that. - if (getLibCrc() != originalLibCrc) { - throw new AssertionError("Class library contents were modified in the JAR file."); - } - if (!Arrays.equals(Files.readAllBytes(LIB_JAR), originalJarContents)) { - throw new AssertionError("Jar file was modified."); - } + assertEquals(originalLibCrc, getLibCrc(), "Class library contents were modified in the JAR file."); + assertArrayEquals(originalJarContents, Files.readAllBytes(LIB_JAR), "Jar file was modified."); // Assertion 2: An output class file was written to the current directory. - if (!Files.exists(outputClassFile)) { - throw new AssertionError("Output class file was not written to the current directory."); - } + assertTrue(Files.exists(outputClassFile), "Output class file was not written to current directory."); } // Note: JarOutputStream only writes modification time, not creation time, but @@ -188,7 +188,10 @@ private static long getLibCrc() throws IOException { // ---- Debug helper methods ---- private static String format(ZipEntry e) { - return String.format("name: %s, size: %s, modified: %s\n", e.getName(), e.getSize(), toLocalTime(e.getLastModifiedTime())); + return String.format("name: %s, size: %s, modified: %s\n", + e.getName(), + e.getSize(), + toLocalTime(e.getLastModifiedTime())); } private static String toLocalTime(FileTime t) { From 2fbb486ed3cce15cd2083b88ccfff10552830f8c Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Thu, 6 Mar 2025 16:40:36 +0100 Subject: [PATCH 07/12] Adding a test for the annotation processing Filer. --- .../javac/NoOverwriteJarClassFilesTest.java | 4 +- .../javac/modules/AnnotationFilerTest.java | 191 ++++++++++++++++++ 2 files changed, 193 insertions(+), 2 deletions(-) create mode 100644 test/langtools/tools/javac/modules/AnnotationFilerTest.java diff --git a/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java b/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java index b11b8d7faaa01..1c042e3d7ac8a 100644 --- a/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java +++ b/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java @@ -106,8 +106,8 @@ public void jarFileNotModified() throws IOException { tb.writeFile(LIB_SOURCE_NAME, OLD_LIB_SOURCE); // Compile the old (broken) source and then store the class file in the JAR. - // The class file generated he is in the lib/ directory, which we delete - // after making the JAR (just to be sure). + // The class file is generated in the lib/ directory, which we delete after + // making the JAR (just to be sure). new JavacTask(tb) .files(LIB_SOURCE_NAME) .run() diff --git a/test/langtools/tools/javac/modules/AnnotationFilerTest.java b/test/langtools/tools/javac/modules/AnnotationFilerTest.java new file mode 100644 index 0000000000000..76866f36884a6 --- /dev/null +++ b/test/langtools/tools/javac/modules/AnnotationFilerTest.java @@ -0,0 +1,191 @@ +/* + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8338675 + * @summary javac shouldn't silently change .jar files on the classpath + * @library /tools/lib + * @modules jdk.compiler/com.sun.tools.javac.api + * jdk.compiler/com.sun.tools.javac.main + * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask + * @run junit AnnotationFilerTest + */ + +import org.junit.jupiter.api.Test; +import toolbox.JarTask; +import toolbox.JavacTask; +import toolbox.ToolBox; + +import javax.annotation.processing.AbstractProcessor; +import javax.annotation.processing.Filer; +import javax.annotation.processing.ProcessingEnvironment; +import javax.annotation.processing.RoundEnvironment; +import javax.annotation.processing.SupportedAnnotationTypes; +import javax.lang.model.element.TypeElement; +import javax.tools.FileObject; +import javax.tools.JavaFileObject; +import java.io.IOException; +import java.net.URI; +import java.nio.file.FileSystem; +import java.nio.file.FileSystems; +import java.nio.file.Path; +import java.util.Set; + +import static javax.tools.StandardLocation.CLASS_OUTPUT; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class AnnotationFilerTest { + private static final String LIB_SOURCE = + """ + package lib; + public class LibClass { + public static final String FIELD = ""; + } + """; + + // Target source references the field to force library compilation. + private static final String TARGET_SOURCE = + """ + class TargetClass { + static final String VALUE = lib.LibClass.FIELD; + } + """; + + private static final String LIB_CLASS_TYPE_NAME = "lib.LibClass"; + private static final Path LIB_JAR = Path.of("lib.jar"); + + private static final String LIB_SOURCE_FILE_NAME = "lib/LibClass.java"; + private static final String LIB_CLASS_FILE_NAME = "lib/LibClass.class"; + + // These differ only in the filename suffix part and represent the trailing + // parts of a Path URI for source/class files in the JAR. The source file + // will exist, but the class file must not be created by annotation processing. + private static final String JAR_SOURCE_URI_SUFFIX = "lib.jar!/" + LIB_SOURCE_FILE_NAME; + private static final String JAR_CLASS_URI_SUFFIX = "lib.jar!/" + LIB_CLASS_FILE_NAME; + + @Test + public void classpathJarsCannotBeWrittenDuringProcessing() throws IOException { + ToolBox tb = new ToolBox(); + tb.createDirectories("lib"); + tb.writeFile(LIB_SOURCE_FILE_NAME, LIB_SOURCE); + new JarTask(tb, LIB_JAR).files(LIB_SOURCE_FILE_NAME).run(); + + // These are assertions about the test environment, not the code-under-test. + try (FileSystem zipFs = FileSystems.newFileSystem(LIB_JAR)) { + // The bug would have only manifested with writable JAR files, so assert that here. + assertFalse(zipFs.isReadOnly()); + // This is the JAR file URI for the *source* code. Later we attempt to create the + // sibling *class* file from within the annotation processor (which MUST FAIL). + // We get the source URI here to verify the naming convention of the JAR file + // URIs to prevent the _negative_ test we do later being fragile. + URI libUri = zipFs.getPath(LIB_SOURCE_FILE_NAME).toUri(); + assertEquals("jar", libUri.getScheme()); + assertTrue(libUri.getSchemeSpecificPart().endsWith(JAR_SOURCE_URI_SUFFIX)); + } + + // Code under test: + // Compile the target class with library source only available in the JAR. This should + // succeed, but MUST NOT be able to create files in the JAR during annotation processing. + new JavacTask(tb) + .processors(new TestAnnotationProcessor()) + .options("-implicit:none", "-g:source,lines,vars") + .sources(TARGET_SOURCE) + .classpath(LIB_JAR) + .run() + .writeAll(); + } + + @SupportedAnnotationTypes("*") + static class TestAnnotationProcessor extends AbstractProcessor { + private ProcessingEnvironment processingEnv = null; + + @Override + public void init(ProcessingEnvironment processingEnv) { + this.processingEnv = processingEnv; + } + + @Override + public boolean process(Set annotations, RoundEnvironment env) { + // Only run this once (during the final pass), or else we get a spurious failure + // about trying to recreate the class file (not allowed during annotation + // processing, but not what we are testing here). + if (!env.processingOver()) { + return false; + } + + TypeElement libType = processingEnv.getElementUtils().getTypeElement(LIB_CLASS_TYPE_NAME); + JavaFileObject libClass; + // This is the primary code-under-test. The Filer must not return a file object + // that's a sibling to the source file of the given type (that's in the JAR, + // which MUST NOT be modified). Before bug 8338675 was fixed, this would fail. + Filer filer = processingEnv.getFiler(); + try { + libClass = filer.createClassFile("LibClass", libType); + } catch (IOException e) { + throw new RuntimeException(e); + } + + // Double check that this is the expected file kind. + assertEquals(JavaFileObject.Kind.CLASS, libClass.getKind()); + + // Primary assertions: Check the file object's URI is not from the JAR file. + URI libUri = libClass.toUri(); + // If this were from the JAR file, the URI scheme would be "jar" (as tested + // for earlier during setup). + assertEquals("file", libUri.getScheme()); + + // Check that the URI is for the right class, but not in the JAR. + assertTrue(libUri.getSchemeSpecificPart().endsWith("/LibClass.class")); + // Testing a negative is fragile, but the earlier checks show this would be + // the expected path if the URI was referencing an entry in the JAR. + assertFalse(libUri.getSchemeSpecificPart().endsWith(JAR_CLASS_URI_SUFFIX)); + + // Additional regression testing for other file objects the Filer can create + // (all specified as originating from the LibClass type in the JAR). These + // should all create file objects, just not in the JAR. + try { + assertNonJar( + filer.createClassFile("FooClass", libType), + "/FooClass.class"); + assertNonJar( + filer.createSourceFile("BarClass", libType), + "/BarClass.java"); + assertNonJar( + filer.createResource(CLASS_OUTPUT, "lib", "data.txt", libType), + "/data.txt"); + } catch (IOException e) { + throw new RuntimeException(e); + } + return false; + } + } + + static void assertNonJar(FileObject file, String uriSuffix) { + URI uri = file.toUri(); + assertEquals("file", uri.getScheme()); + assertTrue(uri.getSchemeSpecificPart().endsWith(uriSuffix)); + } +} From ab27880fb93b7c04838bdc81d35d033091c6cc90 Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Tue, 11 Mar 2025 15:38:07 +0100 Subject: [PATCH 08/12] Updating from feedback. --- .../tools/javac/file/JavacFileManager.java | 2 +- .../javac/modules/AnnotationFilerTest.java | 19 +++---------------- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java index 49abbfec6e189..f4b94d4bb7ab3 100644 --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java @@ -918,7 +918,7 @@ private JavaFileObject getFileForOutput(Location location, String baseName = fileName.basename(); // Use the sibling to determine the output location where possible, unless // it is in a JAR/ZIP file (we don't attempt to write class files back into - // archives). See JDK-8338675. + // archives). if (sibling instanceof PathFileObject pathFileObject && !pathFileObject.isJarFile()) { return pathFileObject.getSibling(baseName); } else { diff --git a/test/langtools/tools/javac/modules/AnnotationFilerTest.java b/test/langtools/tools/javac/modules/AnnotationFilerTest.java index 76866f36884a6..3aa01b73d103d 100644 --- a/test/langtools/tools/javac/modules/AnnotationFilerTest.java +++ b/test/langtools/tools/javac/modules/AnnotationFilerTest.java @@ -25,7 +25,7 @@ * @test * @bug 8338675 * @summary javac shouldn't silently change .jar files on the classpath - * @library /tools/lib + * @library /tools/lib /tools/javac/lib * @modules jdk.compiler/com.sun.tools.javac.api * jdk.compiler/com.sun.tools.javac.main * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask @@ -37,11 +37,7 @@ import toolbox.JavacTask; import toolbox.ToolBox; -import javax.annotation.processing.AbstractProcessor; -import javax.annotation.processing.Filer; -import javax.annotation.processing.ProcessingEnvironment; import javax.annotation.processing.RoundEnvironment; -import javax.annotation.processing.SupportedAnnotationTypes; import javax.lang.model.element.TypeElement; import javax.tools.FileObject; import javax.tools.JavaFileObject; @@ -118,15 +114,7 @@ public void classpathJarsCannotBeWrittenDuringProcessing() throws IOException { .writeAll(); } - @SupportedAnnotationTypes("*") - static class TestAnnotationProcessor extends AbstractProcessor { - private ProcessingEnvironment processingEnv = null; - - @Override - public void init(ProcessingEnvironment processingEnv) { - this.processingEnv = processingEnv; - } - + static class TestAnnotationProcessor extends JavacTestingAbstractProcessor { @Override public boolean process(Set annotations, RoundEnvironment env) { // Only run this once (during the final pass), or else we get a spurious failure @@ -136,12 +124,11 @@ public boolean process(Set annotations, RoundEnvironment return false; } - TypeElement libType = processingEnv.getElementUtils().getTypeElement(LIB_CLASS_TYPE_NAME); + TypeElement libType = elements.getTypeElement(LIB_CLASS_TYPE_NAME); JavaFileObject libClass; // This is the primary code-under-test. The Filer must not return a file object // that's a sibling to the source file of the given type (that's in the JAR, // which MUST NOT be modified). Before bug 8338675 was fixed, this would fail. - Filer filer = processingEnv.getFiler(); try { libClass = filer.createClassFile("LibClass", libType); } catch (IOException e) { From 2905c223e95ec08c60e133a723337da9a79a921c Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Fri, 21 Mar 2025 16:08:13 +0100 Subject: [PATCH 09/12] Rewriting test and merging annotation tests based on feedback. --- .../javac/NoOverwriteJarClassFilesTest.java | 200 ------------- .../javac/modules/AnnotationFilerTest.java | 178 ----------- .../filer/TestNoOverwriteJarFiles.java | 280 ++++++++++++++++++ 3 files changed, 280 insertions(+), 378 deletions(-) delete mode 100644 test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java delete mode 100644 test/langtools/tools/javac/modules/AnnotationFilerTest.java create mode 100644 test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java diff --git a/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java b/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java deleted file mode 100644 index 1c042e3d7ac8a..0000000000000 --- a/test/langtools/tools/javac/NoOverwriteJarClassFilesTest.java +++ /dev/null @@ -1,200 +0,0 @@ -/* - * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. - * - * This code is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License version 2 only, as - * published by the Free Software Foundation. - * - * This code is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License - * version 2 for more details (a copy is included in the LICENSE file that - * accompanied this code). - * - * You should have received a copy of the GNU General Public License version - * 2 along with this work; if not, write to the Free Software Foundation, - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. - * - * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA - * or visit www.oracle.com if you need additional information or have any - * questions. - */ - -/* - * @test - * @bug 8338675 - * @summary javac shouldn't silently change .jar files on the classpath - * @library /tools/lib - * @modules jdk.compiler/com.sun.tools.javac.api - * jdk.compiler/com.sun.tools.javac.main - * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask - * @run junit NoOverwriteJarClassFilesTest - */ - -import toolbox.JavacTask; -import toolbox.ToolBox; - -import java.io.IOException; -import java.io.OutputStream; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.attribute.FileTime; -import java.time.Instant; -import java.time.ZoneId; -import java.util.jar.JarOutputStream; -import java.util.zip.ZipEntry; -import java.util.zip.ZipFile; - -import org.junit.jupiter.api.Test; - -import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; - -/** - * This test makes two specific assertions about javac behaviour when source - * files are found in the classpath. - * - *

    - *
  1. Source files found in classpath JAR files are not overwritten. - *
  2. Class files generated during compilation which are associated with any - * sources found in JAR files are written (flat) to the current directory. - *
- * - *

Note that this behaviour is not obviously well-defined, and should not - * be relied upon, but it matches previous JDK behaviour and so is tested here. - * - *

Specifically, the behaviour in (2) means library classes with the same base - * class name may be overwritten, and the resulting set of class files in the - * current directory may not be usable. - */ -public class NoOverwriteJarClassFilesTest { - - private static final String OLD_LIB_SOURCE = - """ - package lib; - public class LibClass { - public static final String OLD_FIELD = "This will not compile with Target"; - } - """; - - private static final String NEW_LIB_SOURCE = - """ - package lib; - public class LibClass { - public static final String NEW_FIELD = "Only this will compile with Target"; - } - """; - - // Target source references the field only available in the new source. - private static final String TARGET_SOURCE = - """ - class TargetClass { - static final String VALUE = lib.LibClass.NEW_FIELD; - } - """; - - private static final String LIB_SOURCE_NAME = "lib/LibClass.java"; - private static final String LIB_CLASS_NAME = "lib/LibClass.class"; - private static final Path LIB_JAR = Path.of("lib.jar"); - - @Test - public void jarFileNotModified() throws IOException { - ToolBox tb = new ToolBox(); - tb.createDirectories("lib"); - tb.writeFile(LIB_SOURCE_NAME, OLD_LIB_SOURCE); - - // Compile the old (broken) source and then store the class file in the JAR. - // The class file is generated in the lib/ directory, which we delete after - // making the JAR (just to be sure). - new JavacTask(tb) - .files(LIB_SOURCE_NAME) - .run() - .writeAll(); - - // The new (fixed) source is never written to disk, so if compilation works - // it proves it's getting it from the source file in the JAR. - Instant olderTime = Instant.now(); - Instant newerTime = olderTime.plusSeconds(1); - try (OutputStream jos = Files.newOutputStream(Path.of("lib.jar"))) { - JarOutputStream jar = new JarOutputStream(jos); - // Important: The JAR file entry order *matters* if the timestamps are - // the same (the latter entry is used for compilation, regardless of - // whether it's a source file or a class file). So in this test, we - // put the one we want to use *first* with a newer timestamp, to show - // that it's definitely the timestamp being used to select the source. - writeEntry(jar, LIB_SOURCE_NAME, NEW_LIB_SOURCE.getBytes(), newerTime); - // Source is newer than the (broken) compiled class, so should compile - // from the source file in the JAR. If timestamps were not set, or set - // equal, the test would use this (broken) class file, and fail. - writeEntry(jar, LIB_CLASS_NAME, Files.readAllBytes(Path.of(LIB_CLASS_NAME)), olderTime); - jar.close(); - } - // Check there's no output file present and delete the original library files. - Path outputClassFile = Path.of("LibClass.class"); - if (Files.exists(outputClassFile)) { - throw new IllegalStateException("Output class file should not exist (yet)."); - } - Path libDir = Path.of("lib"); - tb.cleanDirectory(libDir); - tb.deleteFiles(libDir); - - // Before running the test itself, get the CRC of the class file in the JAR. - long originalLibCrc = getLibCrc(); - // And read the JAR file completely for comparison later. - byte[] originalJarContents = Files.readAllBytes(LIB_JAR); - - // Code under test: - // Compile the target class with new library source only available in the JAR. - // - // This compilation only succeeds if 'NEW_FIELD' exists, which is only in - // the source file written to the JAR, and nowhere on disk. - new JavacTask(tb) - .sources(TARGET_SOURCE) - .classpath(LIB_JAR) - .run() - .writeAll(); - - // Assertion 1: The class file in the JAR is unchanged. - // - // Since compilation succeeded, we know it used NEW_LIB_SOURCE, and if it - // wrote the class file back to the JAR (bad) then that should now have - // different contents. Note that the modification time of the class file - // is NOT modified, even if the JAR is updated, so we cannot test that. - assertEquals(originalLibCrc, getLibCrc(), "Class library contents were modified in the JAR file."); - assertArrayEquals(originalJarContents, Files.readAllBytes(LIB_JAR), "Jar file was modified."); - - // Assertion 2: An output class file was written to the current directory. - assertTrue(Files.exists(outputClassFile), "Output class file was not written to current directory."); - } - - // Note: JarOutputStream only writes modification time, not creation time, but - // that's what Javac uses to determine "newness" so it's fine. - private static void writeEntry(JarOutputStream jar, String name, byte[] bytes, Instant timestamp) throws IOException { - ZipEntry e = new ZipEntry(name); - e.setLastModifiedTime(FileTime.from(timestamp)); - jar.putNextEntry(e); - jar.write(bytes); - jar.closeEntry(); - } - - private static long getLibCrc() throws IOException { - try (ZipFile zipFile = new ZipFile("lib.jar")) { - // zipFile.stream().map(NoOverwriteJarClassFilesByDefault::format).forEach(System.err::println); - return zipFile.getEntry(LIB_CLASS_NAME).getCrc(); - } - } - - // ---- Debug helper methods ---- - private static String format(ZipEntry e) { - return String.format("name: %s, size: %s, modified: %s\n", - e.getName(), - e.getSize(), - toLocalTime(e.getLastModifiedTime())); - } - - private static String toLocalTime(FileTime t) { - return t != null ? t.toInstant().atZone(ZoneId.systemDefault()).toString() : ""; - } -} diff --git a/test/langtools/tools/javac/modules/AnnotationFilerTest.java b/test/langtools/tools/javac/modules/AnnotationFilerTest.java deleted file mode 100644 index 3aa01b73d103d..0000000000000 --- a/test/langtools/tools/javac/modules/AnnotationFilerTest.java +++ /dev/null @@ -1,178 +0,0 @@ -/* - * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. - * - * This code is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License version 2 only, as - * published by the Free Software Foundation. - * - * This code is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License - * version 2 for more details (a copy is included in the LICENSE file that - * accompanied this code). - * - * You should have received a copy of the GNU General Public License version - * 2 along with this work; if not, write to the Free Software Foundation, - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. - * - * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA - * or visit www.oracle.com if you need additional information or have any - * questions. - */ - -/* - * @test - * @bug 8338675 - * @summary javac shouldn't silently change .jar files on the classpath - * @library /tools/lib /tools/javac/lib - * @modules jdk.compiler/com.sun.tools.javac.api - * jdk.compiler/com.sun.tools.javac.main - * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask - * @run junit AnnotationFilerTest - */ - -import org.junit.jupiter.api.Test; -import toolbox.JarTask; -import toolbox.JavacTask; -import toolbox.ToolBox; - -import javax.annotation.processing.RoundEnvironment; -import javax.lang.model.element.TypeElement; -import javax.tools.FileObject; -import javax.tools.JavaFileObject; -import java.io.IOException; -import java.net.URI; -import java.nio.file.FileSystem; -import java.nio.file.FileSystems; -import java.nio.file.Path; -import java.util.Set; - -import static javax.tools.StandardLocation.CLASS_OUTPUT; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - -public class AnnotationFilerTest { - private static final String LIB_SOURCE = - """ - package lib; - public class LibClass { - public static final String FIELD = ""; - } - """; - - // Target source references the field to force library compilation. - private static final String TARGET_SOURCE = - """ - class TargetClass { - static final String VALUE = lib.LibClass.FIELD; - } - """; - - private static final String LIB_CLASS_TYPE_NAME = "lib.LibClass"; - private static final Path LIB_JAR = Path.of("lib.jar"); - - private static final String LIB_SOURCE_FILE_NAME = "lib/LibClass.java"; - private static final String LIB_CLASS_FILE_NAME = "lib/LibClass.class"; - - // These differ only in the filename suffix part and represent the trailing - // parts of a Path URI for source/class files in the JAR. The source file - // will exist, but the class file must not be created by annotation processing. - private static final String JAR_SOURCE_URI_SUFFIX = "lib.jar!/" + LIB_SOURCE_FILE_NAME; - private static final String JAR_CLASS_URI_SUFFIX = "lib.jar!/" + LIB_CLASS_FILE_NAME; - - @Test - public void classpathJarsCannotBeWrittenDuringProcessing() throws IOException { - ToolBox tb = new ToolBox(); - tb.createDirectories("lib"); - tb.writeFile(LIB_SOURCE_FILE_NAME, LIB_SOURCE); - new JarTask(tb, LIB_JAR).files(LIB_SOURCE_FILE_NAME).run(); - - // These are assertions about the test environment, not the code-under-test. - try (FileSystem zipFs = FileSystems.newFileSystem(LIB_JAR)) { - // The bug would have only manifested with writable JAR files, so assert that here. - assertFalse(zipFs.isReadOnly()); - // This is the JAR file URI for the *source* code. Later we attempt to create the - // sibling *class* file from within the annotation processor (which MUST FAIL). - // We get the source URI here to verify the naming convention of the JAR file - // URIs to prevent the _negative_ test we do later being fragile. - URI libUri = zipFs.getPath(LIB_SOURCE_FILE_NAME).toUri(); - assertEquals("jar", libUri.getScheme()); - assertTrue(libUri.getSchemeSpecificPart().endsWith(JAR_SOURCE_URI_SUFFIX)); - } - - // Code under test: - // Compile the target class with library source only available in the JAR. This should - // succeed, but MUST NOT be able to create files in the JAR during annotation processing. - new JavacTask(tb) - .processors(new TestAnnotationProcessor()) - .options("-implicit:none", "-g:source,lines,vars") - .sources(TARGET_SOURCE) - .classpath(LIB_JAR) - .run() - .writeAll(); - } - - static class TestAnnotationProcessor extends JavacTestingAbstractProcessor { - @Override - public boolean process(Set annotations, RoundEnvironment env) { - // Only run this once (during the final pass), or else we get a spurious failure - // about trying to recreate the class file (not allowed during annotation - // processing, but not what we are testing here). - if (!env.processingOver()) { - return false; - } - - TypeElement libType = elements.getTypeElement(LIB_CLASS_TYPE_NAME); - JavaFileObject libClass; - // This is the primary code-under-test. The Filer must not return a file object - // that's a sibling to the source file of the given type (that's in the JAR, - // which MUST NOT be modified). Before bug 8338675 was fixed, this would fail. - try { - libClass = filer.createClassFile("LibClass", libType); - } catch (IOException e) { - throw new RuntimeException(e); - } - - // Double check that this is the expected file kind. - assertEquals(JavaFileObject.Kind.CLASS, libClass.getKind()); - - // Primary assertions: Check the file object's URI is not from the JAR file. - URI libUri = libClass.toUri(); - // If this were from the JAR file, the URI scheme would be "jar" (as tested - // for earlier during setup). - assertEquals("file", libUri.getScheme()); - - // Check that the URI is for the right class, but not in the JAR. - assertTrue(libUri.getSchemeSpecificPart().endsWith("/LibClass.class")); - // Testing a negative is fragile, but the earlier checks show this would be - // the expected path if the URI was referencing an entry in the JAR. - assertFalse(libUri.getSchemeSpecificPart().endsWith(JAR_CLASS_URI_SUFFIX)); - - // Additional regression testing for other file objects the Filer can create - // (all specified as originating from the LibClass type in the JAR). These - // should all create file objects, just not in the JAR. - try { - assertNonJar( - filer.createClassFile("FooClass", libType), - "/FooClass.class"); - assertNonJar( - filer.createSourceFile("BarClass", libType), - "/BarClass.java"); - assertNonJar( - filer.createResource(CLASS_OUTPUT, "lib", "data.txt", libType), - "/data.txt"); - } catch (IOException e) { - throw new RuntimeException(e); - } - return false; - } - } - - static void assertNonJar(FileObject file, String uriSuffix) { - URI uri = file.toUri(); - assertEquals("file", uri.getScheme()); - assertTrue(uri.getSchemeSpecificPart().endsWith(uriSuffix)); - } -} diff --git a/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java b/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java new file mode 100644 index 0000000000000..6f574d6958cc1 --- /dev/null +++ b/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java @@ -0,0 +1,280 @@ +/* + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8338675 + * @summary javac shouldn't silently change .jar files on the classpath + * @library /tools/lib /tools/javac/lib + * @modules jdk.compiler/com.sun.tools.javac.api + * jdk.compiler/com.sun.tools.javac.main + * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask + * @run junit TestNoOverwriteJarFiles + */ + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.Execution; +import toolbox.JavacTask; +import toolbox.ToolBox; + +import javax.annotation.processing.RoundEnvironment; +import javax.lang.model.element.TypeElement; +import javax.tools.FileObject; +import java.io.IOException; +import java.io.OutputStream; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.time.Instant; +import java.util.Set; +import java.util.jar.JarOutputStream; +import java.util.zip.ZipEntry; + +import static javax.tools.StandardLocation.CLASS_OUTPUT; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD; + +/** + * Tests that javac cannot unexpectedly modify contents of JAR files on the + * class path. + *

+ * Consider the following javac behaviours: + *

    + *
  1. If there is no source path, javac searches the classpath for sources, + * including inside JAR files. + *
  2. If a Java source file was modified more recently that an existing class + * file, or if no class file exists, javac will compile it from the source. + *
  3. If there is no output directory specified, javac will put compiled class + * files next to their corresponding sources. + *
+ * Taken together, this suggests that a newly compiled class file should be + * written back into the JAR in which its source was found, possibly overwriting + * an existing class file entry. This would be very problematic. + *

+ * This test ensures javac will not modify JAR files on the classpath, even if + * it compiles sources contained within them. Instead, the class file will be + * written into the current working directory, which mimics the JDK 8 behavior. + * + *

Important

+ * + * This test creates files from Java compilation and annotation processing, and + * relies on files being written to the current working directory. Since jtreg + * currently offers no way to run each test case in its own directory, or clean + * the test directory between test cases, we must be careful to: + *
    + *
  • Use {@code @Execution(SAME_THREAD)} to run test cases sequentially. + *
  • Clean up the test directory ourselves between test cases (via + * {@code @BeforeEach}). + *
+ * The alternative approach would be to compile the test classes in a specified + * working directory unique to each test case, but this is currently only + * possible using a subprocess via {@code Task.Mode.EXEC} , and this has two + * serious disadvantages: + *
    + *
  • It significantly complicates compilation setup. + *
  • It prevents step-through debugging of the annotation processor. + *
+ */ +@Execution(SAME_THREAD) +public class TestNoOverwriteJarFiles { + private static final String LIB_SOURCE_FILE_NAME = "lib/LibClass.java"; + private static final String LIB_CLASS_FILE_NAME = "lib/LibClass.class"; + private static final String LIB_CLASS_TYPE_NAME = "lib.LibClass"; + + private static final Path TEST_LIB_JAR = Path.of("lib.jar"); + private static final Path OUTPUT_CLASS_FILE = Path.of("LibClass.class"); + + // Source which can only compile against the Java source in the test library. + public static final String TARGET_SOURCE = + """ + class TargetClass { + static final String VALUE = lib.LibClass.NEW_FIELD; + } + """; + + // Not expensive to create, but conceptually a singleton. + private static final ToolBox toolBox = new ToolBox(); + + @BeforeEach + public void cleanUpTestDirectory() throws IOException { + toolBox.cleanDirectory(Path.of(".")); + } + + @Test + public void jarFileNotModifiedOrdinaryCompilation() throws IOException { + byte[] originalJarBytes = compileTestLibJar(); + + new JavacTask(toolBox) + .sources(TARGET_SOURCE) + .classpath(TEST_LIB_JAR) + .run() + .writeAll(); + + // Assertion 1: The JAR is unchanged. + assertArrayEquals(originalJarBytes, Files.readAllBytes(TEST_LIB_JAR), "Jar file was modified."); + // Assertion 2: An output class file was written to the current directory. + assertTrue(Files.exists(OUTPUT_CLASS_FILE), "Output class file missing."); + } + + @Test + public void jarFileNotModifiedAnnotationProcessing() throws IOException { + byte[] originalJarBytes = compileTestLibJar(); + + new JavacTask(toolBox) + .sources(TARGET_SOURCE) + .classpath(TEST_LIB_JAR) + .processors(new TestAnnotationProcessor()) + // Use "-implicit:none" to avoid writing the library class file. + .options("-implicit:none", "-g:source,lines,vars") + .run() + .writeAll(); + + // Assertion 1: The JAR is unchanged. + assertArrayEquals(originalJarBytes, Files.readAllBytes(TEST_LIB_JAR), "Jar file was modified."); + // Assertion 2: All expected output files were written to the current directory. + assertDummyFile("DummySource.java"); + assertDummyFile("DummyClass.class"); + assertDummyFile("DummyResource.txt"); + // Assertion 3: The class file itself wasn't written (because we used "-implicit:none"). + assertFalse(Files.exists(OUTPUT_CLASS_FILE), "Unexpected class file in working directory."); + } + + static class TestAnnotationProcessor extends JavacTestingAbstractProcessor { + @Override + public boolean process(Set annotations, RoundEnvironment env) { + // Only run this once (in the final pass), or else we get spurious failures about + // trying to recreate file objects (not allowed during annotation processing). + if (!env.processingOver()) { + return false; + } + + TypeElement libClass = elements.getTypeElement(LIB_CLASS_TYPE_NAME); + try { + // Note: A generated Java source file must be legal Java, but a generated class + // file that's unreferenced will never be loaded, so can contain any bytes. + writeFileObject( + filer.createSourceFile("DummySource", libClass), + "DummySource.java", + "class DummySource {}"); + writeFileObject( + filer.createClassFile("DummyClass", libClass), + "DummyClass.class", + "<>"); + writeFileObject( + filer.createResource(CLASS_OUTPUT, "", "DummyResource.txt", libClass), + "DummyResource.txt", + "Dummy Resource Bytes"); + } catch (IOException e) { + throw new RuntimeException(e); + } + return false; + } + } + + static void writeFileObject(FileObject file, String expectedName, String contents) throws IOException { + URI fileUri = file.toUri(); + // Check that the file URI doesn't look like it is associated with a JAR. + assertTrue(fileUri.getSchemeSpecificPart().endsWith("/" + expectedName)); + // The JAR file system would have a scheme of "jar", not "file". + assertEquals("file", fileUri.getScheme()); + // Testing a negative is fragile, but a JAR URI would be expected to contain "jar!". + assertFalse(fileUri.getSchemeSpecificPart().contains("jar!")); + // Write dummy data (which should end up in the test working directory). + try (OutputStream os = file.openOutputStream()) { + os.write(contents.getBytes()); + } + } + + static void assertDummyFile(String filename) throws IOException { + Path path = Path.of(filename); + assertTrue(Files.exists(path), "Output file missing: " + filename); + assertTrue(Files.readString(path).contains("Dummy"), "Unexpected file contents: " + filename); + } + + // Compiles and writes the test library JAR (LIB_JAR) into the current directory. + static byte[] compileTestLibJar() throws IOException { + Path libDir = Path.of("lib"); + toolBox.createDirectories(libDir); + try { + toolBox.writeFile(LIB_SOURCE_FILE_NAME, + """ + package lib; + public class LibClass { + public static final String OLD_FIELD = "This will not compile with Target"; + } + """); + + // Compile the old (broken) source and then store the class file in the JAR. + // The class file is generated in the lib/ directory, which we delete after + // making the JAR. This ensures that when compiling the target class, it's + // the source file being read from the JAR, + new JavacTask(toolBox) + .files(LIB_SOURCE_FILE_NAME) + .run() + .writeAll(); + + // If timestamps are equal JAR file resolution of classes is ambiguous + // (currently "last one wins"), so give the source we want to be used a + // newer timestamp. + Instant now = Instant.now(); + try (OutputStream jos = Files.newOutputStream(Path.of("lib.jar"))) { + JarOutputStream jar = new JarOutputStream(jos); + writeEntry(jar, + LIB_SOURCE_FILE_NAME, + """ + package lib; + public class LibClass { + public static final String NEW_FIELD = "This will compile with Target"; + } + """.getBytes(StandardCharsets.UTF_8), + now.plusSeconds(1)); + writeEntry(jar, + LIB_CLASS_FILE_NAME, + Files.readAllBytes(Path.of(LIB_CLASS_FILE_NAME)), + now); + jar.close(); + } + // Return the JAR file bytes for comparison later. + return Files.readAllBytes(TEST_LIB_JAR); + } finally { + toolBox.cleanDirectory(libDir); + toolBox.deleteFiles(libDir); + } + } + + // Note: JarOutputStream only writes modification time, not creation time, but + // that's what Javac uses to determine "newness" so it's fine. + private static void writeEntry(JarOutputStream jar, String name, byte[] bytes, Instant timestamp) throws IOException { + ZipEntry e = new ZipEntry(name); + e.setLastModifiedTime(FileTime.from(timestamp)); + jar.putNextEntry(e); + jar.write(bytes); + jar.closeEntry(); + } +} From 0a3613b6a8f72b8b0676bdb8768d6ba0c7f15039 Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Mon, 24 Mar 2025 10:42:46 +0100 Subject: [PATCH 10/12] Adding sanity check that the test directory is empty (since we clean it between tests). --- .../processing/filer/TestNoOverwriteJarFiles.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java b/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java index 6f574d6958cc1..dde724b6b810a 100644 --- a/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java +++ b/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java @@ -32,6 +32,7 @@ * @run junit TestNoOverwriteJarFiles */ +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.Execution; @@ -51,6 +52,7 @@ import java.time.Instant; import java.util.Set; import java.util.jar.JarOutputStream; +import java.util.stream.Stream; import java.util.zip.ZipEntry; import static javax.tools.StandardLocation.CLASS_OUTPUT; @@ -121,6 +123,16 @@ class TargetClass { // Not expensive to create, but conceptually a singleton. private static final ToolBox toolBox = new ToolBox(); + @BeforeAll + public static void ensureEmptyTestDirectory() throws IOException { + try (var files = Files.walk(Path.of("."), 1)) { + // Always includes the given path as the first returned element, so skip it. + if (files.skip(1).findFirst().isPresent()) { + throw new IllegalStateException("Test working directory must be empty."); + } + } + } + @BeforeEach public void cleanUpTestDirectory() throws IOException { toolBox.cleanDirectory(Path.of(".")); From 1341c7c7e058988d35a5fee1dd74e4c1a3db2dac Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Mon, 24 Mar 2025 10:45:07 +0100 Subject: [PATCH 11/12] Remove unused import. --- .../tools/javac/processing/filer/TestNoOverwriteJarFiles.java | 1 - 1 file changed, 1 deletion(-) diff --git a/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java b/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java index dde724b6b810a..f6154143a64f5 100644 --- a/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java +++ b/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java @@ -52,7 +52,6 @@ import java.time.Instant; import java.util.Set; import java.util.jar.JarOutputStream; -import java.util.stream.Stream; import java.util.zip.ZipEntry; import static javax.tools.StandardLocation.CLASS_OUTPUT; From 6fcea57f2925b2ed6f17f3b0d6b7239adb8e6168 Mon Sep 17 00:00:00 2001 From: David Beaumont Date: Tue, 1 Apr 2025 14:12:48 +0200 Subject: [PATCH 12/12] Add final test for sourcepath entry (same behaviour). --- .../filer/TestNoOverwriteJarFiles.java | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java b/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java index f6154143a64f5..735cdf1b90431 100644 --- a/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java +++ b/test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java @@ -153,6 +153,23 @@ public void jarFileNotModifiedOrdinaryCompilation() throws IOException { assertTrue(Files.exists(OUTPUT_CLASS_FILE), "Output class file missing."); } + // As above, but the JAR is added to the source path instead (with same results). + @Test + public void jarFileNotModifiedForSourcePath() throws IOException { + byte[] originalJarBytes = compileTestLibJar(); + + new JavacTask(toolBox) + .sources(TARGET_SOURCE) + .sourcepath(TEST_LIB_JAR) + .run() + .writeAll(); + + // Assertion 1: The JAR is unchanged. + assertArrayEquals(originalJarBytes, Files.readAllBytes(TEST_LIB_JAR), "Jar file was modified."); + // Assertion 2: An output class file was written to the current directory. + assertTrue(Files.exists(OUTPUT_CLASS_FILE), "Output class file missing."); + } + @Test public void jarFileNotModifiedAnnotationProcessing() throws IOException { byte[] originalJarBytes = compileTestLibJar(); @@ -208,7 +225,8 @@ public boolean process(Set annotations, RoundEnvironment } } - static void writeFileObject(FileObject file, String expectedName, String contents) throws IOException { + static void writeFileObject(FileObject file, String expectedName, String contents) + throws IOException { URI fileUri = file.toUri(); // Check that the file URI doesn't look like it is associated with a JAR. assertTrue(fileUri.getSchemeSpecificPart().endsWith("/" + expectedName)); @@ -281,7 +299,8 @@ public class LibClass { // Note: JarOutputStream only writes modification time, not creation time, but // that's what Javac uses to determine "newness" so it's fine. - private static void writeEntry(JarOutputStream jar, String name, byte[] bytes, Instant timestamp) throws IOException { + private static void writeEntry(JarOutputStream jar, String name, byte[] bytes, Instant timestamp) + throws IOException { ZipEntry e = new ZipEntry(name); e.setLastModifiedTime(FileTime.from(timestamp)); jar.putNextEntry(e);