RFR: 8350880: (zipfs) Add support for read-only zip file systems

[David Beaumont]

On Mon, 12 May 2025 09:40:56 GMT, David Beaumont <duke at openjdk.org> wrote:

> Adding read-only support to ZipFileSystem.
> 
> The new `accessMode` environment property allows for readOnly and readWrite values, and ensures that the requested mode is consistent with what's returned.
> 
> This involved a little refactoring to ensure that "read only" state was set initially and only unset at the end of initialization if appropriate.
> 
> By making 2 methods return values (rather than silently set non-final fields as a side effect) it's now clear in what order fields are initialized and which are final (sadly there are still non-final fields, but only a split of this class into two types can fix that, since determining multi-jar support requires reading the file system).

Preloading some explanatory comment.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 83:

> 81:     private static final boolean isWindows = System.getProperty("os.name")
> 82:             .startsWith("Windows");
> 83:     private static final byte[] ROOTPATH = new byte[]{'/'};

Whitespace format changes from my editor - will revert (sorry hadn't spotted).

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 89:

> 87: 
> 88:     // Posix file permissions allow per-file access control in a posix-like fashion.
> 89:     // Note that using a "readOnly" access mode will force all files, and the

Not correct now - will fix.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 198:

> 196:         this.defaultOwner = supportPosix ? initOwner(zfpath, env) : null;
> 197:         this.defaultGroup = supportPosix ? initGroup(zfpath, env) : null;
> 198:         this.defaultPermissions = supportPosix ? Collections.unmodifiableSet(initPermissions(env)) : null;

Ensuring this is unmodifiable to avoid risk of returning a modifiable enum set.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 221:

> 219:         }
> 220:         // sm and existence check
> 221:         zfpath.getFileSystem().provider().checkAccess(zfpath, java.nio.file.AccessMode.READ);

Type name clash with existing AccessMode class. Since new enum is private, happy to rename.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 241:

> 239:         // is why they are the only non-final fields), and it requires that the
> 240:         // inode map has been initialized.
> 241:         Optional<Integer> multiReleaseVersion = determineReleaseVersion(env);

Now release version and entry lookup determination always happen on a read-only file system.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 259:

> 257: 
> 258:         // Pass "this" as a parameter after everything else is set up.
> 259:         this.rootdir = new ZipPath(this, new byte[]{'/'});

This could probably be set above release version etc. but it's a choice of either:
1. waiting until everything is set up before passing "this"
2. letting an incomplete "this" instance get passed to another class (possible escape risk?)

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 264:

> 262:     /**
> 263:      * Return the compression method to use (STORED or DEFLATED).  If the
> 264:      * property {@code compressionMethod} is set use its value to determine

Typo fix (too many m's).

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 377:

> 375:         Object o = env.get(PROPERTY_DEFAULT_PERMISSIONS);
> 376:         if (o == null) {
> 377:             return PosixFilePermissions.fromString("rwxrwxrwx");

Since DEFAULT_PERMISSIONS was mutable and only used exactly once, it seems better to just inline it where it's clear what it actually is based off. Better readability.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1458:

> 1456:      * use its value to determine the requested version. If not use the value of the "multi-release" property.
> 1457:      */
> 1458:     private Optional<Integer> determineReleaseVersion(Map<String, ?> env) throws IOException {

Refactored to return the value rather than side-effect instance fields.

test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 180:

> 178:      */
> 179:     @Test
> 180:     public void testNoSuchFileFailure() {

In theory some of these could be split more, but they are uncomplicated sanity checks.

test/jdk/jdk/nio/zipfs/Utils.java line 52:

> 50:      */
> 51:     static Path createJarFile(String name, String... entries) throws IOException {
> 52:         Path jarFile = Paths.get(name);

Previous tests always had the test file called the same thing.
Note that in jtreg tests, the file of the same name often already exists in the test directory.
I tried adding a "ensure file doesn't already exist" check and it fails lots of tests.

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

PR Review: https://git.openjdk.org/jdk/pull/25178#pullrequestreview-2832583353
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084284309
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084288335
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084292488
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084296094
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084297859
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084301697
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084302415
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084306071
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084307536
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084311454
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084317768