RFR: 8347712: IllegalStateException on multithreaded ZipFile access with non-UTF8 charset [v5]
Eirik Bjørsnøs
eirbjo at openjdk.org
Sun Mar 23 08:58:10 UTC 2025
On Sat, 22 Mar 2025 17:28:04 GMT, Alan Bateman <alanb at 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
More information about the core-libs-dev
mailing list