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

[Jaikiran Pai]

On Mon, 19 May 2025 12:22:11 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> ""would this change now have a chance of introducing a ReadOnlyFileSystemException when constructing the ZipFileSystem whereas before it wouldn't?""
>> 
>> If there was ever a "ReadOnlyFileSystemException" when initializing the ZipFileSystem, we have a very serious bug already. Since we already have the opening of Zip working for cases where the underlying file is read-only in the file system, it has to be true that no attempt is made to modify the file contents.
>> 
>> While I accept that this new code now calls out to functions with this flag in a different state to before, I believe this is an absolute improvement since:
>> 
>> 1. It is not acceptable to say to users "we support a read-only mode" if during initialization we might modify the file in any way (even including changing last-modification times etc.).
>> 
>> 2. All the evidence points to whichever operations are being done during init as being fundamentally "read-only" (both due to it working on read-only zip files, and there being no conceptual need to modify anything during setup).
>> 
>> I'll happily do a thorough audit of everything which could be affected by this change if that will give you confidence, but I would not want to change this code back to its default "read-write until stated otherwise" behaviour.
>
> Hello David, I had another look at this code and after going through it, it looked like `readOnly` field can in fact be made `final` because of the refactoring changes that you did in this PR. I checked out your latest PR locally and here's the additional change I did:
> 
> 
> diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
> index e2fddd96fe8..f54b5360ac5 100644
> --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
> +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
> @@ -110,8 +110,7 @@ class ZipFileSystem extends FileSystem {
>      private final Path zfpath;
>      final ZipCoder zc;
>      private final ZipPath rootdir;
> -    // Starts in readOnly (safe mode), but might be reset at the end of initialization.
> -    private boolean readOnly = true;
> +    private final boolean readOnly;
>  
>      // default time stamp for pseudo entries
>      private final long zfsDefaultTimeStamp = System.currentTimeMillis();
> @@ -227,11 +226,6 @@ static ZipAccessMode from(Object value) {
>  
>          // Determining a release version uses 'this' instance to read paths etc.
>          Optional<Integer> multiReleaseVersion = determineReleaseVersion(env);
> -
> -        // Set the version-based lookup function for multi-release JARs.
> -        this.entryLookup =
> -                multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
> -
>          // We only allow read-write zip/jar files if they are not multi-release
>          // JARs and the underlying file is writable.
>          this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() || !Files.isWritable(zfpath);
> @@ -241,6 +235,10 @@ static ZipAccessMode from(Object value) {
>                      : "the ZIP file is not writable";
>              throw new IOException(reason);
>          }
> +        // Set the version-based lookup function for multi-release JARs.
> +        this.entryLookup =
> +                multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
> +
>      }
>  
>      /**
> 
> 
> 
> 
> With the refactoring changes you have done so far, we are now able to determine the ultimate value for `readOnly` before anything in the construction of `ZipFileSystem` would need access to it. Does this additional change look reasonable to you? I haven't run any tests against this change.
> 
> Making it `final` and having it not accessed until the ultimate value is set in the constructor would get us past any of t...

On second thought, may be this isn't enough. I see that I missed the fact that `determineReleaseVersion()` requires access to `this`. Please leave this in the current form that you have in your PR. I need a few more moments to consider if anything needs to be changed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095591964