On Sat, 22 Mar 2025 17:28:04 GMT, Alan Bateman <[email protected]> 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
