RFR : 8038491: Improve synchronization in ZipFile.read()

[Chris Hegarty]

Approved. Looks good to me.

-Chris

On 09/04/14 18:39, Seán Coffey wrote:
> On re-read, I believe extending the sync block in read(..) to cover the
> reading and setting of the rem variable works to resolve this fix. It
> should preserve behaviour as well.
>
> http://cr.openjdk.java.net/~coffeys/webrev.8038491.v2/webrev/
>
> regards,
> Sean.
>
> On 08/04/14 21:28, Seán Coffey wrote:
>> Chris,
>>
>> ZipFileInputStream.skip(..) can also close out the stream and free up
>> the underlying jzentry resources.
>>
>> Per Sherman's suggestion I substituted rem for jzentry == 0 check but
>> ran into issues with other tests [1]
>> Another simple change (and to preserve old behaviour) might be just to
>> extend the synchronized block to start at top of the read method and
>> to check for both (rem == 0 || jzentry == 0) [2]
>>
>> tests running.
>>
>> regards,
>> Sean.
>>
>> [1]
>>
>>> java.util.zip.ZipException: ZIP_Read: specified offset out of range
>>>     at java.util.zip.ZipFile.read(Native Method)
>>>     at java.util.zip.ZipFile.access$1400(ZipFile.java:61)
>>>     at java.util.zip.ZipFile$ZipFileInputStream.read(ZipFile.java:715)
>>>     at java.io.InputStream.read(InputStream.java:101)
>>>     at
>>> com.sun.java.util.jar.pack.Package$File.readFrom(Package.java:849)
>>>     at
>>> com.sun.java.util.jar.pack.PackerImpl$DoPack.readFile(PackerImpl.java:517)
>>>
>>>     at
>>> com.sun.java.util.jar.pack.PackerImpl$DoPack.run(PackerImpl.java:466)
>>>     at com.sun.java.util.jar.pack.PackerImpl.pack(PackerImpl.java:97)
>>>     at sun.tools.jar.Main.run(Main.java:228)
>>>     at sun.tools.jar.Main.main(Main.java:1233)
>>> Exception in thread "main" java.util.zip.ZipException: ZIP_Read:
>>> specified offset out of range
>>>     at java.util.zip.ZipFile.read(Native Method)
>>>     at java.util.zip.ZipFile.access$1400(ZipFile.java:61)
>>>     at java.util.zip.ZipFile$ZipFileInputStream.read(ZipFile.java:715)
>>>     at java.io.InputStream.read(InputStream.java:101)
>>>     at
>>> com.sun.java.util.jar.pack.Package$File.readFrom(Package.java:849)
>>>     at
>>> com.sun.java.util.jar.pack.PackerImpl$DoPack.readFile(PackerImpl.java:517)
>>>
>>>     at
>>> com.sun.java.util.jar.pack.PackerImpl$DoPack.run(PackerImpl.java:466)
>>>     at com.sun.java.util.jar.pack.PackerImpl.pack(PackerImpl.java:97)
>>>     at com.sun.java.util.jar.pack.Driver.main(Driver.java:313)
>>> java.util.zip.ZipException: zip file is empty
>>>     at java.util.zip.ZipFile.open(Native Method)
>>>     at java.util.zip.ZipFile.<init>(ZipFile.java:220)
>>>     at java.util.zip.ZipFile.<init>(ZipFile.java:150)
>>>     at java.util.jar.JarFile.<init>(JarFile.java:166)
>>>     at java.util.jar.JarFile.<init>(JarFile.java:103)
>>>     at TestNormal.main(TestNormal.java:59)
>>>     at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>>     at
>>> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>>>
>>>     at
>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>>
>>>     at java.lang.reflect.Method.invoke(Method.java:484)
>>>     at
>>> com.sun.javatest.regtest.MainAction$SameVMRunnable.run(MainAction.java:754)
>>>
>>>     at java.lang.Thread.run(Thread.java:744)
>>
>> [2]
>>> diff --git a/src/share/classes/java/util/zip/ZipFile.java
>>> b/src/share/classes/java/util/zip/ZipFile.java
>>> --- a/src/share/classes/java/util/zip/ZipFile.java
>>> +++ b/src/share/classes/java/util/zip/ZipFile.java
>>> @@ -700,7 +700,8 @@
>>>          }
>>>
>>>          public int read(byte b[], int off, int len) throws
>>> IOException {
>>> -            if (rem == 0) {
>>> +            synchronized (ZipFile.this) {
>>> +                if (jzentry == 0 || rem == 0) {
>>>                  return -1;
>>>              }
>>>              if (len <= 0) {
>>> @@ -709,9 +710,8 @@
>>>              if (len > rem) {
>>>                  len = (int) rem;
>>>              }
>>> -            synchronized (ZipFile.this) {
>>> +
>>>                  ensureOpenOrZipException();
>>> -
>>>                  len = ZipFile.read(ZipFile.this.jzfile, jzentry,
>>> pos, b,
>>>                                     off, len);
>>>              }
>>
>>
>> On 08/04/2014 19:52, Chris Hegarty wrote:
>>>> My take is that the performance is not a concern here, the only real
>>>> problem is the SEGV.
>>>> >Given "num" is not a volatile and is not updated under synchronized
>>>> block,  check "num == 0"
>>>> >is not going to make ZFIS work for mult-thread usage. It also makes
>>>> me nervous to check it
>>>> >inside the synchronized block as a global "flag". I'm also
>>>> concerned that the change to check
>>>> >the rem == 0 after the check of "len" may also change the behavior
>>>> of someone's code in
>>>> >certain circumstance…
>>> To make this safe and simple, why not just move the close inside the
>>> synchronized block to ensure no concurrent access before close
>>> completes ( if needed ). There is very little computation overhead
>>> added to the synchronized block, but guarantees serial access to close.
>>>
>>>          synchronized (ZipFile.this) {
>>>                   ensureOpenOrZipException();
>>>                     len = ZipFile.read(ZipFile.this.jzfile, jzentry,
>>> pos, b,
>>>                                      off, len);
>>>                   if (len > 0) {
>>>                       pos += len;
>>>                       rem -= len;
>>>                   }
>>>                   if (rem == 0) {
>>>                       close();
>>>                   }
>>>          }
>>>
>>> -Chris.
>>>
>>
>