8347712: IllegalStateException on multithreaded ZipFile access with non-UTF8 charset

8355975: ZipFile uses incorrect Charset if another instance for the same ZIP file was constructed with a different Charset

Co-authored-by: Eirik Bjørsnøs <eirbjo@openjdk.org>
Reviewed-by: eirbjo, lancea, redestad, alanb

Author: jaikiran

src/java.base/share/classes/java/util/zip/ZipCoder.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -38,7 +38,10 @@
 import sun.nio.cs.UTF_8;
 
 /**
- * Utility class for ZIP file entry name and comment decoding and encoding
+ * Utility class for ZIP file entry name and comment decoding and encoding.
+ * <p>
+ * The {@code ZipCoder} for UTF-8 charset is thread safe, {@code ZipCoder}
+ * for other charsets require external synchronization.
  */
 class ZipCoder {
 
@@ -174,6 +177,13 @@ protected CharsetDecoder decoder() {
         return dec;
     }
 
+    /**
+     * {@return the {@link Charset} used by this {@code ZipCoder}}
+     */
+    final Charset charset() {
+        return this.cs;
+    }
+
     private CharsetEncoder encoder() {
         if (enc == null) {
             enc = cs.newEncoder()

src/java.base/share/classes/java/util/zip/ZipFile.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1995, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -82,6 +82,8 @@ public class ZipFile implements ZipConstants, Closeable {
 
     private final String filePath;     // ZIP file path
     private final String fileName;     // name of the file
+    // Used when decoding entry names and comments
+    private final ZipCoder zipCoder;
     private volatile boolean closeRequested;
 
     // The "resource" used by this ZIP file that needs to be
@@ -198,7 +200,8 @@ public ZipFile(File file, int mode, Charset charset) throws IOException
         this.fileName = file.getName();
         long t0 = System.nanoTime();
 
-        this.res = new CleanableResource(this, ZipCoder.get(charset), file, mode);
+        this.zipCoder = ZipCoder.get(charset);
+        this.res = new CleanableResource(this, zipCoder, file, mode);
 
         PerfCounter.getZipFileOpenTime().addElapsedTimeFrom(t0);
         PerfCounter.getZipFileCount().increment();
@@ -265,7 +268,7 @@ public String getComment() {
             // If there is a problem decoding the byte array which represents
             // the ZIP file comment, return null;
             try {
-                return res.zsrc.zc.toString(res.zsrc.comment);
+                return zipCoder.toString(res.zsrc.comment);
             } catch (IllegalArgumentException iae) {
                 return null;
             }
@@ -288,7 +291,7 @@ public ZipEntry getEntry(String name) {
             // 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);
+            EntryPos pos = res.zsrc.getEntryPos(name, true, zipCoder);
             if (pos != null) {
                 entry = getZipEntry(pos.name, pos.pos);
             }
@@ -328,7 +331,7 @@ public InputStream getInputStream(ZipEntry entry) throws IOException {
             if (Objects.equals(lastEntryName, entry.name)) {
                 pos = lastEntryPos;
             } else {
-                EntryPos entryPos = zsrc.getEntryPos(entry.name, false);
+                EntryPos entryPos = zsrc.getEntryPos(entry.name, false, zipCoder);
                 if (entryPos != null) {
                     pos = entryPos.pos;
                 } else {
@@ -363,6 +366,35 @@ public InputStream getInputStream(ZipEntry entry) throws IOException {
         }
     }
 
+    /**
+     * Determines and returns a {@link ZipCoder} to use for decoding
+     * name and comment fields of the ZIP entry identified by the {@code pos}
+     * in the ZIP file's {@code cen}.
+     * <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}.
+     *
+     * @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,
+            // so no need to parse the entry flags to determine if
+            // the entry has UTF-8 flag.
+            return fallback;
+        }
+        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;
+    }
+
     private static class InflaterCleanupAction implements Runnable {
         private final Inflater inf;
         private final CleanableResource res;
@@ -561,7 +593,7 @@ public Stream<? extends ZipEntry> stream() {
     private String getEntryName(int pos) {
         byte[] cen = res.zsrc.cen;
         int nlen = CENNAM(cen, pos);
-        ZipCoder zc = res.zsrc.zipCoderForPos(pos);
+        ZipCoder zc = zipCoderFor(cen, pos, zipCoder);
         return zc.toString(cen, pos + CENHDR, nlen);
     }
 
@@ -632,7 +664,7 @@ private ZipEntry getZipEntry(String name, int pos) {
         }
         if (clen != 0) {
             int start = pos + CENHDR + nlen + elen;
-            ZipCoder zc = res.zsrc.zipCoderForPos(pos);
+            ZipCoder zc = zipCoderFor(cen, pos, zipCoder);
             e.comment = zc.toString(cen, start, clen);
         }
         lastEntryName = e.name;
@@ -664,11 +696,12 @@ private static class CleanableResource implements Runnable {
 
         Source zsrc;
 
-        CleanableResource(ZipFile zf, ZipCoder zc, File file, int mode) throws IOException {
+        CleanableResource(ZipFile zf, ZipCoder zipCoder, File file, int mode) throws IOException {
+            assert zipCoder != null : "null ZipCoder";
             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, zc);
+            this.zsrc = Source.get(file, (mode & OPEN_DELETE) != 0, zipCoder);
         }
 
         void clean() {
@@ -1109,6 +1142,7 @@ public void setExternalFileAttributes(ZipEntry ze, int externalFileAttributes) {
     // Represents the resolved name and position of a CEN record
     static record EntryPos(String name, int pos) {}
 
+    // Implementation note: This class is thread safe.
     private static class Source {
         // While this is only used from ZipFile, defining it there would cause
         // a bootstrap cycle that would leave this initialized as null
@@ -1121,7 +1155,6 @@ private static class Source {
         private static final int MAX_CEN_SIZE = ArraysSupport.SOFT_MAX_ARRAY_LENGTH;
 
         private final Key key;               // the key in files
-        private final @Stable ZipCoder zc;   // ZIP coder used to decode/encode
 
         private int refs = 1;
 
@@ -1157,8 +1190,9 @@ private static class Source {
         private int[] entries;                  // array of hashed cen entry
 
         // Checks the entry at offset pos in the CEN, calculates the Entry values as per above,
-        // then returns the length of the entry name.
-        private int checkAndAddEntry(int pos, int index)
+        // then returns the length of the entry name. Uses the given zipCoder for processing the
+        // entry name and the entry comment (if any).
+        private int checkAndAddEntry(final int pos, final int index, final ZipCoder zipCoder)
             throws ZipException
         {
             byte[] cen = this.cen;
@@ -1196,21 +1230,20 @@ private int checkAndAddEntry(int pos, int index)
             }
 
             try {
-                ZipCoder zcp = zipCoderForPos(pos);
-                int hash = zcp.checkedHash(cen, entryPos, nlen);
+                int hash = zipCoder.checkedHash(cen, entryPos, nlen);
                 int hsh = (hash & 0x7fffffff) % tablelen;
                 int next = table[hsh];
                 table[hsh] = index;
                 // Record the CEN offset and the name hash in our hash cell.
-                entries[index++] = hash;
-                entries[index++] = next;
-                entries[index  ] = pos;
+                entries[index] = hash;
+                entries[index + 1] = next;
+                entries[index + 2] = pos;
                 // Validate comment if it exists.
                 // If the bytes representing the comment cannot be converted to
                 // a String via zcp.toString, an Exception will be thrown
                 if (clen > 0) {
                     int start = entryPos + nlen + elen;
-                    zcp.toString(cen, start, clen);
+                    zipCoder.toString(cen, start, clen);
                 }
             } catch (Exception e) {
                 zerror("invalid CEN header (bad entry name or comment)");
@@ -1389,34 +1422,47 @@ private static boolean isZip64ExtBlockSizeValid(int blockSize, long csize,
         private int tablelen;                // number of hash heads
 
         /**
-         * A class representing a key to a ZIP file. A key is based
-         * on the file key if available, or the path value if the
-         * file key is not available. The key is also based on the
-         * file's last modified time to allow for cases where a ZIP
-         * file is re-opened after it has been modified.
+         * A class representing a key to the Source of a ZipFile.
+         * The Key is composed of:
+         * - The BasicFileAttributes.fileKey() if available, or the Path of the ZIP file
+         * if the fileKey() is not available.
+         * - The ZIP file's last modified time (to allow for cases
+         * where a ZIP file is re-opened after it has been modified).
+         * - The Charset that was provided when constructing the ZipFile instance.
+         * The unique combination of these components identifies a Source of a ZipFile.
          */
         private static class Key {
-            final BasicFileAttributes attrs;
-            File file;
-            final boolean utf8;
-
-            public Key(File file, BasicFileAttributes attrs, ZipCoder zc) {
+            private final BasicFileAttributes attrs;
+            private final File file;
+            // the Charset that was provided when constructing the ZipFile instance
+            private final Charset charset;
+
+            /**
+             * Constructs a {@code Key} to a {@code Source} of a {@code ZipFile}
+             *
+             * @param file    the ZIP file
+             * @param attrs   the attributes of the ZIP file
+             * @param charset the Charset that was provided when constructing the ZipFile instance
+             */
+            public Key(File file, BasicFileAttributes attrs, Charset charset) {
                 this.attrs = attrs;
                 this.file = file;
-                this.utf8 = zc.isUTF8();
+                this.charset = charset;
             }
 
+            @Override
             public int hashCode() {
-                long t = utf8 ? 0 : Long.MAX_VALUE;
+                long t = charset.hashCode();
                 t += attrs.lastModifiedTime().toMillis();
                 Object fk = attrs.fileKey();
                 return Long.hashCode(t) +
                         (fk != null ? fk.hashCode() : file.hashCode());
             }
 
+            @Override
             public boolean equals(Object obj) {
                 if (obj instanceof Key key) {
-                    if (key.utf8 != utf8) {
+                    if (!charset.equals(key.charset)) {
                         return false;
                     }
                     if (!attrs.lastModifiedTime().equals(key.attrs.lastModifiedTime())) {
@@ -1440,12 +1486,12 @@ public boolean equals(Object obj) {
         private static final java.nio.file.FileSystem builtInFS =
                 DefaultFileSystemProvider.theFileSystem();
 
-        static Source get(File file, boolean toDelete, ZipCoder zc) throws IOException {
+        static Source get(File file, boolean toDelete, ZipCoder zipCoder) throws IOException {
             final Key key;
             try {
                 key = new Key(file,
                         Files.readAttributes(builtInFS.getPath(file.getPath()),
-                                BasicFileAttributes.class), zc);
+                                BasicFileAttributes.class), zipCoder.charset());
             } catch (InvalidPathException ipe) {
                 throw new IOException(ipe);
             }
@@ -1457,7 +1503,7 @@ static Source get(File file, boolean toDelete, ZipCoder zc) throws IOException {
                     return src;
                 }
             }
-            src = new Source(key, toDelete, zc);
+            src = new Source(key, toDelete, zipCoder);
 
             synchronized (files) {
                 Source prev = files.putIfAbsent(key, src);
@@ -1479,8 +1525,7 @@ static void release(Source src) throws IOException {
             }
         }
 
-        private Source(Key key, boolean toDelete, ZipCoder zc) throws IOException {
-            this.zc = zc;
+        private Source(Key key, boolean toDelete, ZipCoder zipCoder) throws IOException {
             this.key = key;
             if (toDelete) {
                 if (OperatingSystem.isWindows()) {
@@ -1494,7 +1539,7 @@ private Source(Key key, boolean toDelete, ZipCoder zc) throws IOException {
                 this.zfile = new RandomAccessFile(key.file, "r");
             }
             try {
-                initCEN(-1);
+                initCEN(-1, zipCoder);
                 byte[] buf = new byte[4];
                 readFullyAt(buf, 0, 4, 0);
                 this.startsWithLoc = (LOCSIG(buf) == LOCSIG);
@@ -1649,7 +1694,7 @@ private End findEND() throws IOException {
         }
 
         // Reads ZIP file central directory.
-        private void initCEN(int knownTotal) throws IOException {
+        private void initCEN(final int knownTotal, final ZipCoder zipCoder) throws IOException {
             // Prefer locals for better performance during startup
             byte[] cen;
             if (knownTotal == -1) {
@@ -1711,13 +1756,15 @@ private void initCEN(int knownTotal) throws IOException {
                     // 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));
+                    initCEN(countCENHeaders(cen), zipCoder);
                     return;
                 }
 
                 int entryPos = pos + CENHDR;
+                // the ZipCoder for any non-UTF8 entries
+                final ZipCoder entryZipCoder = zipCoderFor(cen, pos, zipCoder);
                 // Checks the entry and adds values to entries[idx ... idx+2]
-                int nlen = checkAndAddEntry(pos, idx);
+                int nlen = checkAndAddEntry(pos, idx, entryZipCoder);
                 idx += 3;
 
                 // Adds name to metanames.
@@ -1741,7 +1788,7 @@ private void initCEN(int knownTotal) throws IOException {
                             try {
                                 // Compute hash code of name from "META-INF/versions/{version)/{name}
                                 int prefixLen = META_INF_VERSIONS_LEN + DecimalDigits.stringSize(version);
-                                int hashCode = zipCoderForPos(pos).checkedHash(cen,
+                                int hashCode = entryZipCoder.checkedHash(cen,
                                         entryPos + prefixLen,
                                         nlen - prefixLen);
                                 // Register version for this hash code
@@ -1786,9 +1833,10 @@ private static void zerror(String msg) throws ZipException {
 
         /*
          * Returns the resolved name and position of the ZIP cen entry corresponding
-         *  to the specified entry name, or {@code null} if not found.
+         * to the specified entry name, or {@code null} if not found.
          */
-        private EntryPos getEntryPos(String name, boolean addSlash) {
+        private EntryPos getEntryPos(final String name, final boolean addSlash,
+                                     final ZipCoder zipCoder) {
             if (total == 0) {
                 return null;
             }
@@ -1807,8 +1855,7 @@ private EntryPos getEntryPos(String name, boolean addSlash) {
                     int noff = pos + CENHDR;
                     int nlen = CENNAM(cen, pos);
 
-                    ZipCoder zc = zipCoderForPos(pos);
-
+                    final ZipCoder zc = zipCoderFor(cen, pos, zipCoder);
                     // Compare the lookup name with the name encoded in the CEN
                     switch (zc.compare(name, cen, noff, nlen, addSlash)) {
                         case ZipCoder.EXACT_MATCH:
@@ -1834,16 +1881,6 @@ private EntryPos getEntryPos(String name, boolean addSlash) {
             return null;
         }
 
-        private ZipCoder zipCoderForPos(int pos) {
-            if (zc.isUTF8()) {
-                return zc;
-            }
-            if ((CENFLG(cen, pos) & USE_UTF8) != 0) {
-                return ZipCoder.UTF8;
-            }
-            return zc;
-        }
-
         /**
          * Returns true if the bytes represent a non-directory name
          * beginning with "META-INF/", disregarding ASCII case.