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.
