Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc2f523
Applying read-only configuration for Javac internal use of ZIP/JAR fi…
david-beaumont Jun 18, 2025
a18a12d
Some (not all) feedback addressed. Still open questions.
david-beaumont Jun 18, 2025
ab5e98d
Reverted test (not caused by this PR).
david-beaumont Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,19 @@

package com.sun.tools.javac.file;

import java.io.IOError;
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.InvalidPathException;
import java.nio.file.Path;
import java.nio.file.spi.FileSystemProvider;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.StringTokenizer;
import java.util.jar.Attributes;
import java.util.jar.JarFile;
Expand Down Expand Up @@ -163,4 +163,30 @@ public synchronized FileSystemProvider getJarFSProvider() {
return null;
}

// Must match the keys/values expected by ZipFileSystem.java.
private static final Map<String, String> READ_ONLY_JARFS_ENV = Map.of(
// Jar files opened by Javac should always be read-only.
"accessMode", "readOnly",
// ignores timestamps not stored in ZIP central directory, reducing I/O.
"zipinfo-time", "false");

/**
* Returns a {@link java.nio.file.FileSystem FileSystem} environment map
* suitable for creating read-only JAR file-systems with default timestamp
* information via {@link FileSystemProvider#newFileSystem(Path, Map)}
* or {@link java.nio.file.FileSystems#newFileSystem(Path, Map)}.
*
* @param releaseVersion the release version to use when creating a
* file-system from a multi-release JAR (or
* {@code null} to ignore release versioning).
*/
public Map<String, ?> readOnlyJarFSEnv(String releaseVersion) {
if (releaseVersion == null) {
return READ_ONLY_JARFS_ENV;
}
// Multi-release JARs need an additional attribute.
Map<String, String> env = new HashMap<>(READ_ONLY_JARFS_ENV);
env.put("releaseVersion", releaseVersion);
return Collections.unmodifiableMap(env);
}
}
14 changes: 6 additions & 8 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,24 +561,22 @@ private final class ArchiveContainer implements Container {

public ArchiveContainer(Path archivePath) throws IOException, ProviderNotFoundException {
this.archivePath = archivePath;
Map<String,String> env = new HashMap<>();
// ignores timestamps not stored in ZIP central directory, reducing I/O
// This key is handled by ZipFileSystem only.
env.put("zipinfo-time", "false");

if (multiReleaseValue != null && archivePath.toString().endsWith(".jar")) {
env.put("multi-release", multiReleaseValue);
FileSystemProvider jarFSProvider = fsInfo.getJarFSProvider();
Assert.checkNonNull(jarFSProvider, "should have been caught before!");
Map<String, ?> env = fsInfo.readOnlyJarFSEnv(multiReleaseValue);
try {
this.fileSystem = jarFSProvider.newFileSystem(archivePath, env);
} catch (ZipException ze) {
throw new IOException("ZipException opening \"" + archivePath.getFileName() + "\": " + ze.getMessage(), ze);
}
} else {
// Less common case is possible if the file manager was not initialized in JavacTask,
// 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// However, Javac should never attempt to write back to archives either way.
Map<String, ?> env = fsInfo.readOnlyJarFSEnv(null);
this.fileSystem = FileSystems.newFileSystem(archivePath, env);
}
packages = new HashMap<>();
for (Path root : fileSystem.getRootDirectories()) {
Expand Down
15 changes: 6 additions & 9 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/file/Locations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,10 @@
import com.sun.tools.javac.resources.CompilerProperties.LintWarnings;
import jdk.internal.jmod.JmodFile;

import com.sun.tools.javac.code.Lint;
import com.sun.tools.javac.code.Lint.LintCategory;
import com.sun.tools.javac.main.Option;
import com.sun.tools.javac.resources.CompilerProperties.Errors;
import com.sun.tools.javac.resources.CompilerProperties.Warnings;
import com.sun.tools.javac.util.DefinedBy;
import com.sun.tools.javac.util.DefinedBy.Api;
import com.sun.tools.javac.util.JCDiagnostic.Warning;
import com.sun.tools.javac.util.ListBuffer;
import com.sun.tools.javac.util.Log;
import com.sun.tools.javac.jvm.ModuleNameReader;
Expand Down Expand Up @@ -141,7 +137,7 @@ public class Locations {

Map<Path, FileSystem> fileSystems = new LinkedHashMap<>();
List<Closeable> closeables = new ArrayList<>();
private Map<String,String> fsEnv = Collections.emptyMap();
private String releaseVersion = null;

Locations() {
initHandlers();
Expand Down Expand Up @@ -233,7 +229,8 @@ private Iterable<Path> getPathEntries(String searchPath, Path emptyPathDefault)
}

public void setMultiReleaseValue(String multiReleaseValue) {
fsEnv = Collections.singletonMap("releaseVersion", multiReleaseValue);
// Null is implicitly allowed and unsets the value.
this.releaseVersion = multiReleaseValue;
}

private boolean contains(Collection<Path> searchPath, Path file) throws IOException {
Expand Down Expand Up @@ -480,7 +477,7 @@ Location getLocationForModule(String moduleName) throws IOException {
}

/**
* @see JavaFileManager#getLocationForModule(Location, JavaFileObject, String)
* @see JavaFileManager#getLocationForModule(Location, JavaFileObject)
*/
Location getLocationForModule(Path file) throws IOException {
return null;
Expand Down Expand Up @@ -1387,7 +1384,7 @@ private Pair<String,Path> inferModuleName(Path p) {
log.error(Errors.NoZipfsForArchive(p));
return null;
}
try (FileSystem fs = jarFSProvider.newFileSystem(p, fsEnv)) {
try (FileSystem fs = jarFSProvider.newFileSystem(p, fsInfo.readOnlyJarFSEnv(releaseVersion))) {
Path moduleInfoClass = fs.getPath("module-info.class");
if (Files.exists(moduleInfoClass)) {
String moduleName = readModuleName(moduleInfoClass);
Expand Down Expand Up @@ -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));
try {
Path moduleInfoClass = fs.getPath("classes/module-info.class");
String moduleName = readModuleName(moduleInfoClass);
Expand Down
18 changes: 14 additions & 4 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/platform/JDKPlatformProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,8 @@
import javax.annotation.processing.Processor;
import javax.tools.ForwardingJavaFileObject;
import javax.tools.JavaFileManager;
import javax.tools.JavaFileManager.Location;
import javax.tools.JavaFileObject;
import javax.tools.JavaFileObject.Kind;
import javax.tools.StandardJavaFileManager;
import javax.tools.StandardLocation;

import com.sun.source.util.Plugin;
Expand Down Expand Up @@ -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(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Symbol file should always be opened read-only.
"accessMode", "readOnly",
// Uses less accurate, but faster, timestamp information
// (nobody should care about timestamps in the CT symbol file).
"zipinfo-time", "false");

private static final Set<String> SUPPORTED_JAVA_PLATFORM_VERSIONS;
public static final Comparator<String> NUMERICAL_COMPARATOR = (s1, s2) -> {
int i1;
Expand All @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try (FileSystem fs = FileSystems.newFileSystem(ctSymFile, CT_SYM_ZIP_ENV);
DirectoryStream<Path> dir =
Files.newDirectoryStream(fs.getRootDirectories().iterator().next())) {
for (Path section : dir) {
Expand Down Expand Up @@ -249,7 +255,11 @@ public String inferBinaryName(Location location, JavaFileObject file) {
try {
FileSystem fs = ctSym2FileSystem.get(file);
if (fs == null) {
ctSym2FileSystem.put(file, fs = FileSystems.newFileSystem(file, (ClassLoader)null));
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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ctSym2FileSystem.put(file, fs);
}

Path root = fs.getRootDirectories().iterator().next();
Expand Down
3 changes: 2 additions & 1 deletion test/langtools/tools/javac/api/file/SJFM_TestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.zip.ZipEntry;
import java.util.zip.ZipOutputStream;
Expand Down Expand Up @@ -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"));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

closeables.add(zipfs);
zipPaths = Files.list(zipfs.getRootDirectories().iterator().next())
.filter(p -> p.getFileName().toString().endsWith(".java"))
Expand Down
24 changes: 21 additions & 3 deletions test/langtools/tools/javac/jvm/ClassRefDupInConstantPoolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,37 @@
* @test
* @bug 8015927
* @summary Class reference duplicates in constant pool
* @clean ClassRefDupInConstantPoolTest$Duplicates
* @library /tools/lib
* @modules jdk.compiler/com.sun.tools.javac.api
* jdk.compiler/com.sun.tools.javac.main
* @build toolbox.JavacTask toolbox.ToolBox
* @run main ClassRefDupInConstantPoolTest
*/

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.TreeSet;

import toolbox.JavacTask;
import toolbox.ToolBox;

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

public class ClassRefDupInConstantPoolTest {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private static final String DUPLICATE_REFS_CLASS =
"""
class Duplicates {
String concat(String s1, String s2) {
return s1 + (s2 == s1 ? " " : s2);
}
}""";

public static void main(String[] args) throws Exception {
ClassModel cls = ClassFile.of().parse(ClassRefDupInConstantPoolTest.class.
getResourceAsStream("ClassRefDupInConstantPoolTest$Duplicates.class").readAllBytes());
new JavacTask(new ToolBox()).sources(DUPLICATE_REFS_CLASS).run();

ClassModel cls = ClassFile.of().parse(Files.readAllBytes(Path.of("Duplicates.class")));
ConstantPool pool = cls.constantPool();

int duplicates = 0;
Expand Down
9 changes: 8 additions & 1 deletion test/langtools/tools/javac/platform/VerifyCTSymClassFiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@
* @summary Verify classfile inside ct.sym
* @library /tools/lib
* @modules jdk.compiler/com.sun.tools.javac.api
* jdk.compiler/com.sun.tools.javac.file
* jdk.compiler/com.sun.tools.javac.main
* jdk.compiler/com.sun.tools.javac.platform
* jdk.compiler/com.sun.tools.javac.util:+open
* @build toolbox.ToolBox VerifyCTSymClassFiles
* @run main VerifyCTSymClassFiles
*/

import com.sun.tools.javac.file.FSInfo;
import com.sun.tools.javac.util.Context;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.lang.classfile.ClassFile;
Expand All @@ -53,14 +57,17 @@ public static void main(String... args) throws IOException, URISyntaxException {
t.checkClassFiles();
}

private final FSInfo fsInfo = FSInfo.instance(new Context());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


void checkClassFiles() throws IOException {
Path ctSym = Paths.get(System.getProperty("java.home"), "lib", "ct.sym");

if (!Files.exists(ctSym)) {
//no ct.sym, nothing to check:
return ;
}
try (FileSystem fs = FileSystems.newFileSystem(ctSym)) {
// Expected to always be a ZIP filesystem.
try (var fs = fsInfo.getJarFSProvider().newFileSystem(ctSym, fsInfo.readOnlyJarFSEnv(null))) {
Files.walk(fs.getRootDirectories().iterator().next())
.filter(p -> Files.isRegularFile(p))
.forEach(p -> checkClassFile(p));
Expand Down