On Sat, 22 Mar 2025 17:28:04 GMT, Alan Bateman <al...@openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains nine additional 
>> commits since the last revision:
>> 
>>  - Eirik's suggestion - update test method comment
>>  - rename entryNameCharset to charset in the test
>>  - Eirik's suggestion - update test to even call ZipFile.getEntry()
>>  - Eirik's inputs - replace useUTF8Coder() with zipCoderFor()
>>  - merge latest from master branch
>>  - add code comment
>>  - rename field
>>  - tiny typo fix in newly introduced documentation
>>  - 8347712: IllegalStateException on multithreaded ZipFile access with 
>> non-UTF8 charset
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 384:
> 
>> 382:      * @param fallback the fallback ZipCoder to return if the entry 
>> doesn't require UTF-8
>> 383:      */
>> 384:     private static ZipCoder zipCoderFor(final byte[] cen, final int 
>> pos, final ZipCoder fallback) {
> 
> Have you tried putting an instance method on ZipFile instead as it has access 
> to the zip coder. That would give you better abstraction and avoid needing to 
> introduce "fallback" as that is confusing to see here.

I agree with Alan that an instance method would provide better abstraction and 
avoid the somewhat clumsy "fallback" parameter introduced in my previous patch.

Here's a patch exploring the instance method:


Index: src/java.base/share/classes/java/util/zip/ZipFile.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/java.base/share/classes/java/util/zip/ZipFile.java 
b/src/java.base/share/classes/java/util/zip/ZipFile.java
--- a/src/java.base/share/classes/java/util/zip/ZipFile.java    (revision 
83ac59fcd292c21bbff4e0ac243a6bf07b4b21dc)
+++ b/src/java.base/share/classes/java/util/zip/ZipFile.java    (date 
1742719028475)
@@ -202,7 +202,7 @@
         long t0 = System.nanoTime();
 
         this.zipCoder = ZipCoder.get(charset);
-        this.res = new CleanableResource(this, zipCoder, file, mode);
+        this.res = new CleanableResource(this, file, mode);
 
         PerfCounter.getZipFileOpenTime().addElapsedTimeFrom(t0);
         PerfCounter.getZipFileCount().increment();
@@ -292,7 +292,7 @@
             // Look up the name and CEN header position of the entry.
             // The resolved name may include a trailing slash.
             // See Source::getEntryPos for details.
-            EntryPos pos = res.zsrc.getEntryPos(name, true, zipCoder);
+            EntryPos pos = res.zsrc.getEntryPos(name, true, this);
             if (pos != null) {
                 entry = getZipEntry(pos.name, pos.pos);
             }
@@ -332,7 +332,7 @@
             if (Objects.equals(lastEntryName, entry.name)) {
                 pos = lastEntryPos;
             } else {
-                EntryPos entryPos = zsrc.getEntryPos(entry.name, false, 
zipCoder);
+                EntryPos entryPos = zsrc.getEntryPos(entry.name, false, this);
                 if (entryPos != null) {
                     pos = entryPos.pos;
                 } else {
@@ -374,26 +374,25 @@
      * <p>
      * A ZIP entry's name and comment fields may be encoded using UTF-8, in
      * which case this method returns a UTF-8 capable {@code ZipCoder}. If the
-     * entry doesn't require UTF-8, then this method returns the {@code 
fallback}
-     * {@code ZipCoder}.
+     * entry doesn't require UTF-8, then this method returns the
+     * {@code ZipCoder} of the ZipFile.
      *
      * @param cen the CEN
      * @param pos the ZIP entry's position in CEN
-     * @param fallback the fallback ZipCoder to return if the entry doesn't 
require UTF-8
      */
-    private static ZipCoder zipCoderFor(final byte[] cen, final int pos, final 
ZipCoder fallback) {
-        if (fallback.isUTF8()) {
-            // the fallback ZipCoder is capable of handling UTF-8,
+    private ZipCoder zipCoderFor(final byte[] cen, final int pos) {
+        if (zipCoder.isUTF8()) {
+            // the ZipCoder is capable of handling UTF-8,
             // so no need to parse the entry flags to determine if
             // the entry has UTF-8 flag.
-            return fallback;
+            return zipCoder;
         }
         if ((CENFLG(cen, pos) & USE_UTF8) != 0) {
             // entry requires a UTF-8 ZipCoder
             return ZipCoder.UTF8;
         }
         // entry doesn't require a UTF-8 ZipCoder
-        return fallback;
+        return zipCoder;
     }
 
     private static class InflaterCleanupAction implements Runnable {
@@ -594,7 +593,7 @@
     private String getEntryName(int pos) {
         byte[] cen = res.zsrc.cen;
         int nlen = CENNAM(cen, pos);
-        ZipCoder zc = zipCoderFor(cen, pos, zipCoder);
+        ZipCoder zc = zipCoderFor(cen, pos);
         return zc.toString(cen, pos + CENHDR, nlen);
     }
 
@@ -665,7 +664,7 @@
         }
         if (clen != 0) {
             int start = pos + CENHDR + nlen + elen;
-            ZipCoder zc = zipCoderFor(cen, pos, zipCoder);
+            ZipCoder zc = zipCoderFor(cen, pos);
             e.comment = zc.toString(cen, start, clen);
         }
         lastEntryName = e.name;
@@ -697,12 +696,11 @@
 
         Source zsrc;
 
-        CleanableResource(ZipFile zf, ZipCoder zipCoder, File file, int mode) 
throws IOException {
-            assert zipCoder != null : "null ZipCoder";
+        CleanableResource(ZipFile zf, File file, int mode) throws IOException {
             this.cleanable = CleanerFactory.cleaner().register(zf, this);
             this.istreams = Collections.newSetFromMap(new WeakHashMap<>());
             this.inflaterCache = new ArrayDeque<>();
-            this.zsrc = Source.get(file, (mode & OPEN_DELETE) != 0, zipCoder);
+            this.zsrc = Source.get(file, (mode & OPEN_DELETE) != 0, zf);
         }
 
         void clean() {
@@ -1492,12 +1490,12 @@
         private static final java.nio.file.FileSystem builtInFS =
                 DefaultFileSystemProvider.theFileSystem();
 
-        static Source get(File file, boolean toDelete, ZipCoder zipCoder) 
throws IOException {
+        static Source get(File file, boolean toDelete, ZipFile zipFile) throws 
IOException {
             final Key key;
             try {
                 key = new Key(file,
                         Files.readAttributes(builtInFS.getPath(file.getPath()),
-                                BasicFileAttributes.class), 
zipCoder.charset());
+                                BasicFileAttributes.class), 
zipFile.zipCoder.charset());
             } catch (InvalidPathException ipe) {
                 throw new IOException(ipe);
             }
@@ -1509,7 +1507,7 @@
                     return src;
                 }
             }
-            src = new Source(key, toDelete, zipCoder);
+            src = new Source(key, toDelete, zipFile);
 
             synchronized (files) {
                 Source prev = files.putIfAbsent(key, src);
@@ -1531,7 +1529,7 @@
             }
         }
 
-        private Source(Key key, boolean toDelete, ZipCoder zipCoder) throws 
IOException {
+        private Source(Key key, boolean toDelete, ZipFile zipFile) throws 
IOException {
             this.key = key;
             if (toDelete) {
                 if (OperatingSystem.isWindows()) {
@@ -1545,7 +1543,7 @@
                 this.zfile = new RandomAccessFile(key.file, "r");
             }
             try {
-                initCEN(-1, zipCoder);
+                initCEN(-1, zipFile);
                 byte[] buf = new byte[4];
                 readFullyAt(buf, 0, 4, 0);
                 this.startsWithLoc = (LOCSIG(buf) == LOCSIG);
@@ -1700,7 +1698,7 @@
         }
 
         // Reads ZIP file central directory.
-        private void initCEN(final int knownTotal, final ZipCoder zipCoder) 
throws IOException {
+        private void initCEN(final int knownTotal, final ZipFile zipFile) 
throws IOException {
             // Prefer locals for better performance during startup
             byte[] cen;
             if (knownTotal == -1) {
@@ -1762,13 +1760,13 @@
                     // This will only happen if the ZIP file has an incorrect
                     // ENDTOT field, which usually means it contains more than
                     // 65535 entries.
-                    initCEN(countCENHeaders(cen), zipCoder);
+                    initCEN(countCENHeaders(cen), zipFile);
                     return;
                 }
 
                 int entryPos = pos + CENHDR;
                 // the ZipCoder for any non-UTF8 entries
-                final ZipCoder entryZipCoder = zipCoderFor(cen, pos, zipCoder);
+                final ZipCoder entryZipCoder = zipFile.zipCoderFor(cen, pos);
                 // Checks the entry and adds values to entries[idx ... idx+2]
                 int nlen = checkAndAddEntry(pos, idx, entryZipCoder);
                 idx += 3;
@@ -1842,7 +1840,7 @@
          * to the specified entry name, or {@code null} if not found.
          */
         private EntryPos getEntryPos(final String name, final boolean addSlash,
-                                     final ZipCoder zipCoder) {
+                                     final ZipFile zipFile) {
             if (total == 0) {
                 return null;
             }
@@ -1861,7 +1859,7 @@
                     int noff = pos + CENHDR;
                     int nlen = CENNAM(cen, pos);
 
-                    final ZipCoder zc = zipCoderFor(cen, pos, zipCoder);
+                    final ZipCoder zc = zipFile.zipCoderFor(cen, pos);
                     // Compare the lookup name with the name encoded in the CEN
                     switch (zc.compare(name, cen, noff, nlen, addSlash)) {
                         case ZipCoder.EXACT_MATCH:

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2009052136

Reply via email to