Clean up ZipFile a little, fix a few bugs, and improve the documentation.

The main bug fixed is fixing support for .zip files larger than 2GiB (where
the central directory offset's top bit is set).

We were checking character counts rather than byte counts in several places
too, which could lead to corrupt zip files.

I've also added a comment to readCentralDirectory because I keep coming back
to this code, wanting to make it lazy, and never remember (until I've done
half the work) why that's not possible.

I've also clarified a lot of the documentation.

Bug: http://code.google.com/p/android/issues/detail?id=36187
Change-Id: Iaa8eadc501ead7c70528bd9063d5893a325dcea1

Conflicts:
	luni/src/main/java/java/util/zip/ZipFile.java

6 files changed, 471 insertions, 245 deletions

diff --git a/luni/src/main/java/java/util/zip/InflaterInputStream.java b/luni/src/main/java/java/util/zip/InflaterInputStream.java
index 397637e..371c80a 100644
--- a/luni/src/main/java/java/util/zip/InflaterInputStream.java
+++ b/luni/src/main/java/java/util/zip/InflaterInputStream.java
@@ -189,13 +189,8 @@ public class InflaterInputStream extends FilterInputStream {
     protected void fill() throws IOException {
         checkClosed();
         if (nativeEndBufSize > 0) {
-            ZipFile.RAFStream is = (ZipFile.RAFStream)in;
-            synchronized (is.mSharedRaf) {
-                long len = is.mLength - is.mOffset;
-                if (len > nativeEndBufSize) len = nativeEndBufSize;
-                int cnt = inf.setFileInput(is.mSharedRaf.getFD(), is.mOffset, nativeEndBufSize);
-                is.skip(cnt);
-            }
+            ZipFile.RAFStream is = (ZipFile.RAFStream) in;
+            len = is.fill(inf, nativeEndBufSize);
         } else {
             if ((len = in.read(buf)) > 0) {
                 inf.setInput(buf, 0, len);
diff --git a/luni/src/main/java/java/util/zip/ZipEntry.java b/luni/src/main/java/java/util/zip/ZipEntry.java
index 3e58727..c313666 100644
--- a/luni/src/main/java/java/util/zip/ZipEntry.java
+++ b/luni/src/main/java/java/util/zip/ZipEntry.java
@@ -40,11 +40,17 @@ import libcore.io.HeapBufferIterator;
  * @see ZipOutputStream
  */
 public class ZipEntry implements ZipConstants, Cloneable {
-    String name, comment;
+    String name;
+    String comment;
 
-    long compressedSize = -1, crc = -1, size = -1;
+    long crc = -1; // Needs to be a long to distinguish -1 ("not set") from the 0xffffffff CRC32.
 
-    int compressionMethod = -1, time = -1, modDate = -1;
+    long compressedSize = -1;
+    long size = -1;
+
+    int compressionMethod = -1;
+    int time = -1;
+    int modDate = -1;
 
     byte[] extra;
 
@@ -80,11 +86,8 @@ public class ZipEntry implements ZipConstants, Cloneable {
     }
 
     /**
-     * Gets the comment for this {@code ZipEntry}.
-     *
-     * @return the comment for this {@code ZipEntry}, or {@code null} if there
-     *         is no comment. If we're reading an archive with
-     *         {@code ZipInputStream} the comment is not available.
+     * Returns the comment for this {@code ZipEntry}, or {@code null} if there is no comment.
+     * If we're reading an archive with {@code ZipInputStream} the comment is not available.
      */
     public String getComment() {
         return comment;
@@ -178,13 +181,17 @@ public class ZipEntry implements ZipConstants, Cloneable {
 
     /**
      * Sets the comment for this {@code ZipEntry}.
-     *
-     * @param comment
-     *            the comment for this entry.
+     * @throws IllegalArgumentException if the comment is longer than 64 KiB.
      */
     public void setComment(String comment) {
-        if (comment != null && comment.length() > 0xffff) {
-            throw new IllegalArgumentException("Comment too long: " + comment.length());
+        if (comment == null) {
+            this.comment = null;
+            return;
+        }
+
+        byte[] commentBytes = comment.getBytes(Charsets.UTF_8);
+        if (commentBytes.length > 0xffff) {
+            throw new IllegalArgumentException("Comment too long: " + commentBytes.length);
         }
         this.comment = comment;
     }
@@ -221,7 +228,7 @@ public class ZipEntry implements ZipConstants, Cloneable {
      * @param data
      *            a byte array containing the extra information.
      * @throws IllegalArgumentException
-     *             when the length of data is greater than 0xFFFF bytes.
+     *             when the length of data is greater than 64 KiB.
      */
     public void setExtra(byte[] data) {
         if (data != null && data.length > 0xffff) {
@@ -231,11 +238,12 @@ public class ZipEntry implements ZipConstants, Cloneable {
     }
 
     /**
-     * Sets the compression method for this {@code ZipEntry}.
-     *
-     * @param value
-     *            the compression method, either {@code DEFLATED} or {@code
-     *            STORED}.
+     * Sets the compression method for this entry to either {@code DEFLATED} or {@code STORED}.
+     * The default is {@code DEFLATED}, which will cause the size, compressed size, and CRC to be
+     * set automatically, and the entry's data to be compressed. If you switch to {@code STORED}
+     * note that you'll have to set the size (or compressed size; they must be the same, but it's
+     * okay to only set one) and CRC yourself because they must appear <i>before</i> the user data
+     * in the resulting zip file. See {@link #setSize} and {@link #setCrc}.
      * @throws IllegalArgumentException
      *             when value is not {@code DEFLATED} or {@code STORED}.
      */
@@ -369,7 +377,7 @@ public class ZipEntry implements ZipConstants, Cloneable {
 
         nameLength = it.readShort();
         int extraLength = it.readShort();
-        int commentLength = it.readShort();
+        int commentByteCount = it.readShort();
 
         // This is a 32-bit value in the file, but a 64-bit field in this object.
         it.seek(42);
@@ -381,9 +389,9 @@ public class ZipEntry implements ZipConstants, Cloneable {
 
         // The RI has always assumed UTF-8. (If GPBF_UTF8_FLAG isn't set, the encoding is
         // actually IBM-437.)
-        if (commentLength > 0) {
-            byte[] commentBytes = new byte[commentLength];
-            Streams.readFully(in, commentBytes, 0, commentLength);
+        if (commentByteCount > 0) {
+            byte[] commentBytes = new byte[commentByteCount];
+            Streams.readFully(in, commentBytes, 0, commentByteCount);
             comment = new String(commentBytes, 0, commentBytes.length, Charsets.UTF_8);
         }
 
diff --git a/luni/src/main/java/java/util/zip/ZipFile.java b/luni/src/main/java/java/util/zip/ZipFile.java
index 83b1992..6e8b516 100644
--- a/luni/src/main/java/java/util/zip/ZipFile.java
+++ b/luni/src/main/java/java/util/zip/ZipFile.java
@@ -34,18 +34,17 @@ import libcore.io.HeapBufferIterator;
 import libcore.io.Streams;
 
 /**
- * This class provides random read access to a <i>ZIP-archive</i> file.
- * <p>
- * While {@code ZipInputStream} provides stream based read access to a
- * <i>ZIP-archive</i>, this class implements more efficient (file based) access
- * and makes use of the <i>central directory</i> within a <i>ZIP-archive</i>.
- * <p>
- * Use {@code ZipOutputStream} if you want to create an archive.
- * <p>
- * A temporary ZIP file can be marked for automatic deletion upon closing it.
+ * This class provides random read access to a <i>ZIP-archive</i> file. You pay more to read
+ * the zip file's central directory up front (from the constructor), but if you're using
+ * {@link #getEntry} to look up multiple files by name, you get the benefit of this index.
  *
- * @see ZipEntry
- * @see ZipOutputStream
+ * <p>If you only want to iterate through all the files (using {@link #entries}, you should
+ * consider {@link ZipInputStream}, which provides stream-like read access to a zip file and
+ * has a lower up-front cost, and doesn't require an in-memory index. The savings could be
+ * particularly large if your zip file has many entries and you only require a few of them.
+ *
+ * <p>If you want to create a zip file, use {@link ZipOutputStream}. There is no API for updating
+ * an existing zip file.
  */
 public class ZipFile implements ZipConstants {
     /**
@@ -70,7 +69,7 @@ public class ZipFile implements ZipConstants {
     static final int GPBF_UTF8_FLAG = 1 << 11;
 
     /**
-     * Open ZIP file for read.
+     * Open ZIP file for reading.
      */
     public static final int OPEN_READ = 1;
 
@@ -79,9 +78,9 @@ public class ZipFile implements ZipConstants {
      */
     public static final int OPEN_DELETE = 4;
 
-    private final String fileName;
+    private final String mFilename;
 
-    private File fileToDeleteOnClose;
+    private File mFileToDeleteOnClose;
 
     private RandomAccessFile mRaf;
 
@@ -90,61 +89,51 @@ public class ZipFile implements ZipConstants {
     private final CloseGuard guard = CloseGuard.get();
 
     /**
-     * Constructs a new {@code ZipFile} with the specified file.
-     *
-     * @param file
-     *            the file to read from.
-     * @throws ZipException
-     *             if a ZIP error occurs.
-     * @throws IOException
-     *             if an {@code IOException} occurs.
+     * Constructs a new {@code ZipFile} allowing read access to the contents of the given file.
+     * @throws ZipException if a ZIP error occurs.
+     * @throws IOException if an {@code IOException} occurs.
      */
     public ZipFile(File file) throws ZipException, IOException {
         this(file, OPEN_READ);
     }
 
     /**
-     * Opens a file as <i>ZIP-archive</i>. "mode" must be {@code OPEN_READ} or
-     * {@code OPEN_DELETE} . The latter sets the "delete on exit" flag through a
-     * file.
+     * Constructs a new {@code ZipFile} allowing read access to the contents of the given file.
+     * @throws IOException if an IOException occurs.
+     */
+    public ZipFile(String name) throws IOException {
+        this(new File(name), OPEN_READ);
+    }
+
+    /**
+     * Constructs a new {@code ZipFile} allowing access to the given file.
+     * The {@code mode} must be either {@code OPEN_READ} or {@code OPEN_READ|OPEN_DELETE}.
      *
-     * @param file
-     *            the ZIP file to read.
-     * @param mode
-     *            the mode of the file open operation.
-     * @throws IOException
-     *             if an {@code IOException} occurs.
+     * <p>If the {@code OPEN_DELETE} flag is supplied, the file will be deleted at or before the
+     * time that the {@code ZipFile} is closed (the contents will remain accessible until
+     * this {@code ZipFile} is closed); it also calls {@code File.deleteOnExit}.
+     *
+     * @throws IOException if an {@code IOException} occurs.
      */
     public ZipFile(File file, int mode) throws IOException {
-        fileName = file.getPath();
+        mFilename = file.getPath();
         if (mode != OPEN_READ && mode != (OPEN_READ | OPEN_DELETE)) {
             throw new IllegalArgumentException("Bad mode: " + mode);
         }
 
         if ((mode & OPEN_DELETE) != 0) {
-            fileToDeleteOnClose = file; // file.deleteOnExit();
+            mFileToDeleteOnClose = file;
+            mFileToDeleteOnClose.deleteOnExit();
         } else {
-            fileToDeleteOnClose = null;
+            mFileToDeleteOnClose = null;
         }
 
-        mRaf = new RandomAccessFile(fileName, "r");
+        mRaf = new RandomAccessFile(mFilename, "r");
 
         readCentralDir();
         guard.open("close");
     }
 
-    /**
-     * Opens a ZIP archived file.
-     *
-     * @param name
-     *            the name of the ZIP file.
-     * @throws IOException
-     *             if an IOException occurs.
-     */
-    public ZipFile(String name) throws IOException {
-        this(new File(name), OPEN_READ);
-    }
-
     @Override protected void finalize() throws IOException {
         try {
             if (guard != null) {
@@ -174,9 +163,9 @@ public class ZipFile implements ZipConstants {
                 mRaf = null;
                 raf.close();
             }
-            if (fileToDeleteOnClose != null) {
-                fileToDeleteOnClose.delete();
-                fileToDeleteOnClose = null;
+            if (mFileToDeleteOnClose != null) {
+                mFileToDeleteOnClose.delete();
+                mFileToDeleteOnClose = null;
             }
         }
     }
@@ -257,19 +246,19 @@ public class ZipFile implements ZipConstants {
             // position of the entry's local header. At position 28 we find the
             // length of the extra data. In some cases this length differs from
             // the one coming in the central header.
-            RAFStream rafstrm = new RAFStream(raf, entry.mLocalHeaderRelOffset + 28);
-            DataInputStream is = new DataInputStream(rafstrm);
+            RAFStream rafStream = new RAFStream(raf, entry.mLocalHeaderRelOffset + 28);
+            DataInputStream is = new DataInputStream(rafStream);
             int localExtraLenOrWhatever = Short.reverseBytes(is.readShort());
             is.close();
 
             // Skip the name and this "extra" data or whatever it is:
-            rafstrm.skip(entry.nameLength + localExtraLenOrWhatever);
-            rafstrm.mLength = rafstrm.mOffset + entry.compressedSize;
+            rafStream.skip(entry.nameLength + localExtraLenOrWhatever);
+            rafStream.mLength = rafStream.mOffset + entry.compressedSize;
             if (entry.compressionMethod == ZipEntry.DEFLATED) {
                 int bufSize = Math.max(1024, (int)Math.min(entry.getSize(), 65535L));
-                return new ZipInflaterInputStream(rafstrm, new Inflater(true), bufSize, entry);
+                return new ZipInflaterInputStream(rafStream, new Inflater(true), bufSize, entry);
             } else {
-                return rafstrm;
+                return rafStream;
             }
         }
     }
@@ -280,7 +269,7 @@ public class ZipFile implements ZipConstants {
      * @return the file name of this {@code ZipFile}.
      */
     public String getName() {
-        return fileName;
+        return mFilename;
     }
 
     /**
@@ -351,18 +340,21 @@ public class ZipFile implements ZipConstants {
         int numEntries = it.readShort() & 0xffff;
         int totalNumEntries = it.readShort() & 0xffff;
         it.skip(4); // Ignore centralDirSize.
-        int centralDirOffset = it.readInt();
+        long centralDirOffset = ((long) it.readInt()) & 0xffffffffL;
 
         if (numEntries != totalNumEntries || diskNumber != 0 || diskWithCentralDir != 0) {
             throw new ZipException("spanned archives not supported");
         }
 
         // Seek to the first CDE and read all entries.
-        RAFStream rafs = new RAFStream(mRaf, centralDirOffset);
-        BufferedInputStream bin = new BufferedInputStream(rafs, 4096);
+        // We have to do this now (from the constructor) rather than lazily because the
+        // public API doesn't allow us to throw IOException except from the constructor
+        // or from getInputStream.
+        RAFStream rafStream = new RAFStream(mRaf, centralDirOffset);
+        BufferedInputStream bufferedStream = new BufferedInputStream(rafStream, 4096);
         byte[] hdrBuf = new byte[CENHDR]; // Reuse the same buffer for each entry.
         for (int i = 0; i < numEntries; ++i) {
-            ZipEntry newEntry = new ZipEntry(hdrBuf, bin);
+            ZipEntry newEntry = new ZipEntry(hdrBuf, bufferedStream);
             String entryName = newEntry.getName();
             if (mEntries.put(entryName, newEntry) != null) {
                 throw new ZipException("Duplicate entry name: " + entryName);
@@ -379,14 +371,13 @@ public class ZipFile implements ZipConstants {
      * <p>We could support mark/reset, but we don't currently need them.
      */
     static class RAFStream extends InputStream {
+        private final RandomAccessFile mSharedRaf;
+        private long mLength;
+        private long mOffset;
 
-        RandomAccessFile mSharedRaf;
-        long mOffset;
-        long mLength;
-
-        public RAFStream(RandomAccessFile raf, long pos) throws IOException {
+        public RAFStream(RandomAccessFile raf, long offset) throws IOException {
             mSharedRaf = raf;
-            mOffset = pos;
+            mOffset = offset;
             mLength = raf.length();
         }
 
@@ -414,28 +405,34 @@ public class ZipFile implements ZipConstants {
             }
         }
 
-        @Override
-        public long skip(long byteCount) throws IOException {
+        @Override public long skip(long byteCount) throws IOException {
             if (byteCount > mLength - mOffset) {
                 byteCount = mLength - mOffset;
             }
             mOffset += byteCount;
             return byteCount;
         }
+
+        public int fill(Inflater inflater, int nativeEndBufSize) throws IOException {
+            synchronized (mSharedRaf) {
+                int len = Math.min((int) (mLength - mOffset), nativeEndBufSize);
+                int cnt = inflater.setFileInput(mSharedRaf.getFD(), mOffset, nativeEndBufSize);
+                skip(cnt);
+                return len;
+            }
+        }
     }
 
     static class ZipInflaterInputStream extends InflaterInputStream {
-
-        ZipEntry entry;
-        long bytesRead = 0;
+        private final ZipEntry entry;
+        private long bytesRead = 0;
 
         public ZipInflaterInputStream(InputStream is, Inflater inf, int bsize, ZipEntry entry) {
             super(is, inf, bsize);
             this.entry = entry;
         }
 
-        @Override
-        public int read(byte[] buffer, int off, int nbytes) throws IOException {
+        @Override public int read(byte[] buffer, int off, int nbytes) throws IOException {
             int i = super.read(buffer, off, nbytes);
             if (i != -1) {
                 bytesRead += i;
@@ -443,8 +440,7 @@ public class ZipFile implements ZipConstants {
             return i;
         }
 
-        @Override
-        public int available() throws IOException {
+        @Override public int available() throws IOException {
             if (closed) {
                 // Our superclass will throw an exception, but there's a jtreg test that
                 // explicitly checks that the InputStream returned from ZipFile.getInputStream
diff --git a/luni/src/main/java/java/util/zip/ZipInputStream.java b/luni/src/main/java/java/util/zip/ZipInputStream.java
index e7c4566..57d9034 100644
--- a/luni/src/main/java/java/util/zip/ZipInputStream.java
+++ b/luni/src/main/java/java/util/zip/ZipInputStream.java
@@ -34,14 +34,15 @@ import libcore.io.Streams;
  *
  * <p>A ZIP archive is a collection of (possibly) compressed files.
  * When reading from a {@code ZipInputStream}, you retrieve the
- * entry's metadata with {@code getNextEntry} before you can read the userdata.
+ * entry's metadata with {@code getNextEntry} (which returns a {@link ZipEntry}
+ * before you can read the userdata.
  *
  * <p>Although {@code InflaterInputStream} can only read compressed ZIP archive
  * entries, this class can read non-compressed entries as well.
  *
- * <p>Use {@code ZipFile} if you can access the archive as a file directly,
- * especially if you want random access to entries, rather than needing to
- * iterate over all entries.
+ * <p>Use {@link ZipFile} if you need random access to entries by name, but use this class
+ * if you just want to iterate over all entries (and remember that iteration is better
+ * than lookup by name in the case where you're only looking for one file).
  *
  * <h3>Example</h3>
  * <p>Using {@code ZipInputStream} is a little more complicated than {@link GZIPInputStream}
@@ -67,9 +68,6 @@ import libcore.io.Streams;
  *     zis.close();
  * }
  * </pre>
- *
- * @see ZipEntry
- * @see ZipFile
  */
 public class ZipInputStream extends InflaterInputStream implements ZipConstants {
     private static final int ZIPLocalHeaderVersionNeeded = 20;
@@ -213,13 +211,10 @@ public class ZipInputStream extends InflaterInputStream implements ZipConstants
     }
 
     /**
-     * Reads the next entry from this {@code ZipInputStream} or {@code null} if
+     * Returns the next entry from this {@code ZipInputStream} or {@code null} if
      * no more entries are present.
      *
-     * @return the next {@code ZipEntry} contained in the input stream.
-     * @throws IOException
-     *             if an {@code IOException} occurs.
-     * @see ZipEntry
+     * @throws IOException if an {@code IOException} occurs.
      */
     public ZipEntry getNextEntry() throws IOException {
         closeEntry();
diff --git a/luni/src/main/java/java/util/zip/ZipOutputStream.java b/luni/src/main/java/java/util/zip/ZipOutputStream.java
index 77a993b..9f7a4ad 100644
--- a/luni/src/main/java/java/util/zip/ZipOutputStream.java
+++ b/luni/src/main/java/java/util/zip/ZipOutputStream.java
@@ -23,20 +23,19 @@ import java.io.OutputStream;
 import java.nio.charset.Charsets;
 import java.util.Arrays;
 import java.util.HashSet;
+import libcore.util.EmptyArray;
 
 /**
  * This class provides an implementation of {@code FilterOutputStream} that
  * compresses data entries into a <i>ZIP-archive</i> output stream.
- * <p>
- * {@code ZipOutputStream} is used to write {@code ZipEntries} to the underlying
- * stream. Output from {@code ZipOutputStream} conforms to the {@code ZipFile}
- * file format.
- * <p>
- * While {@code DeflaterOutputStream} can write a compressed <i>ZIP-archive</i>
- * entry, this extension can write uncompressed entries as well. In this case
- * special rules apply, for this purpose refer to the <a
- * href="http://www.pkware.com/documents/casestudies/APPNOTE.TXT">file format
- * specification</a>.
+ *
+ * <p>{@code ZipOutputStream} is used to write {@link ZipEntry}s to the underlying
+ * stream. Output from {@code ZipOutputStream} can be read using {@link ZipFile}
+ * or {@link ZipInputStream}.
+ *
+ * <p>While {@code DeflaterOutputStream} can write a compressed <i>ZIP-archive</i>
+ * entry, this extension can write uncompressed entries as well.
+ * Use {@link ZipEntry#setMethod} or {@link #setMethod} with the {@link ZipEntry#STORED} flag.
  *
  * <h3>Example</h3>
  * <p>Using {@code ZipOutputStream} is a little more complicated than {@link GZIPOutputStream}
@@ -58,9 +57,6 @@ import java.util.HashSet;
  *     zos.close();
  * }
  * </pre>
- *
- * @see ZipEntry
- * @see ZipFile
  */
 public class ZipOutputStream extends DeflaterOutputStream implements ZipConstants {
 
@@ -76,13 +72,13 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
 
     private static final int ZIPLocalHeaderVersionNeeded = 20;
 
-    private String comment;
+    private byte[] commentBytes = EmptyArray.BYTE;
 
     private final HashSet<String> entries = new HashSet<String>();
 
-    private int compressMethod = DEFLATED;
+    private int defaultCompressionMethod = DEFLATED;
 
-    private int compressLevel = Deflater.DEFAULT_COMPRESSION;
+    private int compressionLevel = Deflater.DEFAULT_COMPRESSION;
 
     private ByteArrayOutputStream cDir = new ByteArrayOutputStream();
 
@@ -95,14 +91,11 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
     private byte[] nameBytes;
 
     /**
-     * Constructs a new {@code ZipOutputStream} with the specified output
-     * stream.
-     *
-     * @param p1
-     *            the {@code OutputStream} to write the data to.
+     * Constructs a new {@code ZipOutputStream} that writes a zip file
+     * to the given {@code OutputStream}.
      */
-    public ZipOutputStream(OutputStream p1) {
-        super(p1, new Deflater(Deflater.DEFAULT_COMPRESSION, true));
+    public ZipOutputStream(OutputStream os) {
+        super(os, new Deflater(Deflater.DEFAULT_COMPRESSION, true));
     }
 
     /**
@@ -131,7 +124,7 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
      *             If an error occurs closing the entry.
      */
     public void closeEntry() throws IOException {
-        checkClosed();
+        checkOpen();
         if (currentEntry == null) {
             return;
         }
@@ -186,12 +179,13 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
         } else {
             writeShort(cDir, 0);
         }
-        String c;
-        if ((c = currentEntry.getComment()) != null) {
-            writeShort(cDir, c.length());
-        } else {
-            writeShort(cDir, 0);
+
+        String comment = currentEntry.getComment();
+        byte[] commentBytes = EmptyArray.BYTE;
+        if (comment != null) {
+            commentBytes = comment.getBytes(Charsets.UTF_8);
         }
+        writeShort(cDir, commentBytes.length); // Comment length.
         writeShort(cDir, 0); // Disk Start
         writeShort(cDir, 0); // Internal File Attributes
         writeLong(cDir, 0); // External File Attributes
@@ -202,8 +196,8 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
             cDir.write(currentEntry.extra);
         }
         offset += curOffset;
-        if (c != null) {
-            cDir.write(c.getBytes());
+        if (commentBytes.length > 0) {
+            cDir.write(commentBytes);
         }
         currentEntry = null;
         crc.reset();
@@ -220,7 +214,7 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
      */
     @Override
     public void finish() throws IOException {
-        // TODO: is there a bug here? why not checkClosed?
+        // TODO: is there a bug here? why not checkOpen?
         if (out == null) {
             throw new IOException("Stream is closed");
         }
@@ -242,11 +236,9 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
         writeShort(cDir, entries.size()); // Number of entries
         writeLong(cDir, cdirSize); // Size of central dir
         writeLong(cDir, offset); // Offset of central dir
-        if (comment != null) {
-            writeShort(cDir, comment.length());
-            cDir.write(comment.getBytes());
-        } else {
-            writeShort(cDir, 0);
+        writeShort(cDir, commentBytes.length);
+        if (commentBytes.length > 0) {
+            cDir.write(commentBytes);
         }
         // Write the central directory.
         cDir.writeTo(out);
@@ -269,18 +261,33 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
         if (currentEntry != null) {
             closeEntry();
         }
-        if (ze.getMethod() == STORED || (compressMethod == STORED && ze.getMethod() == -1)) {
-            if (ze.crc == -1) {
-                throw new ZipException("CRC mismatch");
+
+        // Did this ZipEntry specify a method, or should we use the default?
+        int method = ze.getMethod();
+        if (method == -1) {
+            method = defaultCompressionMethod;
+        }
+
+        // If the method is STORED, check that the ZipEntry was configured appropriately.
+        if (method == STORED) {
+            if (ze.getCompressedSize() == -1) {
+                ze.setCompressedSize(ze.getSize());
+            } else if (ze.getSize() == -1) {
+                ze.setSize(ze.getCompressedSize());
             }
-            if (ze.size == -1 && ze.compressedSize == -1) {
-                throw new ZipException("Size mismatch");
+            if (ze.getCrc() == -1) {
+                throw new ZipException("STORED entry missing CRC");
             }
-            if (ze.size != ze.compressedSize && ze.compressedSize != -1 && ze.size != -1) {
-                throw new ZipException("Size mismatch");
+            if (ze.getSize() == -1) {
+                throw new ZipException("STORED entry missing size");
+            }
+            if (ze.size != ze.compressedSize) {
+                throw new ZipException("STORED entry size/compressed size mismatch");
             }
         }
-        checkClosed();
+
+        checkOpen();
+
         if (entries.contains(ze.name)) {
             throw new ZipException("Entry already exists: " + ze.name);
         }
@@ -294,35 +301,29 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
             throw new IllegalArgumentException("Name too long: " + nameLength + " UTF-8 bytes");
         }
 
-        def.setLevel(compressLevel);
+        def.setLevel(compressionLevel);
+        ze.setMethod(method);
+
         currentEntry = ze;
         entries.add(currentEntry.name);
-        if (currentEntry.getMethod() == -1) {
-            currentEntry.setMethod(compressMethod);
-        }
 
         // Local file header.
         // http://www.pkware.com/documents/casestudies/APPNOTE.TXT
-        int flags = currentEntry.getMethod() == STORED ? 0 : ZipFile.GPBF_DATA_DESCRIPTOR_FLAG;
+        int flags = (method == STORED) ? 0 : ZipFile.GPBF_DATA_DESCRIPTOR_FLAG;
         // Java always outputs UTF-8 filenames. (Before Java 7, the RI didn't set this flag and used
         // modified UTF-8. From Java 7, it sets this flag and uses normal UTF-8.)
         flags |= ZipFile.GPBF_UTF8_FLAG;
         writeLong(out, LOCSIG); // Entry header
         writeShort(out, ZIPLocalHeaderVersionNeeded); // Extraction version
         writeShort(out, flags);
-        writeShort(out, currentEntry.getMethod());
+        writeShort(out, method);
         if (currentEntry.getTime() == -1) {
             currentEntry.setTime(System.currentTimeMillis());
         }
         writeShort(out, currentEntry.time);
         writeShort(out, currentEntry.modDate);
 
-        if (currentEntry.getMethod() == STORED) {
-            if (currentEntry.size == -1) {
-                currentEntry.size = currentEntry.compressedSize;
-            } else if (currentEntry.compressedSize == -1) {
-                currentEntry.compressedSize = currentEntry.size;
-            }
+        if (method == STORED) {
             writeLong(out, currentEntry.crc);
             writeLong(out, currentEntry.size);
             writeLong(out, currentEntry.size);
@@ -344,16 +345,20 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
     }
 
     /**
-     * Sets the {@code ZipFile} comment associated with the file being written.
-     *
-     * @param comment
-     *            the comment associated with the file.
+     * Sets the comment associated with the file being written.
+     * @throws IllegalArgumentException if the comment is longer than 64 KiB.
      */
     public void setComment(String comment) {
-        if (comment.length() > 0xFFFF) {
-            throw new IllegalArgumentException("Comment too long: " + comment.length() + " characters");
+        if (comment == null) {
+            this.commentBytes = null;
+            return;
         }
-        this.comment = comment;
+
+        byte[] newCommentBytes = comment.getBytes(Charsets.UTF_8);
+        if (newCommentBytes.length > 0xffff) {
+            throw new IllegalArgumentException("Comment too long: " + newCommentBytes.length + " bytes");
+        }
+        this.commentBytes = newCommentBytes;
     }
 
     /**
@@ -369,22 +374,18 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
         if (level < Deflater.DEFAULT_COMPRESSION || level > Deflater.BEST_COMPRESSION) {
             throw new IllegalArgumentException("Bad level: " + level);
         }
-        compressLevel = level;
+        compressionLevel = level;
     }
 
     /**
-     * Sets the compression method to be used when compressing entry data.
-     * method must be one of {@code STORED} (for no compression) or {@code
-     * DEFLATED}.
-     *
-     * @param method
-     *            the compression method to use.
+     * Sets the default compression method to be used when a {@code ZipEntry} doesn't
+     * explicitly specify a method. See {@link ZipEntry#setMethod} for more details.
      */
     public void setMethod(int method) {
         if (method != STORED && method != DEFLATED) {
             throw new IllegalArgumentException("Bad method: " + method);
         }
-        compressMethod = method;
+        defaultCompressionMethod = method;
     }
 
     private long writeLong(OutputStream os, long i) throws IOException {
@@ -423,7 +424,7 @@ public class ZipOutputStream extends DeflaterOutputStream implements ZipConstant
         crc.update(buffer, offset, byteCount);
     }
 
-    private void checkClosed() throws IOException {
+    private void checkOpen() throws IOException {
         if (cDir == null) {
             throw new IOException("Stream is closed");
         }
diff --git a/luni/src/test/java/libcore/java/util/zip/ZipFileTest.java b/luni/src/test/java/libcore/java/util/zip/ZipFileTest.java
index afceaba..49dc050 100644
--- a/luni/src/test/java/libcore/java/util/zip/ZipFileTest.java
+++ b/luni/src/test/java/libcore/java/util/zip/ZipFileTest.java
@@ -26,6 +26,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.util.Enumeration;
 import java.util.Random;
+import java.util.zip.CRC32;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipException;
 import java.util.zip.ZipFile;
@@ -35,7 +36,6 @@ import junit.framework.TestCase;
 import libcore.io.IoUtils;
 
 public final class ZipFileTest extends TestCase {
-
     /**
      * Exercise Inflater's ability to refill the zlib's input buffer. As of this
      * writing, this buffer's max size is 64KiB compressed bytes. We'll write a
@@ -45,7 +45,7 @@ public final class ZipFileTest extends TestCase {
     public void testInflatingFilesRequiringZipRefill() throws IOException {
         int originalSize = 1024 * 1024;
         byte[] readBuffer = new byte[8192];
-        ZipFile zipFile = new ZipFile(createZipFile(originalSize));
+        ZipFile zipFile = new ZipFile(createZipFile(1, originalSize));
         for (Enumeration<? extends ZipEntry> e = zipFile.entries(); e.hasMoreElements(); ) {
             ZipEntry zipEntry = e.nextElement();
             assertTrue("This test needs >64 KiB of compressed data to exercise Inflater",
@@ -121,72 +121,303 @@ public final class ZipFileTest extends TestCase {
     public void testInflatingStreamsRequiringZipRefill() throws IOException {
         int originalSize = 1024 * 1024;
         byte[] readBuffer = new byte[8192];
-        ZipInputStream in = new ZipInputStream(new FileInputStream(createZipFile(originalSize)));
+        ZipInputStream in = new ZipInputStream(new FileInputStream(createZipFile(1, originalSize)));
         while (in.getNextEntry() != null) {
             while (in.read(readBuffer, 0, readBuffer.length) != -1) {}
         }
         in.close();
     }
 
+    public void testZipFileWithLotsOfEntries() throws IOException {
+        int expectedEntryCount = 64*1024 - 1;
+        File f = createZipFile(expectedEntryCount, 0);
+        ZipFile zipFile = new ZipFile(f);
+        int entryCount = 0;
+        for (Enumeration<? extends ZipEntry> e = zipFile.entries(); e.hasMoreElements(); ) {
+            ZipEntry zipEntry = e.nextElement();
+            ++entryCount;
+        }
+        assertEquals(expectedEntryCount, entryCount);
+        zipFile.close();
+    }
+
+    // http://code.google.com/p/android/issues/detail?id=36187
+    public void testZipFileLargerThan2GiB() throws IOException {
+        if (false) { // TODO: this test requires too much time and too much disk space!
+            File f = createZipFile(1024, 3*1024*1024);
+            ZipFile zipFile = new ZipFile(f);
+            int entryCount = 0;
+            for (Enumeration<? extends ZipEntry> e = zipFile.entries(); e.hasMoreElements(); ) {
+                ZipEntry zipEntry = e.nextElement();
+                ++entryCount;
+            }
+            assertEquals(1024, entryCount);
+            zipFile.close();
+        }
+    }
+
+    public void testZip64Support() throws IOException {
+        try {
+            createZipFile(64*1024, 0);
+            fail(); // Make this test more like testHugeZipFile when we have Zip64 support.
+        } catch (ZipException expected) {
+        }
+    }
+
     /**
-     * Compresses a single random file into a .zip archive.
+     * Compresses the given number of files, each of the given size, into a .zip archive.
      */
-    private File createZipFile(int uncompressedSize) throws IOException {
+    private File createZipFile(int entryCount, int entrySize) throws IOException {
+        File result = createTemporaryZipFile();
+
+        byte[] writeBuffer = new byte[8192];
+        Random random = new Random();
+
+        ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(result)));
+        for (int entry = 0; entry < entryCount; ++entry) {
+            ZipEntry ze = new ZipEntry(Integer.toHexString(entry));
+            out.putNextEntry(ze);
+
+            for (int i = 0; i < entrySize; i += writeBuffer.length) {
+                random.nextBytes(writeBuffer);
+                int byteCount = Math.min(writeBuffer.length, entrySize - i);
+                out.write(writeBuffer, 0, byteCount);
+            }
+
+            out.closeEntry();
+        }
+
+        out.close();
+        return result;
+    }
+
+    private File createTemporaryZipFile() throws IOException {
         File result = File.createTempFile("ZipFileTest", "zip");
         result.deleteOnExit();
+        return result;
+    }
 
-        ZipOutputStream out = new ZipOutputStream(new FileOutputStream(result));
-        ZipEntry entry = new ZipEntry("random");
-        out.putNextEntry(entry);
+    private ZipOutputStream createZipOutputStream(File f) throws IOException {
+        return new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(f)));
+    }
 
-        byte[] writeBuffer = new byte[8192];
-        Random random = new Random();
-        for (int i = 0; i < uncompressedSize; i += writeBuffer.length) {
-            random.nextBytes(writeBuffer);
-            out.write(writeBuffer, 0, Math.min(writeBuffer.length, uncompressedSize - i));
+    public void testSTORED() throws IOException {
+        ZipOutputStream out = createZipOutputStream(createTemporaryZipFile());
+        CRC32 crc = new CRC32();
+
+        // Missing CRC, size, and compressed size => failure.
+        try {
+            ZipEntry ze = new ZipEntry("a");
+            ze.setMethod(ZipEntry.STORED);
+            out.putNextEntry(ze);
+            fail();
+        } catch (ZipException expected) {
+        }
+
+        // Missing CRC and compressed size => failure.
+        try {
+            ZipEntry ze = new ZipEntry("a");
+            ze.setMethod(ZipEntry.STORED);
+            ze.setSize(0);
+            out.putNextEntry(ze);
+            fail();
+        } catch (ZipException expected) {
         }
 
+        // Missing CRC and size => failure.
+        try {
+            ZipEntry ze = new ZipEntry("a");
+            ze.setMethod(ZipEntry.STORED);
+            ze.setSize(0);
+            ze.setCompressedSize(0);
+            out.putNextEntry(ze);
+            fail();
+        } catch (ZipException expected) {
+        }
+
+        // Missing size and compressed size => failure.
+        try {
+            ZipEntry ze = new ZipEntry("a");
+            ze.setMethod(ZipEntry.STORED);
+            ze.setCrc(crc.getValue());
+            out.putNextEntry(ze);
+            fail();
+        } catch (ZipException expected) {
+        }
+
+        // Missing size is copied from compressed size.
+        {
+            ZipEntry ze = new ZipEntry("okay1");
+            ze.setMethod(ZipEntry.STORED);
+            ze.setCrc(crc.getValue());
+
+            assertEquals(-1, ze.getSize());
+            assertEquals(-1, ze.getCompressedSize());
+
+            ze.setCompressedSize(0);
+
+            assertEquals(-1, ze.getSize());
+            assertEquals(0, ze.getCompressedSize());
+
+            out.putNextEntry(ze);
+
+            assertEquals(0, ze.getSize());
+            assertEquals(0, ze.getCompressedSize());
+        }
+
+        // Missing compressed size is copied from size.
+        {
+            ZipEntry ze = new ZipEntry("okay2");
+            ze.setMethod(ZipEntry.STORED);
+            ze.setCrc(crc.getValue());
+
+            assertEquals(-1, ze.getSize());
+            assertEquals(-1, ze.getCompressedSize());
+
+            ze.setSize(0);
+
+            assertEquals(0, ze.getSize());
+            assertEquals(-1, ze.getCompressedSize());
+
+            out.putNextEntry(ze);
+
+            assertEquals(0, ze.getSize());
+            assertEquals(0, ze.getCompressedSize());
+        }
+
+        // Mismatched size and compressed size => failure.
+        try {
+            ZipEntry ze = new ZipEntry("a");
+            ze.setMethod(ZipEntry.STORED);
+            ze.setCrc(crc.getValue());
+            ze.setCompressedSize(1);
+            ze.setSize(0);
+            out.putNextEntry(ze);
+            fail();
+        } catch (ZipException expected) {
+        }
+
+        // Everything present => success.
+        ZipEntry ze = new ZipEntry("okay");
+        ze.setMethod(ZipEntry.STORED);
+        ze.setCrc(crc.getValue());
+        ze.setSize(0);
+        ze.setCompressedSize(0);
+        out.putNextEntry(ze);
+
+        out.close();
+    }
+
+    private String makeString(int count, String ch) {
+        StringBuilder sb = new StringBuilder();
+        for (int i = 0; i < count; ++i) {
+            sb.append(ch);
+        }
+        return sb.toString();
+    }
+
+    public void testComment() throws Exception {
+        String expectedFileComment = "1 \u0666 2";
+        String expectedEntryComment = "a \u0666 b";
+
+        File file = createTemporaryZipFile();
+        ZipOutputStream out = createZipOutputStream(file);
+
+        // Is file comment length checking done on bytes or characters? (Should be bytes.)
+        out.setComment(null);
+        out.setComment(makeString(0xffff, "a"));
+        try {
+            out.setComment(makeString(0xffff + 1, "a"));
+            fail();
+        } catch (IllegalArgumentException expected) {
+        }
+        try {
+            out.setComment(makeString(0xffff, "\u0666"));
+            fail();
+        } catch (IllegalArgumentException expected) {
+        }
+
+        ZipEntry ze = new ZipEntry("a");
+
+        // Is entry comment length checking done on bytes or characters? (Should be bytes.)
+        ze.setComment(null);
+        ze.setComment(makeString(0xffff, "a"));
+        try {
+            ze.setComment(makeString(0xffff + 1, "a"));
+            fail();
+        } catch (IllegalArgumentException expected) {
+        }
+        try {
+            ze.setComment(makeString(0xffff, "\u0666"));
+            fail();
+        } catch (IllegalArgumentException expected) {
+        }
+
+        ze.setComment(expectedEntryComment);
+        out.putNextEntry(ze);
         out.closeEntry();
+
+        out.setComment(expectedFileComment);
         out.close();
-        return result;
-      }
-
-      public void testHugeZipFile() throws IOException {
-          int expectedEntryCount = 64*1024 - 1;
-          File f = createHugeZipFile(expectedEntryCount);
-          ZipFile zipFile = new ZipFile(f);
-          int entryCount = 0;
-          for (Enumeration<? extends ZipEntry> e = zipFile.entries(); e.hasMoreElements(); ) {
-              ZipEntry zipEntry = e.nextElement();
-              ++entryCount;
-          }
-          assertEquals(expectedEntryCount, entryCount);
-          zipFile.close();
-      }
-
-      public void testZip64Support() throws IOException {
-          try {
-              createHugeZipFile(64*1024);
-              fail(); // Make this test more like testHugeZipFile when we have Zip64 support.
-          } catch (ZipException expected) {
-          }
-      }
-
-      /**
-       * Compresses the given number of empty files into a .zip archive.
-       */
-      private File createHugeZipFile(int count) throws IOException {
-          File result = File.createTempFile("ZipFileTest", "zip");
-          result.deleteOnExit();
-
-          ZipOutputStream out = new ZipOutputStream(new BufferedOutputStream(new FileOutputStream(result)));
-          for (int i = 0; i < count; ++i) {
-              ZipEntry entry = new ZipEntry(Integer.toHexString(i));
-              out.putNextEntry(entry);
-              out.closeEntry();
-          }
-
-          out.close();
-          return result;
-      }
+
+        ZipFile zipFile = new ZipFile(file);
+        // TODO: there's currently no API for reading the file comment --- strings(1) the file?
+        assertEquals(expectedEntryComment, zipFile.getEntry("a").getComment());
+        zipFile.close();
+    }
+
+    public void testNameLengthChecks() throws IOException {
+        // Is entry name length checking done on bytes or characters?
+        // Really it should be bytes, but the RI only checks characters at construction time.
+        // Android does the same, because it's cheap...
+        try {
+            new ZipEntry((String) null);
+            fail();
+        } catch (NullPointerException expected) {
+        }
+        new ZipEntry(makeString(0xffff, "a"));
+        try {
+            new ZipEntry(makeString(0xffff + 1, "a"));
+            fail();
+        } catch (IllegalArgumentException expected) {
+        }
+
+        // ...but Android won't let you create a zip file with a truncated name.
+        ZipOutputStream out = createZipOutputStream(createTemporaryZipFile());
+        ZipEntry ze = new ZipEntry(makeString(0xffff, "\u0666"));
+        try {
+            out.putNextEntry(ze);
+            fail(); // The RI fails this test; it just checks the character count at construction time.
+        } catch (IllegalArgumentException expected) {
+        }
+        out.closeEntry();
+        out.putNextEntry(new ZipEntry("okay")); // ZipOutputStream.close throws if you add nothing!
+        out.close();
+    }
+
+    public void testCrc() throws IOException {
+        ZipEntry ze = new ZipEntry("test");
+        ze.setMethod(ZipEntry.STORED);
+        ze.setSize(4);
+
+        // setCrc takes a long, not an int, so -1 isn't a valid CRC32 (because it's 64 bits).
+        try {
+            ze.setCrc(-1);
+        } catch (IllegalArgumentException expected) {
+        }
+
+        // You can set the CRC32 to 0xffffffff if you're slightly more careful though...
+        ze.setCrc(0xffffffffL);
+        assertEquals(0xffffffffL, ze.getCrc());
+
+        // And it actually works, even though we use -1L to mean "no CRC set"...
+        ZipOutputStream out = createZipOutputStream(createTemporaryZipFile());
+        out.putNextEntry(ze);
+        out.write(-1);
+        out.write(-1);
+        out.write(-1);
+        out.write(-1);
+        out.closeEntry();
+        out.close();
+    }
 }