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