The proposed fix does solve the issue.
However it appears it does not fix the root cause that triggers this problem.
The root cause, or the direct trigger of this bug is the fact that
ZipInputStream
.availble() is really NOT reliable to tell us whether or not there is any byte
"available" to be read from the input stream. The api doc of the ZIS.available()
specifies "Returns 0 after EOF has reached for the current entry data, otherwise
always return 1.". The implementation is straightforward,
public int available() throws IOException {
if (entryEOF) {
return 0;
} else {
return 1;
}
}
However, the flag "entryEOF" is only set if a read attempt has been tried
and failed (see ZIS.read()), which means if a previous read of the entry
succeeded and it actually read all the available bytes from the entry (with
a big enough buffer), there is 0 byte available for read, the "flag" is not
set, so if you invoke zis.available(), it return 1, but a read() invocation will
returns -1 (yes, if you then try zis.available() again , it returns the expected
0). The test below demonstrates this issue.
public static void main(String args[]) throws IOException {
ZipInputStream zis = new ZipInputStream(new FileInputStream(args[0]));
ZipEntry ze;
while((ze = zis.getNextEntry()) !=null) {
System.out.println("--------------" + ze.getName() + "--------");
byte[] buf = new byte[8102];
while (true) {
System.out.println(" zis.available()=" + zis.available());
int n = zis.read(buf, 0, buf.length);
System.out.println(" readN=" + n);
if (n == -1)
break;
}
}
}
It is arguable that the this is not an implementation bug, since it is
reasonable
to argue that the EOF has not been "really" reached yet after the first try, the
implementation happens to return all available bytes, so the next read try
finally
"reached" the EOF, but this obviously is not the expectation.
Unfortunately the fix we put in for #4691425 [1] logically depends on
in.available() to decide whether or not we need to read a little further, which
directly triggers this bug when the ZIS.available() "lies".
// If there are more bytes available in "in" or
// the leftover in the "inf" is > 26 bytes:
// this.trailer(8) + next.header.min(10) + next.trailer(8)
// try concatenated case
if (this.in.available() > 0 || n > 26) {
....
}
Not only ZInputStream has this issue, its parent class InflaterStream (and
its sibling GZIPInputStream, GZS inherits InflaterStream's available()
implementation directly) has the same issue, I do have a bug#7031075
filed against GZIPInputStream.
So the proposed fix is more a workaround for this available() issue. The
alternative is to fix the issue directly, for example, to change the ZIS's
available implementation to something like
public int available() throws IOException {
ensureOpen();
if (entryEOF || entry == null)
return 0;
switch (entry.method) {
case DEFLATED:
return (inf.finished() || inf.needsDictionary()) ? 0 : 1;
case STORED:
return remaining > 0 ? 1 : 0;
default:
throw new ZipException("invalid compression method");
}
}
we probably should go further to simply remove the flag "entryEOF"
and move the "deflated" case implementation into InflaterInputStream
(to fix 7031075 as well).
-Sherman
[1] http://cr.openjdk.java.net/~sherman/4691425/webrev/
[2] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7031075
On 05/10/2013 01:03 AM, Ivan Gerasimov wrote:
Hello everybody!
GzipInputStream uses SequenceInputStream to concatenate the underlying stream
with some data that reside in memory.
SequenceInputStream is implementing in such a way that it closes the stream
when it reaches EOS.
The solution is to wrap the underlying stream with extended FilterInputStream
that overrides the close() method.
BUG: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7021870
WEBREV: http://cr.openjdk.java.net/~dmeetry/7021870/webrev.0/
<http://cr.openjdk.java.net/%7Edmeetry/7021870/webrev.0/>
Sincerely your,
Ivan
