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,
|