Fix a bug I introduced, and do more zip cleanup. This code's quite hairy in its use of int/long. I can't just change the fields to int because they seem to use -1L to mean "unset" while still allowing the whole int range of values (including -1). We'll have to look at the zip specification to see whether that's right, but for now, let's just avoid sign extension. I've also cleaned up some of the other code, most notably throwing more detailed exception messages where it's easy, and removing hand-made readFully implementations. Bug: 3181430 Change-Id: I382568a8975c9b8b2ee5b344817134472369d308
diff --git a/luni/src/main/java/java/util/zip/ZipEntry.java b/luni/src/main/java/java/util/zip/ZipEntry.java index 0c123f8..b7bde59 100644 --- a/luni/src/main/java/java/util/zip/ZipEntry.java +++ b/luni/src/main/java/java/util/zip/ZipEntry.java
@@ -26,6 +26,7 @@ import java.util.Calendar; import java.util.Date; import java.util.GregorianCalendar; +import libcore.base.Streams; import libcore.io.BufferIterator; import libcore.io.HeapBufferIterator; @@ -49,7 +50,7 @@ byte[] extra; - int nameLen = -1; + int nameLength = -1; long mLocalHeaderRelOffset = -1; /** @@ -75,7 +76,7 @@ throw new NullPointerException(); } if (name.length() > 0xFFFF) { - throw new IllegalArgumentException(); + throw new IllegalArgumentException("Name too long: " + name.length()); } this.name = name; } @@ -180,12 +181,12 @@ /** * Sets the comment for this {@code ZipEntry}. * - * @param string + * @param comment * the comment for this entry. */ - public void setComment(String string) { - if (string == null || string.length() <= 0xFFFF) { - comment = string; + public void setComment(String comment) { + if (comment == null || comment.length() <= 0xFFFF) { + this.comment = comment; } else { throw new IllegalArgumentException(); } @@ -213,7 +214,7 @@ if (value >= 0 && value <= 0xFFFFFFFFL) { crc = value; } else { - throw new IllegalArgumentException(); + throw new IllegalArgumentException("Bad CRC32: " + value); } } @@ -244,7 +245,7 @@ */ public void setMethod(int value) { if (value != STORED && value != DEFLATED) { - throw new IllegalArgumentException(); + throw new IllegalArgumentException("Bad method: " + value); } compressionMethod = value; } @@ -261,7 +262,7 @@ if (value >= 0 && value <= 0xFFFFFFFFL) { size = value; } else { - throw new IllegalArgumentException(); + throw new IllegalArgumentException("Bad size: " + value); } } @@ -316,7 +317,7 @@ compressionMethod = ze.compressionMethod; modDate = ze.modDate; extra = ze.extra; - nameLen = ze.nameLen; + nameLength = ze.nameLength; mLocalHeaderRelOffset = ze.mLocalHeaderRelOffset; } @@ -351,18 +352,7 @@ * On exit, "in" will be positioned at the start of the next entry. */ ZipEntry(byte[] hdrBuf, InputStream in) throws IOException { - /* - * We're seeing performance issues when we call readShortLE and - * readIntLE, so we're going to read the entire header at once - * and then parse the results out without using any function calls. - * Uglier, but should be much faster. - * - * Note that some lines look a bit different, because the corresponding - * fields or locals are long and so we need to do & 0xffffffffl to avoid - * problems induced by sign extension. - */ - - myReadFully(in, hdrBuf); + Streams.readFully(in, hdrBuf, 0, hdrBuf.length); BufferIterator it = HeapBufferIterator.iterator(hdrBuf, 0, hdrBuf.length, ByteOrder.LITTLE_ENDIAN); @@ -375,50 +365,35 @@ compressionMethod = it.readShort(); time = it.readShort(); modDate = it.readShort(); - crc = it.readInt(); - compressedSize = it.readInt(); - size = it.readInt(); - nameLen = it.readShort(); - int extraLen = it.readShort(); - int commentLen = it.readShort(); + + // These are 32-bit values in the file, but 64-bit fields in this object. + crc = ((long) it.readInt()) & 0xffffffffL; + compressedSize = ((long) it.readInt()) & 0xffffffffL; + size = ((long) it.readInt()) & 0xffffffffL; + + nameLength = it.readShort(); + int extraLength = it.readShort(); + int commentLength = it.readShort(); + + // This is a 32-bit value in the file, but a 64-bit field in this object. it.seek(42); - mLocalHeaderRelOffset = it.readInt(); + mLocalHeaderRelOffset = ((long) it.readInt()) & 0xffffffffL; - byte[] nameBytes = new byte[nameLen]; - myReadFully(in, nameBytes); - - byte[] commentBytes = null; - if (commentLen > 0) { - commentBytes = new byte[commentLen]; - myReadFully(in, commentBytes); - } - - if (extraLen > 0) { - extra = new byte[extraLen]; - myReadFully(in, extra); - } + byte[] nameBytes = new byte[nameLength]; + Streams.readFully(in, nameBytes, 0, nameBytes.length); + name = new String(nameBytes, 0, nameBytes.length, Charsets.UTF_8); // The RI has always assumed UTF-8. (If GPBF_UTF8_FLAG isn't set, the encoding is // actually IBM-437.) - name = new String(nameBytes, 0, nameBytes.length, Charsets.UTF_8); - if (commentBytes != null) { + if (commentLength > 0) { + byte[] commentBytes = new byte[commentLength]; + Streams.readFully(in, commentBytes, 0, commentLength); comment = new String(commentBytes, 0, commentBytes.length, Charsets.UTF_8); - } else { - comment = null; } - } - private void myReadFully(InputStream in, byte[] b) throws IOException { - int len = b.length; - int off = 0; - - while (len > 0) { - int count = in.read(b, off, len); - if (count <= 0) { - throw new EOFException(); - } - off += count; - len -= count; + if (extraLength > 0) { + extra = new byte[extraLength]; + Streams.readFully(in, extra, 0, extraLength); } } }
diff --git a/luni/src/main/java/java/util/zip/ZipFile.java b/luni/src/main/java/java/util/zip/ZipFile.java index 61f3afc..366ec7f 100644 --- a/luni/src/main/java/java/util/zip/ZipFile.java +++ b/luni/src/main/java/java/util/zip/ZipFile.java
@@ -277,7 +277,7 @@ is.close(); // Skip the name and this "extra" data or whatever it is: - rafstrm.skip(entry.nameLen + localExtraLenOrWhatever); + rafstrm.skip(entry.nameLength + localExtraLenOrWhatever); rafstrm.mLength = rafstrm.mOffset + entry.compressedSize; if (entry.compressionMethod == ZipEntry.DEFLATED) { int bufSize = Math.max(1024, (int)Math.min(entry.getSize(), 65535L));
diff --git a/luni/src/main/java/java/util/zip/ZipInputStream.java b/luni/src/main/java/java/util/zip/ZipInputStream.java index 673a132..a502796 100644 --- a/luni/src/main/java/java/util/zip/ZipInputStream.java +++ b/luni/src/main/java/java/util/zip/ZipInputStream.java
@@ -174,13 +174,13 @@ private void readAndVerifyDataDescriptor(int inB, int out) throws IOException { if (hasDD) { in.read(hdrBuf, 0, EXTHDR); - long sig = OSMemory.peekInt(hdrBuf, 0, ByteOrder.LITTLE_ENDIAN); - if (sig != EXTSIG) { + int sig = OSMemory.peekInt(hdrBuf, 0, ByteOrder.LITTLE_ENDIAN); + if (sig != (int) EXTSIG) { throw new ZipException(String.format("unknown format (EXTSIG=%x)", sig)); } - currentEntry.crc = OSMemory.peekInt(hdrBuf, EXTCRC, ByteOrder.LITTLE_ENDIAN); - currentEntry.compressedSize = OSMemory.peekInt(hdrBuf, EXTSIZ, ByteOrder.LITTLE_ENDIAN); - currentEntry.size = OSMemory.peekInt(hdrBuf, EXTLEN, ByteOrder.LITTLE_ENDIAN); + currentEntry.crc = ((long) OSMemory.peekInt(hdrBuf, EXTCRC, ByteOrder.LITTLE_ENDIAN)) & 0xffffffffL; + currentEntry.compressedSize = ((long) OSMemory.peekInt(hdrBuf, EXTSIZ, ByteOrder.LITTLE_ENDIAN)) & 0xffffffffL; + currentEntry.size = ((long) OSMemory.peekInt(hdrBuf, EXTLEN, ByteOrder.LITTLE_ENDIAN)) & 0xffffffffL; } if (currentEntry.crc != crc.getValue()) { throw new ZipException("CRC mismatch"); @@ -205,14 +205,8 @@ return null; } - int x = 0, count = 0; - while (count != 4) { - count += x = in.read(hdrBuf, count, 4 - count); - if (x == -1) { - return null; - } - } - long hdr = OSMemory.peekInt(hdrBuf, 0, ByteOrder.LITTLE_ENDIAN); + Streams.readFully(in, hdrBuf, 0, 4); + int hdr = OSMemory.peekInt(hdrBuf, 0, ByteOrder.LITTLE_ENDIAN); if (hdr == CENSIG) { entriesEnd = true; return null; @@ -222,70 +216,52 @@ } // Read the local header - count = 0; - while (count != (LOCHDR - LOCVER)) { - count += x = in.read(hdrBuf, count, (LOCHDR - LOCVER) - count); - if (x == -1) { - throw new EOFException(); - } - } + Streams.readFully(in, hdrBuf, 0, (LOCHDR - LOCVER)); int version = OSMemory.peekShort(hdrBuf, 0, ByteOrder.LITTLE_ENDIAN) & 0xff; if (version > ZIPLocalHeaderVersionNeeded) { throw new ZipException("Cannot read local header version " + version); } - int flags = OSMemory.peekShort(hdrBuf, LOCFLG - LOCVER, ByteOrder.LITTLE_ENDIAN); + short flags = OSMemory.peekShort(hdrBuf, LOCFLG - LOCVER, ByteOrder.LITTLE_ENDIAN); hasDD = ((flags & ZipFile.GPBF_DATA_DESCRIPTOR_FLAG) != 0); - int cetime = OSMemory.peekShort(hdrBuf, LOCTIM - LOCVER, ByteOrder.LITTLE_ENDIAN); - int cemodDate = OSMemory.peekShort(hdrBuf, LOCTIM - LOCVER + 2, ByteOrder.LITTLE_ENDIAN); - int cecompressionMethod = OSMemory.peekShort(hdrBuf, LOCHOW - LOCVER, ByteOrder.LITTLE_ENDIAN); - long cecrc = 0, cecompressedSize = 0, cesize = -1; + int ceTime = OSMemory.peekShort(hdrBuf, LOCTIM - LOCVER, ByteOrder.LITTLE_ENDIAN) & 0xffff; + int ceModDate = OSMemory.peekShort(hdrBuf, LOCTIM - LOCVER + 2, ByteOrder.LITTLE_ENDIAN) & 0xffff; + int ceCompressionMethod = OSMemory.peekShort(hdrBuf, LOCHOW - LOCVER, ByteOrder.LITTLE_ENDIAN) & 0xffff; + long ceCrc = 0, ceCompressedSize = 0, ceSize = -1; if (!hasDD) { - cecrc = OSMemory.peekInt(hdrBuf, LOCCRC - LOCVER, ByteOrder.LITTLE_ENDIAN); - cecompressedSize = OSMemory.peekInt(hdrBuf, LOCSIZ - LOCVER, ByteOrder.LITTLE_ENDIAN); - cesize = OSMemory.peekInt(hdrBuf, LOCLEN - LOCVER, ByteOrder.LITTLE_ENDIAN); + ceCrc = ((long) OSMemory.peekInt(hdrBuf, LOCCRC - LOCVER, ByteOrder.LITTLE_ENDIAN)) & 0xffffffffL; + ceCompressedSize = ((long) OSMemory.peekInt(hdrBuf, LOCSIZ - LOCVER, ByteOrder.LITTLE_ENDIAN)) & 0xffffffffL; + ceSize = ((long) OSMemory.peekInt(hdrBuf, LOCLEN - LOCVER, ByteOrder.LITTLE_ENDIAN)) & 0xffffffffL; } - int flen = OSMemory.peekShort(hdrBuf, LOCNAM - LOCVER, ByteOrder.LITTLE_ENDIAN); - if (flen == 0) { + int nameLength = OSMemory.peekShort(hdrBuf, LOCNAM - LOCVER, ByteOrder.LITTLE_ENDIAN) & 0xffff; + if (nameLength == 0) { throw new ZipException("Entry is not named"); } - int elen = OSMemory.peekShort(hdrBuf, LOCEXT - LOCVER, ByteOrder.LITTLE_ENDIAN); + int extraLength = OSMemory.peekShort(hdrBuf, LOCEXT - LOCVER, ByteOrder.LITTLE_ENDIAN) & 0xffff; - count = 0; - if (flen > nameBuf.length) { - nameBuf = new byte[flen]; - charBuf = new char[flen]; + if (nameLength > nameBuf.length) { + nameBuf = new byte[nameLength]; + // The bytes are modified UTF-8, so the number of chars will always be less than or + // equal to the number of bytes. It's fine if this buffer is too long. + charBuf = new char[nameLength]; } - while (count != flen) { - count += x = in.read(nameBuf, count, flen - count); - if (x == -1) { - throw new EOFException(); - } + Streams.readFully(in, nameBuf, 0, nameLength); + currentEntry = createZipEntry(ModifiedUtf8.decode(nameBuf, charBuf, 0, nameLength)); + currentEntry.time = ceTime; + currentEntry.modDate = ceModDate; + currentEntry.setMethod(ceCompressionMethod); + if (ceSize != -1) { + currentEntry.setCrc(ceCrc); + currentEntry.setSize(ceSize); + currentEntry.setCompressedSize(ceCompressedSize); } - currentEntry = createZipEntry(ModifiedUtf8.decode(nameBuf, charBuf, 0, flen)); - currentEntry.time = cetime; - currentEntry.modDate = cemodDate; - currentEntry.setMethod(cecompressionMethod); - if (cesize != -1) { - currentEntry.setCrc(cecrc); - currentEntry.setSize(cesize); - currentEntry.setCompressedSize(cecompressedSize); - } - if (elen > 0) { - count = 0; - byte[] e = new byte[elen]; - while (count != elen) { - count += x = in.read(e, count, elen - count); - if (x == -1) { - throw new EOFException(); - } - } - currentEntry.setExtra(e); + if (extraLength > 0) { + byte[] extraData = new byte[extraLength]; + Streams.readFully(in, extraData, 0, extraLength); + currentEntry.setExtra(extraData); } return currentEntry; } - /* Read 4 bytes from the buffer and store it as an int */ - /** * Reads up to the specified number of uncompressed bytes into the buffer * starting at the offset.