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

[Seán Coffey]

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