diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java index 576ba5de..1b3f577e 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -3,7 +3,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.nio.ByteBuffer; @@ -25,23 +24,64 @@ public class ZipReader { public static final int END_OF_CENTRAL_DIRECTORY_SIZE = 22; public static final int ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIZE = 20; + /** + * How many reads in a row may come back empty before the channel is taken to have stalled. + */ + private static final int MAX_CONSECUTIVE_EMPTY_READS = 16; + + /** + * Reads at least one byte into {@code buf}, which must have room for one. Only {@code -1} + * from {@link SeekableByteChannel#read} is an end of file; {@code 0} just means nothing + * arrived this time, which a non-blocking channel is allowed to do. Empty reads are retried, + * but only {@value #MAX_CONSECUTIVE_EMPTY_READS} times in a row, so that a channel with + * nothing to give fails instead of spinning forever. + * + * @return the number of bytes read, which is always positive, or {@code -1} at end of file + * @throws IOException if the channel keeps returning nothing + */ + private int readSome(ByteBuffer buf) throws IOException { + for (int attempt = 0; attempt < MAX_CONSECUTIVE_EMPTY_READS; attempt++) { + int read = zipChannel.read(buf); + if (read != 0) { + return read; + } + } + throw new IOException("channel returned no data " + MAX_CONSECUTIVE_EMPTY_READS + + " times in a row at offset " + zipChannel.position()); + } + + /** + * Fills {@code buf} from the channel. {@link SeekableByteChannel#read} may return fewer bytes + * than asked for while more are still available, so a single read cannot tell "the archive + * ends here" from "that read came up short" — and taking the second for the first rejects a + * perfectly good archive. + * + * @return false at end of file, in which case {@code buf} holds nothing worth reading + */ + private boolean fill(ByteBuffer buf) throws IOException { + buf.clear(); + while (buf.hasRemaining()) { + if (readSome(buf) < 0) { + return false; + } + } + buf.flip(); + return true; + } + final ByteBuffer longBuf = ByteBuffer.allocate(Long.BYTES).order(ByteOrder.LITTLE_ENDIAN); private long readLong() throws IOException { - longBuf.clear(); - if (this.zipChannel.read(longBuf) != 8) { + if (!fill(longBuf)) { throw new InvalidZipException("Expected long value"); } - longBuf.flip(); return longBuf.getLong(); } final ByteBuffer intBuf = ByteBuffer.allocate(Integer.BYTES).order(ByteOrder.LITTLE_ENDIAN); private Integer readInteger() throws IOException { - intBuf.clear(); - if (this.zipChannel.read(intBuf) != 4) { + if (!fill(intBuf)) { return null; } - intBuf.flip(); return intBuf.getInt(); } private int readInt() throws IOException { @@ -64,11 +104,9 @@ private long readUnsignedInt() throws IOException { final ByteBuffer shortBuf = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.LITTLE_ENDIAN); private short readShort() throws IOException { - shortBuf.clear(); - if (this.zipChannel.read(shortBuf) != 2) { + if (!fill(shortBuf)) { throw new InvalidZipException("Expected short value"); } - shortBuf.flip(); return shortBuf.getShort(); } @@ -102,25 +140,81 @@ public CentralDirectoryRecord(long numEntries, long offsetToStart) { private static final int ZIP64_MAGIC_SHORT = 0xFFFF; private static final int ZIP64_EXTID= 0x0001; + /** + * The longest comment the end of central directory record can carry, since its length lives + * in a 2-byte field. This is how far back from the end of the archive the scan for the record + * looks. Trailing data after the record is tolerated, but only within this same budget, + * because nothing records how long it is. + */ + private static final long MAX_END_OF_CENTRAL_DIRECTORY_COMMENT_SIZE = 0xFFFF; + + /** The fixed part of a central directory file header, before its variable length fields. */ + private static final int CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE = 46; + + /** + * Positions the channel at an offset that came out of the archive itself. The zip64 records + * carry offsets as 64-bit values, so a corrupt archive can point anywhere. Unchecked, a + * negative offset escapes as an {@link IllegalArgumentException} from the channel rather than + * as a zip error, and one past the end fails at the next read with a generic complaint that + * names neither the offset nor the field it came from. + */ + private void seekWithinArchive(String what, long offset) throws IOException { + if (offset < 0 || offset >= zipChannel.size()) { + throw new InvalidZipException(what + " points to offset " + offset + + ", which is outside this " + zipChannel.size() + " byte archive"); + } + zipChannel.position(offset); + } + + /** + * Checks that the comment of a candidate end of central directory record ends inside the + * archive, as a real record's comment does. The backward scan can meet a stray signature + * inside the comment or in trailing data before it reaches the real record; this rules out + * most of them, along with a real record whose comment was cut short. Trailing data after + * the comment is allowed. + */ + private boolean commentEndsWithinArchive(long eoCDRStart) throws IOException { + zipChannel.position(eoCDRStart + END_OF_CENTRAL_DIRECTORY_SIZE - Short.BYTES); + int commentLength = readUnsignedShort(); + return eoCDRStart + END_OF_CENTRAL_DIRECTORY_SIZE + commentLength <= zipChannel.size(); + } + CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { long eoCDRStart = zipChannel.size() - END_OF_CENTRAL_DIRECTORY_SIZE; // 22 is the minimum size of the EOCDR - - while (eoCDRStart >= 0) { + // the record can only be pushed back from the end of the archive by its own comment, plus + // whatever trailing data fits in the same budget, so there is no reason to look back any + // further than the longest possible comment. an unbounded scan walks the whole archive a + // byte at a time doing a positioned four byte read per byte — tens of seconds per hundred + // MiB against a file — before it can report that the archive is not a zip, and it gives a + // stray signature deep inside a payload a chance to be mistaken for the record + long earliestPossibleStart = Math.max(0, zipChannel.size() + - (END_OF_CENTRAL_DIRECTORY_SIZE + MAX_END_OF_CENTRAL_DIRECTORY_COMMENT_SIZE)); + + boolean found = false; + while (eoCDRStart >= earliestPossibleStart) { zipChannel.position(eoCDRStart); Integer signature = readInteger(); - if (signature == null || signature == END_OF_CENTRAL_DIRECTORY_SIGNATURE) { - if (logger.isDebugEnabled()) { - logger.debug("Found end of central directory signature at {}", zipChannel.position() - Integer.BYTES); - } + if (signature == null) { + // every offset this scan probes has a whole record behind it, so an end of file + // here means the channel holds less than its size claims + throw new InvalidZipException("Archive ended at offset " + zipChannel.position() + + ", before its reported size of " + zipChannel.size() + " bytes"); + } + if (signature == END_OF_CENTRAL_DIRECTORY_SIGNATURE && commentEndsWithinArchive(eoCDRStart)) { + logger.debug("Found end of central directory signature at {}", eoCDRStart); + found = true; break; } eoCDRStart--; } - if (eoCDRStart < 0) { - throw new InvalidZipException("Didn't find the end of central directory"); + if (!found) { + throw new InvalidZipException("Didn't find the end of central directory in the last " + + (zipChannel.size() - earliestPossibleStart) + " bytes of this " + + zipChannel.size() + " byte archive"); } + zipChannel.position(eoCDRStart + Integer.BYTES); short diskNumber = readShort(); short centralDirectoryDiskNumber = readShort(); short numCDEntriesOnThisDisk = readShort(); @@ -128,7 +222,7 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { int totalNumEntries = readUnsignedShort(); long sizeOfCentralDirectory = readUnsignedInt(); long offsetToStartOfCentralDirectory = readUnsignedInt(); - int commentLength = readUnsignedShort(); + // the comment length comes next, and the scan has already checked it // any one of these fields may carry the sentinel that sends its real value to the zip64 // end of central directory record; an archive can need zip64 for its entry count alone @@ -140,26 +234,39 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { return new CentralDirectoryRecord(totalNumEntries, offsetToStartOfCentralDirectory); } - long zip64CentralDirectoryLocatorStart = zipChannel.size() - (ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIZE + END_OF_CENTRAL_DIRECTORY_SIZE + commentLength); - zipChannel.position(zip64CentralDirectoryLocatorStart); - return extractZIP64CentralDirectoryInfo(); + // the locator sits immediately before the record we found, so it is measured from there + // rather than from the end of the archive. the two agree only when nothing follows the + // record but a comment whose length is honest; measuring from the end lands at the wrong + // offset for anything else and blames the locator for it + long zip64CentralDirectoryLocatorStart = eoCDRStart - ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIZE; + if (zip64CentralDirectoryLocatorStart < 0) { + throw new InvalidZipException( + "Archive is too small to hold the zip64 end of central directory locator it claims to have"); + } + return extractZIP64CentralDirectoryInfo(zip64CentralDirectoryLocatorStart); } - private CentralDirectoryRecord extractZIP64CentralDirectoryInfo() throws IOException { - // buffer's position at the start of the Central Directory + private CentralDirectoryRecord extractZIP64CentralDirectoryInfo(long locatorStart) throws IOException { + zipChannel.position(locatorStart); Integer signature = readInteger(); if (signature == null || signature != ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIGNATURE) { - throw new InvalidZipException("Invalid Zip64 End of Central Directory Record Signature"); + throw new InvalidZipException("Invalid zip64 end of central directory locator signature at offset " + + locatorStart + ": expected 0x" + + Integer.toHexString(ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIGNATURE) + + " but found " + (signature == null ? "the end of the archive" : "0x" + Integer.toHexString(signature))); } int centralDirectoryDiskNumber = readInt(); long offsetToEndOfCentralDirectory = readLong(); int totalNumberOfDisks = readInt(); - zipChannel.position(offsetToEndOfCentralDirectory); + seekWithinArchive("the zip64 end of central directory locator", offsetToEndOfCentralDirectory); int sig = readInt(); if (sig != ZIP_64_END_OF_CENTRAL_DIRECTORY_SIGNATURE) { - throw new InvalidZipException("Invalid"); + throw new InvalidZipException("Invalid zip64 end of central directory signature at offset " + + offsetToEndOfCentralDirectory + ": expected 0x" + + Integer.toHexString(ZIP_64_END_OF_CENTRAL_DIRECTORY_SIGNATURE) + + " but found 0x" + Integer.toHexString(sig)); } long sizeOfEndOfCentralDirectoryRecord = readLong(); short versionMadeBy = readShort(); @@ -241,10 +348,8 @@ public int read() throws IOException { } setChannelPosition(); buf.clear(); - while (buf.hasRemaining()) { - if (zipChannel.read(buf) <= 0) { - return -1; - } + if (readSome(buf) < 0) { + throw truncated(); } offset += 1; return buf.array()[0] & 0xFF; @@ -254,6 +359,15 @@ private boolean doneReading() { return offset >= fileSize; } + /** + * The archive ended before this entry's data did. Reporting that as an ordinary + * end of stream would hand the caller a short entry with no sign anything is wrong. + */ + private InvalidZipException truncated() { + return new InvalidZipException("Archive ended " + offset + " bytes into the " + + fileSize + " byte entry [" + fileName + "]"); + } + private void setChannelPosition() throws IOException { var nextPosition = startPosition + offset; if (zipChannel.position() != nextPosition) { @@ -263,16 +377,22 @@ private void setChannelPosition() throws IOException { @Override public int read(byte[] b, int off, int len) throws IOException { + if (len == 0) { + return 0; + } if (doneReading()) { return -1; } setChannelPosition(); var lenToRead = (int)Math.min(len, fileSize - offset); // cast is always valid because len is an int var buf = ByteBuffer.wrap(b, off, lenToRead); - var nread = zipChannel.read(buf); - if (nread > 0) { - offset += nread; + // InputStream must not return 0 for a non-empty request, so this has to wait + // for at least one byte rather than pass an empty channel read through + var nread = readSome(buf); + if (nread < 0) { + throw truncated(); } + offset += nread; return nread; } }; @@ -300,11 +420,12 @@ public Entry readCentralDirectoryFileHeader() throws IOException { int externalFileAttributes = readInt(); long relativeOffsetOfLocalHeader = readUnsignedInt(); + long fileNameStart = zipChannel.position(); ByteBuffer fileName = ByteBuffer.allocate(fileNameLength); - while (fileName.hasRemaining()) { - if (zipChannel.read(fileName) <= 0) { - throw new EOFException("Unexpected EOF when reading filename of length: " + fileNameLength); - } + if (!fill(fileName)) { + throw new InvalidZipException("Archive ended inside the " + fileNameLength + + " byte filename of the central directory file header at offset " + + (fileNameStart - CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE)); } // Parse the extra field @@ -341,8 +462,17 @@ public Entry readCentralDirectoryFileHeader() throws IOException { public ZipReader(SeekableByteChannel channel) throws IOException { zipChannel = channel; var centralDirectoryRecord = readEndOfCentralDirectory(); - zipChannel.position(centralDirectoryRecord.offsetToStart); - for (int i = 0; i < centralDirectoryRecord.numEntries; i++) { + seekWithinArchive("the central directory", centralDirectoryRecord.offsetToStart); + // the zip64 count is a 64-bit value straight from the archive. a corrupt one that comes + // out negative would otherwise read as an empty archive, with no sign anything was wrong + long bytesAvailable = zipChannel.size() - centralDirectoryRecord.offsetToStart; + long mostEntriesThatFit = bytesAvailable / CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE; + if (centralDirectoryRecord.numEntries < 0 || centralDirectoryRecord.numEntries > mostEntriesThatFit) { + throw new InvalidZipException("The central directory claims " + centralDirectoryRecord.numEntries + + " entries, but the " + bytesAvailable + " bytes from its start to the end of the archive" + + " can hold at most " + mostEntriesThatFit); + } + for (long i = 0; i < centralDirectoryRecord.numEntries; i++) { entries.add(readCentralDirectoryFileHeader()); } } diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java index 65357573..55cdd045 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java @@ -30,6 +30,30 @@ public class ZipWriter { * {@link ZipReader}. */ static final long MAX_NON_ZIP64_VALUE = Integer.MAX_VALUE; + + /** + * The largest entry count we will write into the 2-byte end of central directory field. + * {@code 0xFFFF} in that field is the sentinel that sends the real count to the zip64 end of + * central directory record, so the format's own limit is {@code 0xFFFE}. + *

+ * We stop at {@link Short#MAX_VALUE} instead, for the reason spelled out on + * {@link #MAX_NON_ZIP64_VALUE}: the field is unsigned on the wire, but a reader that widens it + * with a signed read sees any count above {@code 0x7FFF} as negative and then finds no entries + * at all — which is what versions of this SDK that predate the unsigned reads in + * {@link ZipReader} do. Switching to ZIP64 above 32,767 entries costs one zip64 end of central + * directory record and locator for the whole archive. + */ + private static final long MAX_NON_ZIP64_ENTRY_COUNT = Short.MAX_VALUE; + + /** + * The longest entry name, in UTF-8 bytes, we will write. The filename length field is 2 bytes + * and unsigned, so the format allows {@code 0xFFFF}. We stop at {@link Short#MAX_VALUE} for + * the same reason as {@link #MAX_NON_ZIP64_ENTRY_COUNT}: versions of this SDK that predate + * the unsigned reads in {@link ZipReader} read the length as signed, and a negative length + * fails there. + */ + static final int MAX_FILENAME_LENGTH = Short.MAX_VALUE; + private static final long ZIP_64_END_OF_CD_RECORD_SIZE = 56; private static final int ZIP_64_GLOBAL_EXTENDED_INFO_EXTRA_FIELD_SIZE = 28; @@ -81,16 +105,40 @@ static void checkFitsInCentralDirectory(String name, long offset, long size) { } } + /** + * Encodes an entry name the way it is written to the archive. Zip filename lengths are + * counted in bytes, so anything derived from {@link String#length()} — which counts UTF-16 + * code units — desyncs the central directory for a non-ASCII name. + */ + private static byte[] encodeFilename(String name) { + var bytes = name.getBytes(StandardCharsets.UTF_8); + if (bytes.length > MAX_FILENAME_LENGTH) { + throw new SDKException("zip entry name [" + abbreviate(name) + "] is " + bytes.length + + " bytes when encoded as UTF-8, more than the " + MAX_FILENAME_LENGTH + + " bytes we write into the filename length field"); + } + return bytes; + } + + private static final int ABBREVIATED_NAME_CODE_POINTS = 32; + + /** The start of a name too long to put in an error message whole. */ + private static String abbreviate(String name) { + if (name.codePointCount(0, name.length()) <= ABBREVIATED_NAME_CODE_POINTS) { + return name; + } + return name.substring(0, name.offsetByCodePoints(0, ABBREVIATED_NAME_CODE_POINTS)) + "..."; + } + public OutputStream stream(String name) throws IOException { var startPosition = out.position; long fileTime, fileDate; fileTime = fileDate = getTimeDateUnMSDosFormat(); - var nameBytes = name.getBytes(StandardCharsets.UTF_8); + var nameBytes = encodeFilename(name); LocalFileHeader localFileHeader = new LocalFileHeader(); localFileHeader.lastModifiedTime = (int) fileTime; localFileHeader.lastModifiedDate = (int) fileDate; - localFileHeader.filenameLength = (short) nameBytes.length; localFileHeader.crc32 = 0; localFileHeader.generalPurposeBitFlag = (1 << 3) | (1 << 11); // we are using the data descriptor and we are using UTF-8 localFileHeader.compressedSize = ZIP_64_MAGIC_VAL; @@ -170,12 +218,12 @@ private static void writeCentralDirectoryHeader(FileInfo fileInfo, OutputStream checkFitsInCentralDirectory(fileInfo.filename, fileInfo.offset, fileInfo.size); } + var nameBytes = encodeFilename(fileInfo.filename); CDFileHeader cdFileHeader = new CDFileHeader(); cdFileHeader.generalPurposeBitFlag = fileInfo.flag; cdFileHeader.lastModifiedTime = fileInfo.fileTime; cdFileHeader.lastModifiedDate = fileInfo.fileDate; cdFileHeader.crc32 = (int) fileInfo.crc; - cdFileHeader.filenameLength = (short) fileInfo.filename.length(); cdFileHeader.extraFieldLength = 0; cdFileHeader.compressedSize = (int) fileInfo.size; cdFileHeader.uncompressedSize = (int) fileInfo.size; @@ -188,7 +236,7 @@ private static void writeCentralDirectoryHeader(FileInfo fileInfo, OutputStream cdFileHeader.extraFieldLength = ZIP_64_GLOBAL_EXTENDED_INFO_EXTRA_FIELD_SIZE; } - cdFileHeader.write(out, fileInfo.filename.getBytes(StandardCharsets.UTF_8)); + cdFileHeader.write(out, nameBytes); if (fileInfo.isZip64) { Zip64GlobalExtendedInfoExtraField zip64ExtendedInfoExtraField = new Zip64GlobalExtendedInfoExtraField(); @@ -208,24 +256,25 @@ private FileInfo writeByteArray(String name, byte[] data, CountingOutputStream o crc.update(data); var crcValue = crc.getValue(); - var nameBytes = name.getBytes(StandardCharsets.UTF_8); + var nameBytes = encodeFilename(name); LocalFileHeader localFileHeader = new LocalFileHeader(); localFileHeader.lastModifiedTime = (int) fileTime; localFileHeader.lastModifiedDate = (int) fileDate; - localFileHeader.filenameLength = (short) nameBytes.length; - localFileHeader.generalPurposeBitFlag = 0; + // the name is UTF-8, and readers that go by the local header need to be told so just as + // much as those that go by the central directory + localFileHeader.generalPurposeBitFlag = (1 << 11); localFileHeader.crc32 = (int) crcValue; localFileHeader.compressedSize = data.length; localFileHeader.uncompressedSize = data.length; localFileHeader.extraFieldLength = 0; - localFileHeader.write(out, name.getBytes(StandardCharsets.UTF_8)); + localFileHeader.write(out, nameBytes); out.write(data); var fileInfo = new FileInfo(); fileInfo.offset = startPosition; - fileInfo.flag = (1 << 11); + fileInfo.flag = (short) localFileHeader.generalPurposeBitFlag; fileInfo.size = data.length; fileInfo.crc = crcValue; fileInfo.filename = name; @@ -238,10 +287,15 @@ private FileInfo writeByteArray(String name, byte[] data, CountingOutputStream o private void writeEndOfCentralDirectory(boolean hasZip64Entry, long numEntries, long startOfCentralDirectory, long sizeOfCentralDirectory, CountingOutputStream out) throws IOException { + // each of these corresponds to a field in the end of central directory record that has to + // carry a sentinel — and send its real value to the zip64 record — once the value stops + // fitting. the entry count is 2 bytes and uses its own fixed limit; the offset and size + // are 4 bytes each and go through the configured threshold, which is MAX_NON_ZIP64_VALUE + // outside of tests. both limits stop short of what the format allows, so that readers + // which widen these fields with a signed read never see a negative value var isZip64 = hasZip64Entry - || (numEntries & ~0xFF) != 0 - || (startOfCentralDirectory & ~0xFFFF) != 0 - || (sizeOfCentralDirectory & ~0xFFFF) != 0; + || numEntries > MAX_NON_ZIP64_ENTRY_COUNT + || needsZip64(startOfCentralDirectory, sizeOfCentralDirectory); if (isZip64) { var endPosition = out.position; @@ -322,7 +376,6 @@ private static class LocalFileHeader { int compressedSize; int uncompressedSize; - short filenameLength; short extraFieldLength = 0; void write(OutputStream out, byte[] filename) throws IOException { @@ -337,7 +390,8 @@ void write(OutputStream out, byte[] filename) throws IOException { buffer.putInt(crc32); buffer.putInt(compressedSize); buffer.putInt(uncompressedSize); - buffer.putShort(filenameLength); + // the length of the encoded bytes, never String.length() + buffer.putShort((short) filename.length); buffer.putShort(extraFieldLength); buffer.put(filename); @@ -376,7 +430,6 @@ private static class CDFileHeader { int crc32; int compressedSize; int uncompressedSize; - short filenameLength; short extraFieldLength; final short fileCommentLength = 0; final short diskNumberStart = 0; @@ -397,6 +450,7 @@ void write(OutputStream out, byte[] filename) throws IOException { buffer.putInt(crc32); buffer.putInt(compressedSize); buffer.putInt(uncompressedSize); + // the length of the encoded bytes, never String.length() buffer.putShort((short) filename.length); buffer.putShort(extraFieldLength); buffer.putShort(fileCommentLength); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java index fa2014bd..6c9cebba 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -16,13 +16,18 @@ import java.nio.ByteOrder; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Random; import java.util.stream.Collectors; import java.util.stream.IntStream; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; public class ZipReaderTest { @@ -241,11 +246,7 @@ private static byte[] keepOnlyTheEntryCountSentinel(byte[] archive) { private static void assertReadsEveryEntry(byte[] archive) throws IOException { try (var channel = new SeekableInMemoryByteChannel(archive)) { - var reader = new ZipReader(channel); - assertThat(reader.getEntries()).hasSize(3); - assertThat(readEntry(reader, "small.txt")).isEqualTo("tiny"); - assertThat(readEntry(reader, "0.payload")).isEqualTo(PAYLOAD); - assertThat(readEntry(reader, "0.manifest.json")).isEqualTo(MANIFEST); + assertReadsEveryEntry(channel); } // and an independent implementation, so this shows the patched archive is well formed @@ -258,6 +259,453 @@ private static void assertReadsEveryEntry(byte[] archive) throws IOException { } } + /** Our reader alone, for archives or channels an independent implementation can't take. */ + private static void assertReadsEveryEntry(SeekableByteChannel channel) throws IOException { + var reader = new ZipReader(channel); + assertThat(reader.getEntries()).hasSize(3); + assertThat(readEntry(reader, "small.txt")).isEqualTo("tiny"); + assertThat(readEntry(reader, "0.payload")).isEqualTo(PAYLOAD); + assertThat(readEntry(reader, "0.manifest.json")).isEqualTo(MANIFEST); + } + + /** + * An archive missing the trailing records it needs is not a zip, and has to say so rather + * than fail somewhere downstream. These all reach the same rejection, by different routes: + * the scan finds no signature within its window. + */ + @Test + public void testTruncatedArchiveIsRejected() throws IOException { + var archive = zip64Archive(); + + // the trailing end of central directory record and its locator are gone + var withoutTrailingRecords = + Arrays.copyOf(archive, archive.length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE)); + // only the last few bytes of the record are gone + var clippedRecord = Arrays.copyOf(archive, archive.length - 4); + // cut down to less than a single end of central directory record + var shorterThanARecord = Arrays.copyOf(archive, EOCD_SIZE - 1); + + for (var truncated : List.of(withoutTrailingRecords, clippedRecord, shorterThanARecord, new byte[0])) { + assertThatThrownBy(() -> readArchive(truncated)) + .withFailMessage("a %d byte truncation should fail the scan", truncated.length) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("Didn't find the end of central directory"); + } + } + + /** + * The end of central directory record claims a zip64 locator that the archive is too short to + * hold. Reaching for it has to be a zip error rather than an out of range seek. + */ + @Test + public void testArchiveTooShortForTheZip64LocatorIsRejected() throws IOException { + var archive = zip64Archive(); + // drop everything before the trailing records, leaving the end of central directory (which + // still carries its sentinels) with nothing in front of it + var truncated = Arrays.copyOfRange(archive, archive.length - EOCD_SIZE, archive.length); + + assertThatThrownBy(() -> readArchive(truncated)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("too small to hold the zip64 end of central directory locator"); + } + + private static void readArchive(byte[] archive) throws IOException { + try (var channel = new SeekableInMemoryByteChannel(archive)) { + new ZipReader(channel); + } + } + + /** + * A channel may satisfy a read with fewer bytes than were asked for while more are still + * available. Every multi-byte field the reader parses has to cope with that, or a valid + * archive read through such a channel is rejected as corrupt — and {@code loadTDF} takes a + * caller-supplied channel, so this is reachable from the public API. + */ + @Test + public void testArchiveReadThroughAChannelThatReturnsShortReads() throws IOException { + var archive = zip64Archive(); + + for (int maxRead = 1; maxRead <= 8; maxRead++) { + try (var channel = new ShortReadChannel(new SeekableInMemoryByteChannel(archive), maxRead)) { + var reader = new ZipReader(channel); + assertThat(reader.getEntries()) + .withFailMessage("reading %d byte(s) at a time lost entries", maxRead) + .hasSize(3); + assertThat(readEntry(reader, "0.payload")) + .withFailMessage("reading %d byte(s) at a time corrupted the payload", maxRead) + .isEqualTo(PAYLOAD); + } + } + } + + /** + * Trailing bytes push the end of central directory record back from the end of the archive. + * Everything the reader derives from it — the zip64 locator above all — has to be measured + * from where the record actually is, not from the end of the file. + */ + @Test + public void testZip64ArchiveWithTrailingDataStillReads() throws IOException { + var archive = zip64Archive(); + var padded = Arrays.copyOf(archive, archive.length + 100); + + assertReadsEveryEntry(padded); + } + + /** + * The record can only be pushed back by its own comment, whose length field caps it at 65,535 + * bytes, so the scan stops there. Beyond that limit an archive is not one we can read, and + * saying so immediately is the point: an unbounded scan reads its way back through the whole + * file a byte at a time before reaching the same conclusion. + */ + @Test + public void testEndOfCentralDirectoryPushedBeyondTheCommentLimitIsRejected() throws IOException { + var archive = zip64Archive(); + var padded = Arrays.copyOf(archive, archive.length + 0xFFFF + 1); + + assertThatThrownBy(() -> readArchive(padded)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("Didn't find the end of central directory"); + } + + /** + * The zip64 locator's pointer to the zip64 end of central directory record is a 64-bit value + * taken straight from the archive, so a corrupt one can point anywhere. Following it has to + * be a zip error rather than an out of range seek. + */ + @Test + public void testZip64LocatorPointingOutsideTheArchiveIsRejected() throws IOException { + // the locator sits between the zip64 end of central directory record and the trailing + // end of central directory record; its pointer is 8 bytes in, after the signature and + // the disk number + int pointer = zip64Archive().length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE) + 8; + + for (long badOffset : new long[] { -1L, Long.MIN_VALUE, 1L << 40 }) { + var archive = zip64Archive(); + ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).putLong(pointer, badOffset); + + assertThatThrownBy(() -> readArchive(archive)) + .withFailMessage("an offset of %d should be rejected as a zip error", badOffset) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("outside this"); + } + } + + /** + * The same for the central directory offset the zip64 record carries, which is the other + * 64-bit offset the reader seeks to. + */ + @Test + public void testZip64CentralDirectoryOffsetOutsideTheArchiveIsRejected() throws IOException { + var archive = zip64Archive(); + int zip64Eocd = archive.length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE + ZIP64_EOCD_SIZE); + var buf = ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN); + assertThat(buf.getInt(zip64Eocd)).isEqualTo(ZIP64_EOCD_SIGNATURE); + buf.putLong(zip64Eocd + 48, -1L); + + assertThatThrownBy(() -> readArchive(archive)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("outside this"); + } + + /** + * The largest comment the format allows pushes the end of central directory record as far + * back as it can go. That is the edge of the scan window, and the record still has to be + * found there — and the zip64 locator in front of it, measured from the record rather than + * from the end of the archive. + */ + @Test + public void testZip64ArchiveWithTheLongestPossibleCommentStillReads() throws IOException { + var comment = new byte[0xFFFF]; + Arrays.fill(comment, (byte) 'c'); + + assertReadsEveryEntry(withComment(zip64Archive(), comment)); + } + + /** + * A comment can contain anything, including the end of central directory signature. Scanning + * backwards meets that one first; it has to be passed over because the comment it would + * carry runs off the end of the archive, which the real record's does not. + */ + @Test + public void testSignatureInsideTheCommentIsNotMistakenForTheRecord() throws IOException { + var comment = new byte[64]; + Arrays.fill(comment, (byte) 0xFF); // so the decoy's own comment length reads as 0xFFFF + ByteBuffer.wrap(comment).order(ByteOrder.LITTLE_ENDIAN).putInt(0, EOCD_SIGNATURE); + + // commons-compress takes the decoy for the record, so this one is ours alone + try (var channel = new SeekableInMemoryByteChannel(withComment(zip64Archive(), comment))) { + assertReadsEveryEntry(channel); + } + } + + /** A record whose comment was cut short isn't the end of a whole archive. */ + @Test + public void testRecordWhoseCommentRunsPastTheEndIsRejected() throws IOException { + var withFullComment = withComment(zip64Archive(), new byte[10]); + var truncated = Arrays.copyOf(withFullComment, withFullComment.length - 5); + + assertThatThrownBy(() -> readArchive(truncated)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("Didn't find the end of central directory"); + } + + /** + * Gives the trailing end of central directory record a comment. Assumes, as is true of + * everything our writer produces, that the archive doesn't already have one. + */ + private static byte[] withComment(byte[] archive, byte[] comment) { + var result = Arrays.copyOf(archive, archive.length + comment.length); + ByteBuffer.wrap(result).order(ByteOrder.LITTLE_ENDIAN) + .putShort(archive.length - Short.BYTES, (short) comment.length); + System.arraycopy(comment, 0, result, archive.length, comment.length); + return result; + } + + /** + * A channel is allowed to return zero bytes without being at the end of the stream; only + * {@code -1} means that. An empty read has to be retried, not taken for the end of the + * archive. + */ + @Test + public void testArchiveReadThroughAChannelThatReturnsEmptyReads() throws IOException { + try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 1)) { + assertReadsEveryEntry(channel); + } + } + + /** + * But a channel that never returns anything has to fail, rather than leave the reader + * spinning on it — whether it stops before the archive is opened or partway through an entry. + */ + @Test + public void testChannelThatStopsReturningDataFails() throws IOException { + assertTimeoutPreemptively(Duration.ofSeconds(10), () -> { + try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 0)) { + channel.stalled = true; + assertThatThrownBy(() -> new ZipReader(channel)) + .isInstanceOf(IOException.class) + .hasMessageContaining("no data"); + } + + try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 0)) { + var reader = new ZipReader(channel); + channel.stalled = true; + assertThatThrownBy(() -> readEntry(reader, "0.payload")) + .isInstanceOf(IOException.class) + .hasMessageContaining("no data"); + } + }); + } + + /** + * The zip64 entry count is a 64-bit value taken straight from the archive. One that is + * negative would read as an empty archive, and one too large for the central directory to + * hold would send the reader past its end; both have to be zip errors instead. + */ + @Test + public void testZip64EntryCountTheCentralDirectoryCannotHoldIsRejected() throws IOException { + for (long badCount : new long[] { -1L, Long.MIN_VALUE, 1_000_000L }) { + var archive = zip64Archive(); + int zip64Eocd = archive.length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE + ZIP64_EOCD_SIZE); + var buf = ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN); + assertThat(buf.getInt(zip64Eocd)).isEqualTo(ZIP64_EOCD_SIGNATURE); + buf.putLong(zip64Eocd + 32, badCount); + + assertThatThrownBy(() -> readArchive(archive)) + .withFailMessage("an entry count of %d should be rejected as a zip error", badCount) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("can hold at most"); + } + } + + /** The same bounds check, on the 32-bit offset of an archive that isn't zip64 at all. */ + @Test + public void testCentralDirectoryOffsetOutsideTheArchiveIsRejected() throws IOException { + int length = plainArchive().length; + for (long badOffset : new long[] { length, 0xFFFFFFFEL }) { + var archive = plainArchive(); + ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN) + .putInt(archive.length - EOCD_SIZE + 16, (int) badOffset); + + assertThatThrownBy(() -> readArchive(archive)) + .withFailMessage("an offset of %d should be rejected as a zip error", badOffset) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("outside this"); + } + } + + /** + * An archive with no entries has nothing but its end of central directory record, and its + * central directory offset points right at it. That is still inside the archive. + */ + @Test + public void testEmptyArchiveReads() throws IOException { + var out = new ByteArrayOutputStream(); + new ZipWriter(out).finish(); + + try (var channel = new SeekableInMemoryByteChannel(out.toByteArray())) { + assertThat(new ZipReader(channel).getEntries()).isEmpty(); + } + } + + /** A central directory that ends partway through a filename is a zip error like any other. */ + @Test + public void testCentralDirectoryFilenameRunningPastTheEndIsRejected() throws IOException { + var archive = plainArchive(); + var buf = ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN); + int centralDirectory = buf.getInt(archive.length - EOCD_SIZE + 16); + buf.putShort(centralDirectory + 28, (short) 0x7000); + + assertThatThrownBy(() -> readArchive(archive)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("byte filename"); + } + + /** + * An entry whose data runs past the end of the archive is truncated, and reading it has to + * say so rather than end the stream early and hand back a short entry as if it were whole. + */ + @Test + public void testEntryRunningPastTheEndOfTheArchiveFailsToRead() throws IOException { + var archive = plainArchive(); + var buf = ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN); + int centralDirectory = buf.getInt(archive.length - EOCD_SIZE + 16); + // small.txt comes first; claim it is far bigger than the archive + buf.putInt(centralDirectory + 24, 1_000_000); + + try (var channel = new SeekableInMemoryByteChannel(archive)) { + var reader = new ZipReader(channel); + assertThatThrownBy(() -> readEntry(reader, "small.txt")) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("bytes into the 1000000 byte entry [small.txt]"); + + // and a byte at a time, which takes a different path through the stream + assertThatThrownBy(() -> { + try (var data = reader.getEntries().get(0).getData()) { + while (data.read() != -1) { + // drain + } + } + }).isInstanceOf(InvalidZipException.class) + .hasMessageContaining("byte entry [small.txt]"); + } + } + + /** The same three entries as {@link #zip64Archive()}, written without anything zip64. */ + private static byte[] plainArchive() throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + writer.data("small.txt", "tiny".getBytes(StandardCharsets.UTF_8)); + writer.data("0.payload", PAYLOAD.getBytes(StandardCharsets.UTF_8)); + writer.data("0.manifest.json", MANIFEST.getBytes(StandardCharsets.UTF_8)); + writer.finish(); + var archive = out.toByteArray(); + assertReadsEveryEntry(archive); + return archive; + } + + /** Hands back at most {@code maxRead} bytes per read, as a channel is permitted to do. */ + private static final class ShortReadChannel extends DelegatingChannel { + private final int maxRead; + + ShortReadChannel(SeekableByteChannel delegate, int maxRead) { + super(delegate); + this.maxRead = maxRead; + } + + @Override + public int read(ByteBuffer dst) throws IOException { + if (!dst.hasRemaining()) { + return 0; + } + int limit = dst.limit(); + dst.limit(dst.position() + Math.min(maxRead, dst.remaining())); + try { + return delegate.read(dst); + } finally { + dst.limit(limit); + } + } + } + + /** + * Returns nothing {@code emptyReadsBeforeEachRead} times before every read that does return + * data, as a non-blocking channel is permitted to do, and nothing at all once {@code stalled}. + */ + private static final class StallingChannel extends DelegatingChannel { + private final int emptyReadsBeforeEachRead; + private int emptyReadsSoFar; + volatile boolean stalled; + + StallingChannel(SeekableByteChannel delegate, int emptyReadsBeforeEachRead) { + super(delegate); + this.emptyReadsBeforeEachRead = emptyReadsBeforeEachRead; + } + + @Override + public int read(ByteBuffer dst) throws IOException { + if (stalled) { + return 0; + } + if (emptyReadsSoFar < emptyReadsBeforeEachRead) { + emptyReadsSoFar++; + return 0; + } + emptyReadsSoFar = 0; + return delegate.read(dst); + } + } + + private static class DelegatingChannel implements SeekableByteChannel { + protected final SeekableByteChannel delegate; + + DelegatingChannel(SeekableByteChannel delegate) { + this.delegate = delegate; + } + + @Override + public int read(ByteBuffer dst) throws IOException { + return delegate.read(dst); + } + + @Override + public int write(ByteBuffer src) throws IOException { + return delegate.write(src); + } + + @Override + public long position() throws IOException { + return delegate.position(); + } + + @Override + public SeekableByteChannel position(long newPosition) throws IOException { + delegate.position(newPosition); + return this; + } + + @Override + public long size() throws IOException { + return delegate.size(); + } + + @Override + public SeekableByteChannel truncate(long size) throws IOException { + delegate.truncate(size); + return this; + } + + @Override + public boolean isOpen() { + return delegate.isOpen(); + } + + @Override + public void close() throws IOException { + delegate.close(); + } + } + private static String readEntry(ZipReader reader, String name) throws IOException { var entry = reader.getEntries().stream() .filter(e -> e.getName().equals(name)) diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java index d1d287b2..aa6c3150 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java @@ -15,16 +15,19 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; import java.nio.channels.FileChannel; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; import java.nio.file.StandardOpenOption; +import java.util.Locale; import java.util.Random; import java.util.zip.CRC32; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.assertj.core.api.AssertionsForClassTypes.assertThat; public class ZipWriterTest { @Test @@ -157,6 +160,172 @@ public void rejectsAnOutOfRangeZip64Threshold() { assertThatThrownBy(() -> new ZipWriter(out, 1L << 32)).isInstanceOf(IllegalArgumentException.class); } + /** + * The entry count in the end of central directory record is a 2-byte field. An archive can + * need zip64 for its entry count alone while every one of its entries, and its whole central + * directory, stays comfortably inside 32 bits. + *

+ * The threshold is {@link Short#MAX_VALUE} rather than the {@code 0xFFFE} the format allows, + * so that a reader widening the field with a signed read never sees a negative count. Going + * over it has to produce a real zip64 record, not a count of {@code -32768}. + */ + @Test + public void entryCountAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + var justFits = archiveOfEmptyEntries(Short.MAX_VALUE, ZipWriter.MAX_NON_ZIP64_VALUE); + assertThat(endOfCentralDirectory(justFits).totalEntries).isEqualTo(Short.MAX_VALUE); + assertThat(containsZip64EndOfCentralDirectory(justFits)) + .withFailMessage("an archive whose entry count still fits should not be zip64") + .isFalse(); + // the non-zip64 side of the boundary has to read back too, not just look right + try (var chan = new SeekableInMemoryByteChannel(justFits)) { + assertThat(new ZipReader(chan).getEntries()).hasSize(Short.MAX_VALUE); + } + + var overflows = archiveOfEmptyEntries(Short.MAX_VALUE + 1, ZipWriter.MAX_NON_ZIP64_VALUE); + var eocd = endOfCentralDirectory(overflows); + assertThat(eocd.totalEntries).isEqualTo(0xFFFF); + assertThat(eocd.entriesOnThisDisk).isEqualTo(0xFFFF); + assertThat(containsZip64EndOfCentralDirectory(overflows)) + .withFailMessage("the entry count no longer fits, so the archive has to be zip64") + .isTrue(); + + // and the real count survives, which it only can if it went into the zip64 record + try (var chan = new SeekableInMemoryByteChannel(overflows)) { + assertThat(new ZipReader(chan).getEntries()).hasSize(Short.MAX_VALUE + 1); + } + } + + /** + * The central directory offset is a 4-byte field. Held to the same 2 GiB ceiling as the + * per-entry fields, so the lowered threshold drives it here. + */ + @Test + public void centralDirectoryOffsetAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + // one entry, small enough to stay non-zip64 itself, whose data pushes the start of the + // central directory past the threshold while the directory stays under it + var belowThreshold = archiveOfOneEntry(50, 100); + assertThat(containsZip64EndOfCentralDirectory(belowThreshold)) + .withFailMessage("nothing here crosses the threshold, so the archive should not be zip64") + .isFalse(); + + var archive = archiveOfOneEntry(100, 100); + var eocd = endOfCentralDirectory(archive); + assertThat(eocd.offsetOfCentralDirectory).isEqualTo(ZIP64_SENTINEL); + assertThat(containsZip64EndOfCentralDirectory(archive)) + .withFailMessage("a central directory past the threshold has to be zip64") + .isTrue(); + + assertOnlyTheEndOfCentralDirectoryIsZip64(archive, "big.bin"); + } + + /** + * The central directory size is also a 4-byte field. An empty entry costs 46 bytes in the + * directory but only 30 before it, so a pile of them makes the directory outgrow its own + * offset and this sentinel fires while the offset one does not. + */ + @Test + public void centralDirectorySizeAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + // 5 entries: the directory starts at 180 and is 260 bytes long, both under the threshold + var belowThreshold = archiveOfEmptyEntries(5, 400); + assertThat(containsZip64EndOfCentralDirectory(belowThreshold)) + .withFailMessage("nothing here crosses the threshold, so the archive should not be zip64") + .isFalse(); + + // 10 entries: the directory still starts at 360 but is now 520 bytes long + var archive = archiveOfEmptyEntries(10, 400); + var eocd = endOfCentralDirectory(archive); + assertThat(eocd.sizeOfCentralDirectory).isEqualTo(ZIP64_SENTINEL); + assertThat(containsZip64EndOfCentralDirectory(archive)) + .withFailMessage("a central directory larger than the threshold has to be zip64") + .isTrue(); + + assertOnlyTheEndOfCentralDirectoryIsZip64(archive, "e00000"); + } + + /** + * Zip counts filename lengths in bytes. Deriving one from {@link String#length()}, which + * counts UTF-16 code units, desyncs the central directory by the difference for any entry + * name that isn't pure ASCII. + */ + @Test + public void filenameLengthIsMeasuredInUtf8Bytes() throws IOException { + // a surrogate pair (2 code units, 4 bytes) and a BMP character (1 code unit, 3 bytes), + // so String.length() is 7 where the encoded form is 11 bytes + var name = "🔒両.txt"; + var nameBytes = name.getBytes(StandardCharsets.UTF_8); + assertThat(name.length()).isNotEqualTo(nameBytes.length); + + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + writer.data(name, "contents".getBytes(StandardCharsets.UTF_8)); + writer.finish(); + var archive = out.toByteArray(); + + // the sole local file header starts at 0, and its filename length is at offset 26 + assertThat(readUnsignedShort(archive, 26)).isEqualTo(nameBytes.length); + // the central directory file header keeps its filename length at offset 28 + var centralDirectory = (int) endOfCentralDirectory(archive).offsetOfCentralDirectory; + assertThat(readUnsignedShort(archive, centralDirectory + 28)).isEqualTo(nameBytes.length); + + // and both headers flag the name as UTF-8 (general purpose bit 11), so a reader that goes + // by either one decodes it the same way + assertThat(readUnsignedShort(archive, 6) & UTF8_FLAG) + .withFailMessage("the local file header doesn't flag the name as UTF-8") + .isEqualTo(UTF8_FLAG); + assertThat(readUnsignedShort(archive, centralDirectory + 8) & UTF8_FLAG) + .withFailMessage("the central directory file header doesn't flag the name as UTF-8") + .isEqualTo(UTF8_FLAG); + + try (var chan = new SeekableInMemoryByteChannel(archive)) { + assertThat(readEntry(new ZipReader(chan), name)).isEqualTo("contents"); + } + try (var chan = new SeekableInMemoryByteChannel(archive)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(getDataStream(z, z.getEntry(name)).toString(StandardCharsets.UTF_8)) + .isEqualTo("contents"); + } + } + + /** + * The longest name we write is {@link ZipWriter#MAX_FILENAME_LENGTH} UTF-8 bytes. One byte + * more is rejected on both the byte array and the streaming path — the second is how a TDF + * writes its payload — and a rejected name leaves nothing behind, so the writer can carry on. + */ + @Test + public void rejectsAnEntryNameTooLongToDescribe() throws IOException { + // "両" is 3 bytes, so this lands exactly on the limit only once padded out with ASCII + var longest = "両".repeat(ZipWriter.MAX_FILENAME_LENGTH / 3) + + "x".repeat(ZipWriter.MAX_FILENAME_LENGTH % 3); + assertThat(longest.getBytes(StandardCharsets.UTF_8)).hasSize(ZipWriter.MAX_FILENAME_LENGTH); + var oneTooMany = longest + "x"; + + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + assertThatThrownBy(() -> writer.data(oneTooMany, new byte[0])) + .isInstanceOf(SDKException.class) + .hasMessageContaining("filename length field"); + assertThatThrownBy(() -> writer.stream(oneTooMany)) + .isInstanceOf(SDKException.class) + .hasMessageContaining("filename length field"); + + writer.data(longest, "contents".getBytes(StandardCharsets.UTF_8)); + writer.finish(); + var archive = out.toByteArray(); + + try (var chan = new SeekableInMemoryByteChannel(archive)) { + var reader = new ZipReader(chan); + assertThat(reader.getEntries()) + .withFailMessage("a rejected name should not leave an entry behind") + .hasSize(1); + assertThat(readEntry(reader, longest)).isEqualTo("contents"); + } + try (var chan = new SeekableInMemoryByteChannel(archive)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(getDataStream(z, z.getEntry(longest)).toString(StandardCharsets.UTF_8)) + .isEqualTo("contents"); + } + } + @Test @Disabled("this takes a long time and shouldn't run on build machines") public void testWritingLargeFile() throws IOException { @@ -282,6 +451,87 @@ private static boolean containsZip64EndOfCentralDirectory(byte[] archive) { return false; } + private static final long ZIP64_SENTINEL = 0xFFFFFFFFL; + private static final int UTF8_FLAG = 1 << 11; + private static final int END_OF_CENTRAL_DIRECTORY_SIZE = 22; + private static final int END_OF_CENTRAL_DIRECTORY_SIGNATURE = 0x06054b50; + + /** An archive of {@code count} empty entries, whose names are all the same length. */ + private static byte[] archiveOfEmptyEntries(int count, long threshold) throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out, threshold); + var empty = new byte[0]; + for (int i = 0; i < count; i++) { + writer.data(String.format(Locale.ROOT, "e%05d", i), empty); + } + writer.finish(); + return out.toByteArray(); + } + + /** An archive of one entry named {@code big.bin} carrying {@code dataSize} bytes. */ + private static byte[] archiveOfOneEntry(int dataSize, long threshold) throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out, threshold); + writer.data("big.bin", new byte[dataSize]); + writer.finish(); + return out.toByteArray(); + } + + /** + * Shows that it was an end of central directory field, and not an entry, that made the + * archive zip64: no entry carries a zip64 extra field, and the whole thing still reads. + */ + private static void assertOnlyTheEndOfCentralDirectoryIsZip64(byte[] archive, String anEntryName) + throws IOException { + try (var chan = new SeekableInMemoryByteChannel(archive)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(zip64ExtraField(z, anEntryName)) + .withFailMessage("no entry should be zip64 here, only the end of central directory") + .isNull(); + } + try (var chan = new SeekableInMemoryByteChannel(archive)) { + assertThat(new ZipReader(chan).getEntries()) + .withFailMessage("the archive should still be readable") + .isNotEmpty(); + } + } + + private static int readUnsignedShort(byte[] archive, int position) { + return ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).getShort(position) & 0xFFFF; + } + + /** The fields of the trailing end of central directory record. We never write a comment. */ + private static final class EndOfCentralDirectory { + final int entriesOnThisDisk; + final int totalEntries; + final long sizeOfCentralDirectory; + final long offsetOfCentralDirectory; + + EndOfCentralDirectory(int entriesOnThisDisk, int totalEntries, long sizeOfCentralDirectory, + long offsetOfCentralDirectory) { + this.entriesOnThisDisk = entriesOnThisDisk; + this.totalEntries = totalEntries; + this.sizeOfCentralDirectory = sizeOfCentralDirectory; + this.offsetOfCentralDirectory = offsetOfCentralDirectory; + } + } + + private static EndOfCentralDirectory endOfCentralDirectory(byte[] archive) { + var buf = ByteBuffer + .wrap(archive, archive.length - END_OF_CENTRAL_DIRECTORY_SIZE, END_OF_CENTRAL_DIRECTORY_SIZE) + .order(ByteOrder.LITTLE_ENDIAN); + assertThat(buf.getInt()) + .withFailMessage("the archive doesn't end in an end of central directory record") + .isEqualTo(END_OF_CENTRAL_DIRECTORY_SIGNATURE); + buf.getShort(); // disk number + buf.getShort(); // disk the central directory starts on + return new EndOfCentralDirectory( + buf.getShort() & 0xFFFF, + buf.getShort() & 0xFFFF, + buf.getInt() & 0xFFFFFFFFL, + buf.getInt() & 0xFFFFFFFFL); + } + private static long crcOfWholeFile(File file) throws IOException { var crc = new CRC32(); var buf = new byte[1 << 16];