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

Seán Coffey sean.coffey at oracle.com
Wed Apr 9 17:39:03 UTC 2014

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: ZIPRead: 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: ZIPRead: 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.(ZipFile.java:220) at java.util.zip.ZipFile.(ZipFile.java:150) at java.util.jar.JarFile.(JarFile.java:166) at java.util.jar.JarFile.(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.

More information about the core-libs-dev mailing list