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,

          Activity

          Hide
          sherman Xueming Shen added a comment -
          need check "jzentry == 0" inside the synchronized block
          Show
          sherman Xueming Shen added a comment - need check "jzentry == 0" inside the synchronized block
          Hide
          hgupdate HG Updates added a comment -
          URL: http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/834e2159bd2a
          User: coffeys
          Date: 2014-04-24 13:06:23 +0000
          Show
          hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/834e2159bd2a User: coffeys Date: 2014-04-24 13:06:23 +0000
          Hide
          hgupdate HG Updates added a comment -
          URL: http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/834e2159bd2a
          User: lana
          Date: 2014-05-05 17:28:41 +0000
          Show
          hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/834e2159bd2a User: lana Date: 2014-05-05 17:28:41 +0000
          Hide
          kshefov Konstantin Shefov added a comment -
          Not veririfed.
          Used regtest attached. Regrest passes 8u20 b23, 8u20 b09, 8u20 b01 on Windows x64. The test also passes before the fix.
          Show
          kshefov Konstantin Shefov added a comment - Not veririfed. Used regtest attached. Regrest passes 8u20 b23, 8u20 b09, 8u20 b01 on Windows x64. The test also passes before the fix.

            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: