Fixing available() and close() for archive streams.

This builds on work originally submitted to Harmony:
  http://issues.apache.org/jira/browse/HARMONY-6210
The approach is to change available() to eagerly set eof to true,
rather than waiting for a read to fail.

8 files changed, 194 insertions, 133 deletions

diff --git a/archive/src/main/java/java/util/zip/GZIPInputStream.java b/archive/src/main/java/java/util/zip/GZIPInputStream.java
index bb84f5b..cc7a019 100644
--- a/archive/src/main/java/java/util/zip/GZIPInputStream.java
+++ b/archive/src/main/java/java/util/zip/GZIPInputStream.java
@@ -161,38 +161,54 @@ public class GZIPInputStream extends InflaterInputStream {
         if (closed) {
             throw new IOException(Messages.getString("archive.1E")); //$NON-NLS-1$
         }
-        if (eof) {
+        // BEGIN android-changed
+        if (eos) {
             return -1;
         }
         // avoid int overflow, check null buffer
-        if (off <= buffer.length && nbytes >= 0 && off >= 0
-                && buffer.length - off >= nbytes) {
-            int val = super.read(buffer, off, nbytes);
-            if (val != -1) {
-                crc.update(buffer, off, val);
-            } else if (!eos) {
-                eos = true;
-                // Get non-compressed bytes read by fill
-                int size = inf.getRemaining();
-                final int trailerSize = 8; // crc (4 bytes) + total out (4
-                // bytes)
-                byte[] b = new byte[trailerSize];
-                int copySize = (size > trailerSize) ? trailerSize : size;
-
-                System.arraycopy(buf, len - size, b, 0, copySize);
-                readFully(b, copySize, trailerSize - copySize);
-
-                if (getLong(b, 0) != crc.getValue()) {
-                    throw new IOException(Messages.getString("archive.20")); //$NON-NLS-1$
-                }
-                if ((int) getLong(b, 4) != inf.getTotalOut()) {
-                    throw new IOException(Messages.getString("archive.21")); //$NON-NLS-1$
-                }
-            }
-            return val;
+        if (off > buffer.length || nbytes < 0 || off < 0
+                || buffer.length - off < nbytes) {
+            throw new ArrayIndexOutOfBoundsException();
+        }
+
+        int bytesRead;
+        try {
+            bytesRead = super.read(buffer, off, nbytes);
+        } finally {
+            eos = eof; // update eos after every read(), even when it throws
+        }
+
+        if (bytesRead != -1) {
+            crc.update(buffer, off, bytesRead);
+        }
+
+        if (eos) {
+            verifyCrc();
+        }
+
+        return bytesRead;
+        // END android-changed
+    }
+
+    // BEGIN android-added
+    private void verifyCrc() throws IOException {
+        // Get non-compressed bytes read by fill
+        int size = inf.getRemaining();
+        final int trailerSize = 8; // crc (4 bytes) + total out (4 bytes)
+        byte[] b = new byte[trailerSize];
+        int copySize = (size > trailerSize) ? trailerSize : size;
+
+        System.arraycopy(buf, len - size, b, 0, copySize);
+        readFully(b, copySize, trailerSize - copySize);
+
+        if (getLong(b, 0) != crc.getValue()) {
+            throw new IOException(Messages.getString("archive.20")); //$NON-NLS-1$
+        }
+        if ((int) getLong(b, 4) != inf.getTotalOut()) {
+            throw new IOException(Messages.getString("archive.21")); //$NON-NLS-1$
         }
-        throw new ArrayIndexOutOfBoundsException();
     }
+    // END android-added
 
     private void readFully(byte[] buffer, int offset, int length)
             throws IOException {
diff --git a/archive/src/main/java/java/util/zip/InflaterInputStream.java b/archive/src/main/java/java/util/zip/InflaterInputStream.java
index 1fd3602..8a7c86b 100644
--- a/archive/src/main/java/java/util/zip/InflaterInputStream.java
+++ b/archive/src/main/java/java/util/zip/InflaterInputStream.java
@@ -53,6 +53,11 @@ public class InflaterInputStream extends FilterInputStream {
 
     boolean closed;
 
+    /**
+     * True if this stream's last byte has been returned to the user. This
+     * could be because the underlying stream has been exhausted, or if errors
+     * were encountered while inflating that stream.
+     */
     boolean eof;
 
     static final int BUF_SIZE = 512;
@@ -165,41 +170,47 @@ public class InflaterInputStream extends FilterInputStream {
             return 0;
         }
 
-        if (inf.finished()) {
-            eof = true;
+        // BEGIN android-changed
+        if (eof) {
             return -1;
         }
 
         // avoid int overflow, check null buffer
-        if (off <= buffer.length && nbytes >= 0 && off >= 0
-                && buffer.length - off >= nbytes) {
-            do {
-                if (inf.needsInput()) {
-                    fill();
-                }
-                int result;
-                try {
-                    result = inf.inflate(buffer, off, nbytes);
-                } catch (DataFormatException e) {
-                    if (len == -1) {
-                        throw new EOFException();
-                    }
-                    throw (IOException) (new IOException().initCause(e));
-                }
+        if (off > buffer.length || nbytes < 0 || off < 0
+                || buffer.length - off < nbytes) {
+            throw new ArrayIndexOutOfBoundsException();
+        }
+
+        do {
+            if (inf.needsInput()) {
+                fill();
+            }
+            // Invariant: if reading returns -1 or throws, eof must be true.
+            // It may also be true if the next read() should return -1.
+            try {
+                int result = inf.inflate(buffer, off, nbytes);
+                eof = inf.finished();
                 if (result > 0) {
                     return result;
-                } else if (inf.finished()) {
-                    eof = true;
+                } else if (eof) {
                     return -1;
                 } else if (inf.needsDictionary()) {
+                    eof = true;
                     return -1;
                 } else if (len == -1) {
+                    eof = true;
                     throw new EOFException();
                     // If result == 0, fill() and try again
                 }
-            } while (true);
-        }
-        throw new ArrayIndexOutOfBoundsException();
+            } catch (DataFormatException e) {
+                eof = true;
+                if (len == -1) {
+                    throw new EOFException();
+                }
+                throw (IOException) (new IOException().initCause(e));
+            }
+        } while (true);
+        // END android-changed
     }
 
     /**
@@ -252,7 +263,9 @@ public class InflaterInputStream extends FilterInputStream {
                         (rem = nbytes - count) > buf.length ? buf.length
                                 : (int) rem);
                 if (x == -1) {
-                    eof = true;
+                    // BEGIN android-removed
+                    // eof = true;
+                    // END android-removed
                     return count;
                 }
                 count += x;
@@ -263,9 +276,18 @@ public class InflaterInputStream extends FilterInputStream {
     }
 
     /**
-     * Returns whether data can be read from this stream.
+     * Returns 0 when when this stream has exhausted its input; and 1 otherwise.
+     * A result of 1 does not guarantee that further bytes can be returned,
+     * with or without blocking.
+     *
+     * <p>Although consistent with the RI, this behavior is inconsistent with
+     * {@link InputStream#available()}, and violates the <a
+     * href="http://en.wikipedia.org/wiki/Liskov_substitution_principle">Liskov
+     * Substitution Principle</a>. This method should not be used.
      *
-     * @return 0 if this stream has been closed, 1 otherwise.
+     * @return 0 if no further bytes are available. Otherwise returns 1,
+     *         which suggests (but does not guarantee) that additional bytes are
+     *         available.
      * @throws IOException
      *             If an error occurs.
      */
diff --git a/archive/src/main/java/java/util/zip/ZipInputStream.java b/archive/src/main/java/java/util/zip/ZipInputStream.java
index fd78e4c..f86cbe0 100644
--- a/archive/src/main/java/java/util/zip/ZipInputStream.java
+++ b/archive/src/main/java/java/util/zip/ZipInputStream.java
@@ -292,9 +292,6 @@ public class ZipInputStream extends InflaterInputStream implements ZipConstants
             }
             currentEntry.setExtra(e);
         }
-        // BEGIN android-added
-        eof = false;
-        // END android-added
         return currentEntry;
     }
 
@@ -318,62 +315,56 @@ public class ZipInputStream extends InflaterInputStream implements ZipConstants
         if (closed) {
             throw new IOException(Messages.getString("archive.1E")); //$NON-NLS-1$
         }
-        // END android-changed
         if (inf.finished() || currentEntry == null) {
             return -1;
         }
         // avoid int overflow, check null buffer
-        if (start <= buffer.length && length >= 0 && start >= 0
-                && buffer.length - start >= length) {
-            if (currentEntry.compressionMethod == STORED) {
-                int csize = (int) currentEntry.size;
-                if (inRead >= csize) {
-                    // BEGIN android-added
+        if (start > buffer.length || length < 0 || start < 0
+                || buffer.length - start < length) {
+            throw new ArrayIndexOutOfBoundsException();
+        }
+
+        if (currentEntry.compressionMethod == STORED) {
+            int csize = (int) currentEntry.size;
+            if (inRead >= csize) {
+                return -1;
+            }
+            if (lastRead >= len) {
+                lastRead = 0;
+                if ((len = in.read(buf)) == -1) {
                     eof = true;
-                    // END android-added
                     return -1;
                 }
-                if (lastRead >= len) {
-                    lastRead = 0;
-                    if ((len = in.read(buf)) == -1) {
-                        // BEGIN android-added
-                        eof = true;
-                        // END android-added
-                        return -1;
-                    }
-                    entryIn += len;
-                }
-                // BEGIN android-changed
-                int toRead = length > (len - lastRead) ? len - lastRead : length;
-                // END android-changed
-                if ((csize - inRead) < toRead) {
-                    toRead = csize - inRead;
-                }
-                System.arraycopy(buf, lastRead, buffer, start, toRead);
-                lastRead += toRead;
-                inRead += toRead;
-                crc.update(buffer, start, toRead);
-                return toRead;
-            }
-            if (inf.needsInput()) {
-                fill();
-                if (len > 0) {
-                    entryIn += len;
-                }
+                entryIn += len;
             }
-            int read = 0;
-            try {
-                read = inf.inflate(buffer, start, length);
-            } catch (DataFormatException e) {
-                throw new ZipException(e.getMessage());
+            int toRead = length > (len - lastRead) ? len - lastRead : length;
+            if ((csize - inRead) < toRead) {
+                toRead = csize - inRead;
             }
-            if (read == 0 && inf.finished()) {
-                return -1;
+            System.arraycopy(buf, lastRead, buffer, start, toRead);
+            lastRead += toRead;
+            inRead += toRead;
+            crc.update(buffer, start, toRead);
+            return toRead;
+        }
+        if (inf.needsInput()) {
+            fill();
+            if (len > 0) {
+                entryIn += len;
             }
-            crc.update(buffer, start, read);
-            return read;
         }
-        throw new ArrayIndexOutOfBoundsException();
+        int read;
+        try {
+            read = inf.inflate(buffer, start, length);
+        } catch (DataFormatException e) {
+            throw new ZipException(e.getMessage());
+        }
+        if (read == 0 && inf.finished()) {
+            return -1;
+        }
+        crc.update(buffer, start, read);
+        return read;
+        // END android-changed
     }
 
     /**
@@ -416,10 +407,7 @@ public class ZipInputStream extends InflaterInputStream implements ZipConstants
         if (closed) {
             throw new IOException(Messages.getString("archive.1E")); //$NON-NLS-1$
         }
-        if (currentEntry == null) {
-            return 1;
-        }
-        return super.available();
+        return (currentEntry == null || inRead < currentEntry.size) ? 1 : 0;
         // END android-changed
     }
 
diff --git a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/jar/JarInputStreamTest.java b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/jar/JarInputStreamTest.java
index 151eabc..5befa77 100644
--- a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/jar/JarInputStreamTest.java
+++ b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/jar/JarInputStreamTest.java
@@ -154,12 +154,12 @@ public class JarInputStreamTest extends junit.framework.TestCase {
         assertEquals(actual, desired);
         jis.close();
 
-//        try {
-//            jis.getNextJarEntry(); //Android implementation does not throw exception
-//            fail("IOException expected");
-//        } catch (IOException ee) {
-//            // expected
-//        }
+        try {
+            jis.getNextJarEntry();
+            fail("IOException expected");
+        } catch (IOException ee) {
+            // expected
+        }
 
         File resources = Support_Resources.createTempFolder();
         Support_Resources.copyFile(resources, null, "Broken_entry.jar");
@@ -181,8 +181,7 @@ public class JarInputStreamTest extends junit.framework.TestCase {
     )
     public void test_getNextJarEntry_Ex() throws Exception {
         final Set<String> desired = new HashSet<String>(Arrays
-                .asList(new String[] {
-                        "foo/", "foo/bar/", "foo/bar/A.class", "Blah.txt"}));
+                .asList("foo/", "foo/bar/", "foo/bar/A.class", "Blah.txt"));
         Set<String> actual = new HashSet<String>();
         InputStream is = new URL(jarName).openConnection().getInputStream();
         JarInputStream jis = new JarInputStream(is);
@@ -195,7 +194,7 @@ public class JarInputStreamTest extends junit.framework.TestCase {
         jis.close();
 
         try {
-            jis.getNextJarEntry(); //Android implementation does not throw exception
+            jis.getNextJarEntry();
             fail("IOException expected");
         } catch (IOException ee) {
             // expected
@@ -275,9 +274,9 @@ public class JarInputStreamTest extends junit.framework.TestCase {
     )
     public void test_JarInputStream_Modified_Manifest_MainAttributes_getNextEntry()
             throws IOException {
-        String modJarName = Support_Resources
-                .getURL("Modified_Manifest_MainAttributes.jar");
-        InputStream is = new URL(modJarName).openConnection().getInputStream();
+        String modJarName = Support_Resources.getURL("Modified_Manifest_MainAttributes.jar");
+        InputStream is = new URL(modJarName).openConnection()
+                .getInputStream();
         JarInputStream jin = new JarInputStream(is, true);
 
         assertEquals("META-INF/TESTROOT.SF", jin.getNextEntry().getName());
diff --git a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/DeflaterOutputStreamTest.java b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/DeflaterOutputStreamTest.java
index ed7238c..738f610 100644
--- a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/DeflaterOutputStreamTest.java
+++ b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/DeflaterOutputStreamTest.java
@@ -211,21 +211,22 @@ public class DeflaterOutputStreamTest extends TestCase {
     )
     public void test_close() throws Exception {
         File f1 = File.createTempFile("close", ".tst");
-        FileOutputStream fos = new FileOutputStream(f1);
-        DeflaterOutputStream dos = new DeflaterOutputStream(fos);
-        byte byteArray[] = {1, 3, 4, 6};
-        dos.write(byteArray);
 
-        FileInputStream fis = new FileInputStream(f1);
-        InflaterInputStream iis = new InflaterInputStream(fis);
+        InflaterInputStream iis = new InflaterInputStream(new FileInputStream(f1));
         try {
             iis.read();
             fail("EOFException Not Thrown");
         } catch (EOFException e) {
         }
 
+        FileOutputStream fos = new FileOutputStream(f1);
+        DeflaterOutputStream dos = new DeflaterOutputStream(fos);
+        byte byteArray[] = {1, 3, 4, 6};
+        dos.write(byteArray);
         dos.close();
 
+        iis = new InflaterInputStream(new FileInputStream(f1));
+
         // Test to see if the finish method wrote the bytes to the file.
         assertEquals("Incorrect Byte Returned.", 1, iis.read());
         assertEquals("Incorrect Byte Returned.", 3, iis.read());
diff --git a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/InflaterInputStreamTest.java b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/InflaterInputStreamTest.java
index 2de996e..707f13b 100644
--- a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/InflaterInputStreamTest.java
+++ b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/InflaterInputStreamTest.java
@@ -32,6 +32,8 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.FileOutputStream;
+import java.io.EOFException;
 import java.util.zip.DeflaterOutputStream;
 import java.util.zip.Inflater;
 import java.util.zip.InflaterInputStream;
@@ -208,6 +210,42 @@ public class InflaterInputStreamTest extends TestCase {
         }
     }
 
+    public void testAvailableNonEmptySource() throws Exception {
+        // this byte[] is a deflation of these bytes: { 1, 3, 4, 6 }
+        byte[] deflated = { 72, -119, 99, 100, 102, 97, 3, 0, 0, 31, 0, 15, 0 };
+        InputStream in = new InflaterInputStream(new ByteArrayInputStream(deflated));
+        // InflaterInputStream.available() returns either 1 or 0, even though
+        // that contradicts the behavior defined in InputStream.available()
+        assertEquals(1, in.read());
+        assertEquals(1, in.available());
+        assertEquals(3, in.read());
+        assertEquals(1, in.available());
+        assertEquals(4, in.read());
+        assertEquals(1, in.available());
+        assertEquals(6, in.read());
+        assertEquals(0, in.available());
+        assertEquals(-1, in.read());
+        assertEquals(-1, in.read());
+    }
+
+    public void testAvailableSkip() throws Exception {
+        // this byte[] is a deflation of these bytes: { 1, 3, 4, 6 }
+        byte[] deflated = { 72, -119, 99, 100, 102, 97, 3, 0, 0, 31, 0, 15, 0 };
+        InputStream in = new InflaterInputStream(new ByteArrayInputStream(deflated));
+        assertEquals(1, in.available());
+        assertEquals(4, in.skip(4));
+        assertEquals(0, in.available());
+    }
+
+    public void testAvailableEmptySource() throws Exception {
+        // this byte[] is a deflation of the empty file
+        byte[] deflated = { 120, -100, 3, 0, 0, 0, 0, 1 };
+        InputStream in = new InflaterInputStream(new ByteArrayInputStream(deflated));
+        assertEquals(-1, in.read());
+        assertEquals(-1, in.read());
+        assertEquals(0, in.available());
+    }
+
     /**
      * @tests java.util.zip.InflaterInputStream#read(byte[], int, int)
      */
@@ -471,12 +509,11 @@ public class InflaterInputStreamTest extends TestCase {
         InflaterInputStream iis = new InflaterInputStream(is);
 
         int available;
-        int read;
         for (int i = 0; i < 11; i++) {
-            read = iis.read();
+            iis.read();
             available = iis.available();
-            if (read == -1) {
-                assertEquals("Bytes Available Should Return 0 ", 0, available);
+            if (available == 0) {
+                assertEquals("Expected no more bytes to read", -1, iis.read());
             } else {
                 assertEquals("Bytes Available Should Return 1.", 1, available);
             }
diff --git a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/ZipFileTest.java b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/ZipFileTest.java
index c9e7bb8..b025e11 100644
--- a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/ZipFileTest.java
+++ b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/ZipFileTest.java
@@ -18,7 +18,6 @@
 package org.apache.harmony.archive.tests.java.util.zip;
 
 import dalvik.annotation.KnownFailure;
-import dalvik.annotation.BrokenTest;
 import dalvik.annotation.TestLevel;
 import dalvik.annotation.TestTargetClass;
 import dalvik.annotation.TestTargetNew;
diff --git a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/ZipInputStreamTest.java b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/ZipInputStreamTest.java
index 1d6c339..c5efedc 100644
--- a/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/ZipInputStreamTest.java
+++ b/archive/src/test/java/org/apache/harmony/archive/tests/java/util/zip/ZipInputStreamTest.java
@@ -17,7 +17,6 @@
 
 package org.apache.harmony.archive.tests.java.util.zip;
 
-import dalvik.annotation.KnownFailure;
 import dalvik.annotation.TestLevel;
 import dalvik.annotation.TestTargetClass;
 import dalvik.annotation.TestTargetNew;
@@ -384,19 +383,19 @@ public class ZipInputStreamTest extends TestCase {
         long entrySize = entry.getSize();
         assertTrue("Entry size was < 1", entrySize > 0);
         int i = 0;
-        for (i = 0; i < entrySize; i++) {
+        while (zis1.available() > 0) {
             zis1.skip(1);
-            if (zis1.available() == 0) break;
+            i++;
         }
         if (i != entrySize) {
             fail("ZipInputStream.available or ZipInputStream.skip does not " +
                     "working properly. Only skipped " + i +
                     " bytes instead of " + entrySize);
         }
-        zis1.skip(1);
-        assertTrue(zis1.available() == 0);
+        assertEquals(0, zis1.skip(1));
+        assertEquals(0, zis1.available());
         zis1.closeEntry();
-        assertFalse(zis.available() == 0);
+        assertEquals(1, zis.available());
         zis1.close();
         try {
             zis1.available();