-
Bug
-
Resolution: Fixed
-
P2
-
8
-
b13
-
Not verified
Issue | Fix Version | Assignee | Priority | Status | Resolution | Resolved In Build |
---|---|---|---|---|---|---|
JDK-8039958 | 9 | Sean Coffey | P2 | Closed | Fixed | b08 |
JDK-8045740 | 8u25 | Sean Coffey | P2 | Resolved | Fixed | b01 |
JDK-8053569 | emb-8u26 | Sean Coffey | P2 | Resolved | Fixed | b17 |
JDK-8136634 | 7u95 | Sean Coffey | P2 | Closed | Fixed | b01 |
JDK-8136692 | 6u111 | Sean Coffey | P2 | Closed | Fixed | b01 |
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,
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,
- backported by
-
JDK-8045740 Improve synchronization in ZipFile.read()
- Resolved
-
JDK-8053569 Improve synchronization in ZipFile.read()
- Resolved
-
JDK-8039958 Improve synchronization in ZipFile.read()
- Closed
-
JDK-8136634 Improve synchronization in ZipFile.read()
- Closed
-
JDK-8136692 Improve synchronization in ZipFile.read()
- Closed