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 all 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
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