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

david-beaumont · 2025-06-04T12:08:38Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8356645: Javac should utilize new ZIP file system read-only access mode (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25639/head:pull/25639
$ git checkout pull/25639

Update a local copy of the PR:
$ git checkout pull/25639
$ git pull https://git.openjdk.org/jdk.git pull/25639/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25639

View PR using the GUI difftool:
$ git pr show -t 25639

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25639.diff

Using Webrev

Link to Webrev Comment

david-beaumont

Some pre-review comments.

david-beaumont · 2025-06-04T12:10:03Z

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

+     *                       file-system from a multi-release JAR (or
+     *                       {@code null} to ignore release versioning).
+     */
+    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.

I am a bit uneasy about having the method static, as this class's API is normally instance methods. I think that an instance of FSInfo (fsInfo) is available in both JavacFileManager and Locations, so it would be better to simply have an instance method. (The static field is OK, that's not an API.)

Interesting idea since it permits per instance changes (e.g. due to flags/environment) later.
However there's no instance (yet) in VerifyCTSymClassFiles. I could mint one though.
Thoughts?

david-beaumont · 2025-06-04T12:11:13Z

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

-                // or if non "*.jar" files are on the classpath.
-                this.fileSystem = FileSystems.newFileSystem(archivePath, env, (ClassLoader)null);
+                // or if non "*.jar" files are on the classpath. If this is not a ZIP/JAR file then it
+                // 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.

david-beaumont · 2025-06-04T12:12:57Z

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

@@ -1463,7 +1460,7 @@ private Pair<String,Path> inferModuleName(Path p) {
                                log.error(Errors.LocnCantReadFile(p));
                                return null;
                            }
-                            fs = jarFSProvider.newFileSystem(p, Collections.emptyMap());
+                            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.

This seems like a fallback path, so don't think we should add a no-arg version to promote skipping the release version.

I think just readOnlyJarFSEnv(null) is OK.

Left it using readOnlyJarFSEnv(null)

david-beaumont · 2025-06-04T12:14:19Z

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

@@ -96,6 +94,14 @@ public PlatformDescription getPlatformTrusted(String platformName) {

    private static final String[] symbolFileLocation = { "lib", "ct.sym" };

+    // These must match attributes defined in ZipFileSystem.java.
+    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.

david-beaumont · 2025-06-04T12:15:41Z

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

@@ -117,7 +123,7 @@ public PlatformDescription getPlatformTrusted(String platformName) {
        SUPPORTED_JAVA_PLATFORM_VERSIONS = new TreeSet<>(NUMERICAL_COMPARATOR);
        Path ctSymFile = findCtSym();
        if (Files.exists(ctSymFile)) {
-            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.

david-beaumont · 2025-06-04T12:17:15Z

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

+                        fs = FileSystems.newFileSystem(file, CT_SYM_ZIP_ENV);
+                        // If for any reason this was not opened from a ZIP file,
+                        // then the resulting file system would not be read-only.
+                        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).

We typically don't use assert in javac anymore, as it can be disabled. Either the condition is important, then we typically use com.sun.tools.javac.util.Assert.check, or we don't do the check. I think using Assert here would be appropriate.

Hmmm .... question:

The other place using FSInfo static method was the cySym verifier. And this is the cySym code. So it feels like it would be good to be consistent between both these cases. So is this a place for FSInfo as well, or not?

The advantage of FSInfo is that you can get the JAR provider (which we know is the ZIP provider too) and not have to worry about mismatches between given file and "discovered" file system type. You just only use the JAR provider and you're done.

I removed the FSInfo use in the test, just not worth the dependency.

david-beaumont · 2025-06-04T12:17:47Z

test/langtools/tools/javac/api/file/SJFM_TestBase.java

@@ -160,7 +161,7 @@ List<Path> getTestFilePaths() throws IOException {
    List<Path> getTestZipPaths() throws IOException {
        if (zipfs == null) {
            Path testZip = createSourceZip();
-            zipfs = FileSystems.newFileSystem(testZip);
+            zipfs = FileSystems.newFileSystem(testZip, Map.of("accessMode", "readOnly"));


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

david-beaumont · 2025-06-04T12:19:30Z

test/langtools/tools/javac/jvm/ClassRefDupInConstantPoolTest.java

 import java.lang.classfile.*;
 import java.lang.classfile.constantpool.*;

 public class ClassRefDupInConstantPoolTest {
+


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

The class file is from this test file instead of the test libraries, and this pattern of shipping additional classes in the same compilation unit for class file handling in tests is common in jdk/classfile and other packages. Could the clean directive be the culprit?

It's possible. I remember having to change this when I prototyped the new code a while ago, but I'm not sure I remember exactly why. I can try reverting it if the original style is acceptable (personally I prefer to compile on-the-fly since it can test the compiler with different options - which is probably why I changed it originally).

Also for compile on-the-fly, I usually use InMemoryJavaCompiler and ByteCodeLoader available with /test/lib library. InMemoryJavaCompiler conveniently returns a list of all class files, and its one-class-file version throws an exception if a compilation unit generates more than one class, which is not usually checked by manual invocations to javac task.

For new tests, I would often prefer to use on-the-fly compilation (although javac has its own frameworks to do that, like the ToolBox used here).

For existing tests, I generally prefer to restrict changes to a minimum (although opinions may differ on that).

But here, I wonder: why is the test failing? I didn't see anything in the patch that would seem to be an obvious cause of a breakage of a test like this. So, I think it deserves a good look to find out what's the cause of the test failing, rather than just adjusting the test to pass.

When I revert to the original code, I get:

java.lang.NullPointerException: Cannot invoke "java.io.InputStream.readAllBytes()" because the return value of "java.lang.Class.getResourceAsStream(String)" is null at ClassRefDupInConstantPoolTest.main(ClassRefDupInConstantPoolTest.java:40) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) at java.base/java.lang.reflect.Method.invoke(Method.java:565) at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) at java.base/java.lang.Thread.run(Thread.java:1474) JavaTest Message: Test threw exception: java.lang.NullPointerException: Cannot invoke "java.io.InputStream.readAllBytes()" because the return value of "java.lang.Class.getResourceAsStream(String)" is null JavaTest Message: shutting down test

when running via IntelliJ, but it passes when running via make. So there's some fragility with the environment.

I probably didn't realize originally that this was conditionally failing, so just changed it on the assumption that it was failing everywhere.

I have just verified that syncing back to master causes this test to fail when run via IntelliJ, so it's been "broken" for a while (whether it is at fault, or something in the way IntelliJ runs tests is at fault, I'm not sure). I'll revert it in this PR then, since it's not related to my change (I guess I just stumbled over the break and thought it was, so fixed it). Thanks for getting me to dig deeper.

david-beaumont · 2025-06-04T12:20:14Z

test/langtools/tools/javac/platform/VerifyCTSymClassFiles.java

-        try (FileSystem fs = FileSystems.newFileSystem(ctSym)) {
+        // Expected to always be a ZIP filesystem.
+        try (FileSystem fs = FileSystems.newFileSystem(ctSym, FSInfo.readOnlyJarFSEnv(null))) {
+            // 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.

jtreg runs tests with -ea, so either works fine.

For me: not relying on assert is preferred, esp. in tests. I.e. what is here currently seems good to me.

But you would be okay with this assert, or just happy to shrug and leave it as read-write if it somehow wasn't a ZIP file system? Alternately we could do what I see done elsewhere and extract the ZIP file system provider and use it directly?

I think this test is verifying ct.sym is sensible, and we can extend that to make sure the file can be opened in the correct way. I.e., I would keep the check at this place as you did.

bridgekeeper · 2025-06-04T13:08:23Z

👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-06-04T13:11:56Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-06-04T13:14:05Z

@david-beaumont The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-06-04T13:19:43Z

Webrevs

ExE-Boss · 2025-06-06T23:03:31Z

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

-            env.put("zipinfo-time", "false");
-
+            // Common parameters for opening ZIP/JAR file systems in Javac.
+            Map<String, ?> env = FSInfo.readOnlyJarFSEnv(multiReleaseValue);


Note that this will now cause .zip files to be treated the same as .jar files when multiReleaseValue ≠ null (previously, this was ignored for .zip files)

That's an excellent point. I will be sure to ask people if that looks acceptable and revert to the original behaviour otherwise. Thank you for spotting it and saying something.

I think I would prefer to no send the multi release value for non-jar files - possibly there's no observable difference at this time, but the multi-release jar is for jars only.

(Also the comment above the variable seems a bit superfluous.)

Done. Though the more times I write readOnlyJarFSEnv(null) the more I think a no-arg version of that method might carry its weight.

lahodaj

Overall seems like a reasonable direction. Please see some comments inline.

lahodaj · 2025-06-11T05:43:31Z

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

+     *                       file-system from a multi-release JAR (or
+     *                       {@code null} to ignore release versioning).
+     */
+    public static Map<String, ?> readOnlyJarFSEnv(String releaseVersion) {


I am a bit uneasy about having the method static, as this class's API is normally instance methods. I think that an instance of FSInfo (fsInfo) is available in both JavacFileManager and Locations, so it would be better to simply have an instance method. (The static field is OK, that's not an API.)

lahodaj · 2025-06-11T05:45:48Z

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

-            env.put("zipinfo-time", "false");
-
+            // Common parameters for opening ZIP/JAR file systems in Javac.
+            Map<String, ?> env = FSInfo.readOnlyJarFSEnv(multiReleaseValue);


I think I would prefer to no send the multi release value for non-jar files - possibly there's no observable difference at this time, but the multi-release jar is for jars only.

(Also the comment above the variable seems a bit superfluous.)

lahodaj · 2025-06-11T05:48:24Z

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

@@ -1463,7 +1460,7 @@ private Pair<String,Path> inferModuleName(Path p) {
                                log.error(Errors.LocnCantReadFile(p));
                                return null;
                            }
-                            fs = jarFSProvider.newFileSystem(p, Collections.emptyMap());
+                            fs = jarFSProvider.newFileSystem(p, FSInfo.readOnlyJarFSEnv(null));


I think just readOnlyJarFSEnv(null) is OK.

lahodaj · 2025-06-11T05:50:38Z

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

+                        fs = FileSystems.newFileSystem(file, CT_SYM_ZIP_ENV);
+                        // If for any reason this was not opened from a ZIP file,
+                        // then the resulting file system would not be read-only.
+                        assert fs.isReadOnly();


We typically don't use assert in javac anymore, as it can be disabled. Either the condition is important, then we typically use com.sun.tools.javac.util.Assert.check, or we don't do the check. I think using Assert here would be appropriate.

lahodaj · 2025-06-11T05:51:56Z

test/langtools/tools/javac/platform/VerifyCTSymClassFiles.java

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


For me: not relying on assert is preferred, esp. in tests. I.e. what is here currently seems good to me.

lahodaj · 2025-06-11T06:01:04Z

test/langtools/tools/javac/jvm/ClassRefDupInConstantPoolTest.java

 import java.lang.classfile.*;
 import java.lang.classfile.constantpool.*;

 public class ClassRefDupInConstantPoolTest {
+


For new tests, I would often prefer to use on-the-fly compilation (although javac has its own frameworks to do that, like the ToolBox used here).

For existing tests, I generally prefer to restrict changes to a minimum (although opinions may differ on that).

But here, I wonder: why is the test failing? I didn't see anything in the patch that would seem to be an obvious cause of a breakage of a test like this. So, I think it deserves a good look to find out what's the cause of the test failing, rather than just adjusting the test to pass.

…le systems.

openjdk · 2025-06-18T19:22:56Z

@david-beaumont Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

david-beaumont · 2025-06-18T20:25:03Z

Apologies for the recent force push. I'm having to switch laptops and this is coming from a different local repo.

david-beaumont · 2025-06-18T20:28:25Z

test/langtools/tools/javac/platform/VerifyCTSymClassFiles.java

@@ -56,6 +57,8 @@ public static void main(String... args) throws IOException, URISyntaxException {
        t.checkClassFiles();
    }

+    private final FSInfo fsInfo = FSInfo.instance(new Context());


I don't think this is actually right unless we make the other cySym code use FSInfo. Happy to hear people's thoughts on this. One benefit here is that we know we've matched the expected ZIP file with the ZIP file system provider, so no need to worry if the environment was going to be honoured.

I changed both case (test and code) to NOT use FSInfo now. The dependency and extra code (esp. now the method is an instance method) just doesn't seem worth it.

david-beaumont · 2025-06-19T11:22:46Z

Opened https://bugs.openjdk.org/browse/JDK-8360022 for the weird failing test thing (it was @cleanup, good call whoever suggested that!).

david-beaumont commented Jun 4, 2025

View reviewed changes

openjdk bot added the rfr Pull request is ready for review label Jun 4, 2025

openjdk bot added the compiler [email protected] label Jun 4, 2025

ExE-Boss reviewed Jun 6, 2025

View reviewed changes

lahodaj reviewed Jun 11, 2025

View reviewed changes

Applying read-only configuration for Javac internal use of ZIP/JAR fi…

bc2f523

…le systems.

david-beaumont force-pushed the jdk_8356645 branch from 94f36b1 to bc2f523 Compare June 18, 2025 19:21

Some (not all) feedback addressed. Still open questions.

a18a12d

david-beaumont commented Jun 18, 2025

View reviewed changes

Reverted test (not caused by this PR).

ab5e98d

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

david-beaumont commented Jun 4, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

david-beaumont left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jun 4, 2025

Uh oh!

openjdk bot commented Jun 4, 2025

Uh oh!

openjdk bot commented Jun 4, 2025

Uh oh!

mlbridge bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-beaumont commented Jun 4, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 4, 2025 •

edited

Loading

david-beaumont commented Jun 18, 2025 •

edited

Loading