From 258b803885ed6cfebcd913f372b845d53942519d Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 9 Sep 2026 08:50:12 -0400 Subject: [PATCH] fix(sdk): DSPX-4589 zip64 EOCD sentinels, truncated archive detection, UTF-8 entry names Three zip container conformance fixes found while auditing the TDF zip container against PKWARE APPNOTE.TXT. 1. ZipWriter only set the zip64 flag on the end of central directory record when the entry count exceeded 0xFF or the central directory offset/size exceeded 0xFFFF. Those masks do not match the field widths: the entry count is 2 bytes and the offset and size are 4 bytes each. Archives with between 256 and 65534 entries were needlessly promoted to zip64, and the offset/size checks now go through needsZip64 so they honor the same 2 GiB ceiling as the per-entry fields. 2. ZipReader treated a short read while scanning backwards for the end of central directory signature as a signature match, so a truncated archive could fall out of the scan loop and parse whatever followed as an end of central directory record. It now only breaks on a real match and throws InvalidZipException otherwise, and rejects an archive too small to hold the zip64 locator it claims to have. 3. ZipWriter computed the central directory filename length from String.length() rather than from the UTF-8 encoded byte count. The value was assigned to a field that write() never read, so the bytes on the wire were already correct, but the dead field is removed, the name is encoded once instead of twice, and a name too long for the 2 byte length field is now rejected instead of silently truncated. --- .../io/opentdf/platform/sdk/ZipReader.java | 210 ++++++-- .../io/opentdf/platform/sdk/ZipWriter.java | 84 +++- .../opentdf/platform/sdk/ZipReaderTest.java | 458 +++++++++++++++++- .../opentdf/platform/sdk/ZipWriterTest.java | 252 +++++++++- 4 files changed, 943 insertions(+), 61 deletions(-) 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];