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

[Seán Coffey]

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