Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8038491

Improve synchronization in ZipFile.read()

    Details

    • Subcomponent:
    • Resolved In Build:
      b13
    • Verification:
      Not verified

      Backports

        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,

          Attachments

            Issue Links

              Activity

                People

                • Assignee:
                  coffeys Sean Coffey
                  Reporter:
                  asaha Abhijit Saha
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  4 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved: