RFR: 8356645: Javac should utilize new ZIP file system read-only access mode

Wed Jun 4 13:19:54 UTC 2025

On Wed, 4 Jun 2025 12:08:38 GMT, David Beaumont <duke at openjdk.org> wrote:

> This PR seeks to integrate the new ZipFileSystem "accessMode" parameter to open internal ZIP/JAR files as read only, to act as defense in-depth against accidental modification.
> 
> Note that this currently also propagates the (currently undocumented) "zipinfo-time" parameter to several other places where ZIP/JAR files are opened, which is likely to improve performance. This was discussed and is expected to be safe (but it's something to be careful about).
> This will, of course, be thoroughly tested before integration.
> 
> It also unifies several places to use a common helper method to obtain the environment map, adds more comments, and changes a small number of affected tests.
> 
> I'm also happy to update the original bug description to include the timestamp related changes as necessary.

Some pre-review comments.

src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java line 183:

> 181:      *                       {@code null} to ignore release versioning).
> 182:      */
> 183:     public static Map<String, ?> readOnlyJarFSEnv(String releaseVersion) {

I named this after the getJarFSProvider() method above, since they are used in close proximity, but would be happy to consider other names.

src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java line 577:

> 575:                 // Less common case is possible if the file manager was not initialized in JavacTask,
> 576:                 // or if non "*.jar" files are on the classpath. If this is not a ZIP/JAR file then it
> 577:                 // will ignore ZIP specific parameters in env, and may not end up being read-only.

Added comment just to pull out the fact that we might not be able to promise that all ArchiveContainer instances are backed by a read-only file system, and the compiler still has a duty not to attempt any modification.

src/jdk.compiler/share/classes/com/sun/tools/javac/file/Locations.java line 1463:

> 1461:                                 return null;
> 1462:                             }
> 1463:                             fs = jarFSProvider.newFileSystem(p, FSInfo.readOnlyJarFSEnv(null));

Happy to consider either commenting the null here (e.g. `readOnlyJarFSEnv(/* releaseVersion */ null)`), or adding a no-arg version of the method in FSInfo.

src/jdk.compiler/share/classes/com/sun/tools/javac/platform/JDKPlatformProvider.java line 98:

> 96: 
> 97:     // These must match attributes defined in ZipFileSystem.java.
> 98:     private static final Map<String, ?> CT_SYM_ZIP_ENV = Map.of(

Didn't use FSInfo here, even though it's mostly the same logic, because the symbol file is a ZIP, not a JAR, so naming gets a little muddled, and in this case there's no runtime info, so I can just make a constant map.

src/jdk.compiler/share/classes/com/sun/tools/javac/platform/JDKPlatformProvider.java line 120:

> 118:         Path ctSymFile = findCtSym();
> 119:         if (Files.exists(ctSymFile)) {
> 120:             try (FileSystem fs = FileSystems.newFileSystem(ctSymFile, (ClassLoader)null);

Several places in existing code pass "(ClassLoader) null", which by inspection makes no difference compared to simply not passing the parameter, so I took the decision to remove it for neatness.

src/jdk.compiler/share/classes/com/sun/tools/javac/platform/JDKPlatformProvider.java line 261:

> 259:                         // If for any reason this was not opened from a ZIP file,
> 260:                         // then the resulting file system would not be read-only.
> 261:                         assert fs.isReadOnly();

Not sure if an assert is the right thing here or if this should be an always-on runtime check (it can be provoked by having a non-ZIP symbol file, but it's not sanctioned in any way).

test/langtools/tools/javac/api/file/SJFM_TestBase.java line 164:

> 162:         if (zipfs == null) {
> 163:             Path testZip = createSourceZip();
> 164:             zipfs = FileSystems.newFileSystem(testZip, Map.of("accessMode", "readOnly"));

Not strictly necessary, but seems like a good idea to use read-only for testing.

test/langtools/tools/javac/jvm/ClassRefDupInConstantPoolTest.java line 46:

> 44: 
> 45: public class ClassRefDupInConstantPoolTest {
> 46: 

This test was failing and needed to be compiling its own class file rather than reading the one in the test library.

test/langtools/tools/javac/platform/VerifyCTSymClassFiles.java line 68:

> 66:         // Expected to always be a ZIP filesystem.
> 67:         try (FileSystem fs = FileSystems.newFileSystem(ctSym, FSInfo.readOnlyJarFSEnv(null))) {
> 68:             // Check that the file system is read only (not true if not a zip file system).

Another case where I'm not 100% sure if it should be an assert, or a runtime exception/error.

-------------

PR Review: https://git.openjdk.org/jdk/pull/25639#pullrequestreview-2896486601
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126444200
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126446371
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126449539
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126452047
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126454461
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126457316
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126458265
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126461426
PR Review Comment: https://git.openjdk.org/jdk/pull/25639#discussion_r2126462843