JDK-8038491 : Improve synchronization in ZipFile.read()
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.jar
  • Affected Version: 8
  • Priority: P2
  • Status: Closed
  • Resolution: Fixed
  • Submitted: 2014-03-27
  • Updated: 2015-11-24
  • Resolved: 2014-04-24
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 6 JDK 7 JDK 8 JDK 9
6u111Fixed 7u95Fixed 8u20 b13Fixed 9Fixed
Description
Licensee reported issue.

A crash can occur when multiple threads work with the same ZipFileInputStream 
object for a ZipEntry in a ZipFile. We understand that the ZipFile APIs are 
not declared to be thread safe, but even so, the VM should not crash under 
these circumstances. The situation should be handled gracefully by throwing 
an Exception. 

The problem happens when one of the threads reaches the end of the stream and 
calls close() on it. The ZipFileInputStream.close() method clears up the 
native memory held by the jzentry and also sets the field holding the address 
of the native jzentry structure to 0. 

The majority of the read() and close() methods are conducted while holding a 
lock on the ZipFile object. If a thread calls read() on a closed stream, -1 
is returned by a check which is at the beginning of read(): 
 
    if (rem == 0) 
        return -1; 
 
However, there is a small time window between the above check and obtaining 
the lock, during which another thread can close the stream. This is the 
sequence of events: 
 
1. T1 calls read() and passes check described above 
2. T2 calls close() and frees up the jzentry 
3. T1 obtains the lock on the ZipFile object, and calls ZipFile.read() 
 
Step 3 leads to a crash because the jzentry has been freed. This is the stack 
trace (from an hs_err file on Windows): 
 
    Stack: [0x04fe0000,0x05030000],  sp=0x0502d3b4,  free space=308k 
    Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) 
    C  [zip.dll+0x323d]  ZIP_Read+0x8 
    C  [zip.dll+0x1d22]  Java_java_util_zip_ZipFile_read+0x68 
    j  java.util.zip.ZipFile.read(JJJ[BII)I+0 
    j  java.util.zip.ZipFile.access$1400(JJJ[BII)I+10 
    j  java.util.zip.ZipFile$ZipFileInputStream.read([BII)I+66 
    j  java.util.zip.ZipFile$ZipFileInflaterInputStream.fill()V+32 
    j  java.util.zip.InflaterInputStream.read([BII)I+100 
    j  java.util.zip.InflaterInputStream.read()I+11 
    j  ZipCrashTest.run()V+3 
    v  ~StubRoutines::call_stub 
    V  [jvm.dll+0x13f24a]  JavaCalls::call_helper+0x16a 
    V  [jvm.dll+0x202c6e]  os::os_exception_wrapper+0x6e 
    V  [jvm.dll+0x13f415]  JavaCalls::call_virtual+0x85 
    V  [jvm.dll+0x13f477]  JavaCalls::call_virtual+0x57 
    V  [jvm.dll+0xeb6bf]  thread_entry+0x5f 
    V  [jvm.dll+0x16042c]  JavaThread::thread_main_inner+0x8c 
    V  [jvm.dll+0x160e67]  JavaThread::run+0xf7 
    V  [jvm.dll+0x1a4a99]  java_start+0x99 
    C  [msvcr100.dll+0x5c556] 
    C  [msvcr100.dll+0x5c600] 
    C  [kernel32.dll+0x1336a] 
    C  [ntdll.dll+0x39f72] 
    C  [ntdll.dll+0x39f45] 
 
It is also important to note that by design, data can be read from the 
ZipFileInputStream even after the ZipFile has been closed. 
 
We believe that the the best way to avoid a crash is by adding a (rem == 0) 
check just before calling ZipFile.read(), when we already have the lock on 
the ZipFile (see diffs below). 
 

WORKAROUND 
---------- 
Refrain from sharing ZipFileInputStreams between multiple threads. 

SUGGESTED FIX
--------------------------- 
The suggested fix relative to 7u51 is described below. Note that we don't 
have to worry about the array length checks ((len <= 0) and (len > rem)) 
because these are duplicated in ZIP_Read() in zip_util.c. As an aside, it's 
not clear to me why we have the redundant checks in the Java layer. 
Performance of this code is crucial for class loading etc., so it might be 
worth considering the removal of the duplicate checks from ZipFile.read(). 

--- ZipFile-old.java    2013-12-14 01:58:06.000000000 +0000 
+++ ZipFile-new.java    2014-03-14 13:44:44.388224300 +0000 
@-664,9 +664,6 @
         } 
 
         public int read(byte b[], int off, int len) throws IOException { 
-            if (rem == 0) { 
-                return -1; 
-            } 
             if (len <= 0) { 
                 return 0; 
             } 
@-674,6 +671,9 @
                 len = (int) rem; 
             } 
             synchronized (ZipFile.this) { 
+                if (rem == 0) { 
+                    return -1; 
+                } 
                 ensureOpenOrZipException(); 
 
                 len = ZipFile.read(ZipFile.this.jzfile, jzentry, pos, b, 
Comments
Not veririfed. Used regtest attached. Regrest passes 8u20 b23, 8u20 b09, 8u20 b01 on Windows x64. The test also passes before the fix.
23-07-2014

need check "jzentry == 0" inside the synchronized block
27-03-2014