From 6489f5650214c4e9486a66836b97f32bbf3585c1 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 10:25:01 +0200 Subject: [PATCH 01/13] Phase 2: sector checksums, attributes, hi-block and user data headers Sector checksums (P2-3) are Adler-32 seeded with zero, not CRC32 and not standard Adler-32. Both StormLib call sites pass a seed of 0, which starts the accumulators one below where java.util.zip.Adler32 starts them; the results differ by 1 in the low half and by the byte count in the high half for every input. tools/mpqref.py now computes the value independently and caught the first attempt getting it wrong -- a reader and writer that both use the standard seed agree with each other and with nothing else. The checksum chunk is never encrypted and is zlib compressed. Fixing the read path meant fixing the verbatim-copy paths in both the core and the deprecated MpqFile, which decrypted every offset-table gap including that chunk; since a copy clears the encryption flags while keeping SECTOR_CRC, that would have written the corruption back as authoritative. (attributes) (P2-4, #11) is now modelled on its own bytemask rather than an assumed CRC32-plus-FILETIME layout, so a file carrying MD5 digests or patch bits is read instead of misread, and the unexplained -1 in the entry count is gone. Generation is opt-in with a pinnable timestamp. v2-v4 reading (P2-2) gains the hi-block table, compressed hash and block tables, and the version 3 MD5 digests -- reported through MpqArchive.integrity() rather than enforced, as StormLib does. HET/BET is split out as P2-2a and deliberately not attempted: it is not needed to extract StormLib-generated archives, and there is no fixture to verify it against. Also: the user data header is modelled instead of discarded (P2-1), and the header scan no longer commits to an implausible candidate (P2-5b, #47). --- .github/workflows/build.yml | 1 + AUDIT.md | 9 + docs/migration-2.0.md | 42 +- docs/mpq-format-notes.md | 142 +++++ src/main/java/org/inwc3/jmpq/MpqArchive.java | 215 ++++++- .../java/org/inwc3/jmpq/MpqArchiveWriter.java | 96 ++- .../java/org/inwc3/jmpq/MpqAttributes.java | 342 +++++++++++ .../java/org/inwc3/jmpq/MpqChecksums.java | 68 +++ .../java/org/inwc3/jmpq/MpqFileReader.java | 89 ++- src/main/java/org/inwc3/jmpq/MpqHeader.java | 301 +++++++++- .../java/org/inwc3/jmpq/MpqOpenOptions.java | 33 +- .../java/org/inwc3/jmpq/MpqSectorWriter.java | 70 ++- src/main/java/org/inwc3/jmpq/MpqUserData.java | 85 +++ .../java/org/inwc3/jmpq/MpqWriteOptions.java | 119 +++- .../systems/crigges/jmpq3/AttributesFile.java | 111 +++- .../java/systems/crigges/jmpq3/MpqFile.java | 7 +- .../crigges/jmpq3test/LegacyApiTests.java | 15 +- .../crigges/jmpq3test/MpqAttributesTests.java | 199 +++++++ .../crigges/jmpq3test/Phase2FormatTests.java | 547 ++++++++++++++++++ tools/mpqref.py | 30 + 20 files changed, 2404 insertions(+), 117 deletions(-) create mode 100644 src/main/java/org/inwc3/jmpq/MpqAttributes.java create mode 100644 src/main/java/org/inwc3/jmpq/MpqChecksums.java create mode 100644 src/main/java/org/inwc3/jmpq/MpqUserData.java create mode 100644 src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java create mode 100644 src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 4803285..0a0bce9 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -46,6 +46,7 @@ jobs: python tools/mpqref.py verify build/roundtrip/archives --manifest build/roundtrip/expected.tsv python tools/mpqref.py verify build/roundtrip-newcore/archives --manifest build/roundtrip-newcore/expected.tsv python tools/mpqref.py verify build/stored-encrypted/archives --manifest build/stored-encrypted/expected.tsv + python tools/mpqref.py verify build/phase2/archives --manifest build/phase2/expected.tsv # Catches drift between the committed golden manifest and the fixtures. - name: Check the golden manifest is up to date diff --git a/AUDIT.md b/AUDIT.md index 1b68fb8..6d3aaee 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -79,12 +79,21 @@ These are real bugs or hazards in behaviour that must not be carried into the ne ## Phase 2 — Format completeness - **P2-1 Polish v0/v1 read+write.** Correct v1 header round-trip (hi-word hash/block positions, 64-bit `archiveSize` handling), `Block.filePos` kept as `long` end-to-end (today `getFilePos()` truncates to int — BlockTable.java:88), user-data header (`MPQ\x1B`) parsed into a model instead of skipped (JMpqEditor.java:516 TODO), header-search alignment verified against StormLib. + - Done. `Block.filePos` is `long` throughout and the truncating `getFilePos()` is deprecated in favour of `getFilePosition()`; the user data header is modelled as `MpqUserData`, with its payload readable rather than discarded. - **P2-2 v2–v4 read support.** 64-bit table offsets, hash/block table hi-word arrays, **HET/BET tables** (encrypted + compressed variants), v4 MD5 validation of header/tables, compressed block/hash tables. Acceptance: open and fully extract StormLib-generated v2, v3, v4 archives byte-identically. + - Done via the classic tables: 64-bit and hi-word table offsets, the hi-block table (`MAKE_OFFSET64`, and unlike the other tables neither encrypted nor compressed), compressed hash/block tables detected from the version 3 stored-length fields, and the version 3 MD5 digests checked and reported through `MpqArchive.integrity()`. See format notes 12 and 13. + - **Not verified against real fixtures.** The acceptance criterion asks for StormLib-generated v2/v3/v4 archives; there are none in the repository and no StormLib build available to produce them. What exists is exercised with synthetic fixtures, which is weaker evidence and should not be read as the criterion being met. Generating fixtures with `smpq`/StormLib is the outstanding work. +- **P2-2a HET/BET tables -- deferred** *(split out of P2-2)*. Not implemented. Two reasons, in order of weight. First, it is not needed for the acceptance criterion above: StormLib writes classic hash and block tables alongside HET/BET for v2-v4 archives, so extraction goes through the path that now works, and HET/BET is only *required* for an archive that omits the classic tables. Second, writing it blind is the failure mode this project keeps hitting -- an implementation derived from the spec, verified against a fixture built from the same reading of that spec, proves only self-consistency. Format note 9 is a worked example of that trap costing real correctness. Do this once a StormLib-generated fixture exists. - **P2-3 Sector CRC flag (0x04000000).** Verify per-sector ADLER/CRC on read when flag present; option to emit on write. Wire into extraction pipeline, not bolted onto `MpqFile`. + - Done, in `MpqFileReader` on read and `MpqSectorWriter` on write, opt-in via `MpqWriteOptions.withSectorChecksums`. The checksums are Adler-32 **seeded with zero** -- not CRC32, and not standard Adler-32; see format notes 9 and 10, which also record what that distinction cost. Verification is on by default when reading and can be turned off to recover damaged archives. - **P2-4 `(attributes)` write support** *(issue [#11](https://github.com/inwc3/JMPQ3/issues/11))*. Honour the attributes bytemask properly on read (today hardcodes crc+timestamp layout and has a suspicious `-1` in the entry count — AttributesFile.java:38); regenerate CRC32+FILETIME on write when requested. Remove the dead commented block in `JMpqEditor.close`. Historical context from the issue thread: generation was disabled because CRC32 differed from StormLib for some `.wav` files — root cause is likely multi-compression handling (first sector of ADPCM-compressed wavs is not ADPCM-compressed since it holds the wav header). Fix alongside P2-6 and pin with a StormLib-golden CRC test. Acceptance for the issue itself: load + close `war3.mpq`-style archives without dropping `(attributes)` in a way the game rejects. + - Done. `MpqAttributes` reads every array the bytemask declares and accepts the entry counts StormLib tolerates; the unexplained `-1` is gone from the deprecated parser too. Generation is opt-in via `MpqWriteOptions.withAttributes`, with a pinnable timestamp so builds stay reproducible. The `.wav` CRC32 concern from the issue thread does not arise: the checksum is taken over decoded content, and the multi-compression ordering it depended on was fixed in P2-6. - **P2-5a Tolerant header parsing for real-world (protected) maps** *(issue [#46](https://github.com/inwc3/JMPQ3/issues/46))*. 59/857 sampled maps fail with "Bad header size": `readHeaderSize` hard-rejects `headerSize < 32 || > 208` (JMpqEditor.java:443) even though the game itself ignores the field for v0 archives. Mirror StormLib's leniency: derive the effective header size from the format version, clamp/ignore garbage values, and treat other header fields defensively (this also removes most of the need for consumers to pass `FORCE_V0`). Acceptance: the Forest Defense sample from the issue opens and extracts. + - Done in Phase 0, as a side effect of modelling the header: `MpqHeader` repairs rather than rejects, following `ConvertMpqHeaderToFormat4`. - **P2-5b Fake-header protection resilience — nice to have** *(issue [#47](https://github.com/inwc3/JMPQ3/issues/47))*. Some protected maps plant decoy `MPQ\x1A` headers so `searchHeader` either accepts a bogus one or gives up. Approach: on finding a candidate header, validate it (plausible table positions/sizes within file) and keep scanning on failure instead of committing to the first match. Best-effort only — full protected-map support is explicitly not a goal; skip if it destabilises normal parsing. + - Done. The plausibility test now applies to every candidate -- archive headers and user-data redirects alike -- rather than only in `FORCE_V0` mode, and the first candidate is kept as a fallback, so the scan can only ever find a header where the old one found one, never fewer. - **P2-6 Complete decompression matrix.** Add BZIP2, SPARSE, LZMA (xz dep is already on the classpath and unused), and correct multi-compression ordering (ADPCM+Huffman path exists; verify against StormLib order). Compression write side stays deflate (+ zopfli option), but the sector-type byte handling must be table-driven per spec. + - Done in Phase 0: dispatch is table-driven off StormLib's `dcmp_table`, and format note 2 records why `0x12` cannot be tested as a mask. ## Phase 3 — Code hygiene & dependencies diff --git a/docs/migration-2.0.md b/docs/migration-2.0.md index 1d0710c..0622f15 100644 --- a/docs/migration-2.0.md +++ b/docs/migration-2.0.md @@ -97,9 +97,45 @@ replaced by the version's real size and the archive opens with `header().malformed()` set, matching what Storm.dll does. 1.x refused unless you passed `FORCE_V0`. -**Hi-block tables are refused.** Archives placing file data beyond 4 GiB are -rejected explicitly rather than misread. Support arrives with the version 2 to 4 -read work. +**Hi-block tables are read.** Archives placing file data beyond 4 GiB have their +file positions extended from the hi-block table, as StormLib does. A hi-block +table that falls outside the file is ignored and the archive flagged malformed, +rather than refused. + +**Sector checksums are verified by default.** Where an archive records an +Adler-32 per sector, a mismatch now fails the read instead of returning bytes +known to be wrong. 1.x ignored the checksums entirely. If you would rather +recover what is still intact from a damaged archive, turn it off: + +```java +MpqArchive.open(path, MpqOpenOptions.defaults() + .withSectorChecksumVerification(false)); +``` + +**Attributes are parsed properly.** `archive.attributes()` returns a +`MpqAttributes` honouring the file's own bytemask, so archives carrying MD5 +digests or patch bits are read rather than misread. The 1.x `AttributesFile` +assumed one fixed layout and reported one entry fewer than the file held. + +## Recording metadata on write + +Both are opt-in, because both change the bytes of every file written and +neither is needed for a valid archive. Warcraft III wants neither; StormLib +normally writes both. + +```java +MpqWriteOptions.defaults() + .withSectorChecksums(true) // Adler-32 per sector + .withAttributes(true) // generate (attributes) + .withAttributesTimestamp(buildTimestampMillis) // pin it, or the build is not reproducible +``` + +Two things to know. Generating `(attributes)` requires a CRC32 over each file's +decoded content, so it forces a decode of files that would otherwise have been +copied verbatim — enabling it costs real time on a large rebuild. And supplying +your own `(attributes)` while asking for generation is refused rather than +producing two entries under one name; supplying it alone stays legal, which is +how you preserved it before generation existed. ## Protection tooling diff --git a/docs/mpq-format-notes.md b/docs/mpq-format-notes.md index 8501ad5..dd6f790 100644 --- a/docs/mpq-format-notes.md +++ b/docs/mpq-format-notes.md @@ -213,3 +213,145 @@ short. Accepting it would hand back the missing tail as zeros at exactly the length the caller expected, so the corruption would pass every downstream check. Nothing is lost by being strict here: JMPQ3 never writes sparse sectors, so the only streams affected are genuinely damaged. + + +## 9. Sector checksums are Adler-32 seeded with zero, not one + +The flag is `MPQ_FILE_SECTOR_CRC` (`0x04000000`) and the StormLib field is +`SectorChksums`, so "CRC32" is the natural reading. It is wrong twice over. + +StormLib computes the value with zlib's `adler32`, and it passes a seed of `0`: + +```c +// SFileReadFile.cpp, ReadMpqSectors +DWORD dwAdlerExpected = hf->SectorChksums[dwIndex]; +// We can only check sector CRC when it is not zero +// Neither can we check it if it is 0xFFFFFFFF. +if(dwAdlerExpected != 0 && dwAdlerExpected != 0xFFFFFFFF) +{ + dwAdlerValue = adler32(0, pbInSector, dwRawBytesInThisSector); + if(dwAdlerValue != dwAdlerExpected) + { dwErrCode = ERROR_CHECKSUM_ERROR; break; } +} + +// SFileAddFile.cpp, on the write side +hf->SectorChksums[dwSectorIndex] = adler32(0, pbCompressed, nOutBuffer); +``` + +A *standard* Adler-32 starts its accumulators at `s1 = 1, s2 = 0`; seeding zlib +with `0` starts them at `s1 = 0, s2 = 0`. The two results differ by 1 in the low +half and by the byte count in the high half — for every input, without +exception. + +**Decision.** `MpqChecksums.adler32` implements the seeded-zero form. +`java.util.zip.Adler32` cannot be used: it offers no way to seed, so it always +computes the standard variant. + +This one is worth dwelling on, because no self-consistent test can catch it. A +reader and a writer that both use the standard seed agree with each other on +every archive they exchange, and disagree with every archive StormLib ever +wrote. It was caught only by `tools/mpqref.py`, which derives the value +independently, and it is the reason that cross-check is in CI rather than being +a one-off. + +**Bytes covered.** Both quotes above take the checksum over the sector *as +stored, minus its encryption* — after decrypting, before decompressing, and +including the compression-type byte. The read and write sides therefore agree +without either needing to know how the sector was compressed. + +**Absent values.** `0` and `0xFFFFFFFF` both mean "not recorded" and are skipped, +so a file may legitimately carry the flag and no usable checksums. + + +## 10. The checksum chunk is never encrypted, and is zlib compressed + +The checksums live in a chunk after the data sectors, delimited by the last two +entries of the sector offset table — which is why a `SECTOR_CRC` file has one +more offset entry than sectors plus one. + +Two properties are easy to get wrong: + +- **Never encrypted**, even when every data sector is. StormLib writes it + without encrypting, and loads it with `LoadMpqTable(..., 0, ...)` — key `0`, + meaning no decryption. An encrypted file therefore has encrypted sectors and a + plain checksum chunk side by side. +- **Zlib compressed** when that is smaller, detected the same way a sector's + compression is: the stored length being shorter than the natural length of + `sectorCount * 4` bytes. + +**Decision.** `MpqFileReader.readSectorChecksums` and +`MpqSectorWriter.encodeChecksums` follow both. The verbatim-copy path +(`storedBytesDecrypted`) iterates only the *data* sectors when decrypting, and +deliberately leaves the final chunk alone: decrypting it there corrupted it, and +because the copy clears the encryption flags while keeping `SECTOR_CRC`, the +corruption became permanent in the rebuilt archive. + + +## 11. The `(attributes)` bytemask decides the layout, and several lengths are legal + +``` +0x00 u32 version, always 100 +0x04 u32 bytemask +0x08 u32 crc32 [n] when 0x01 + u64 fileTime [n] when 0x02 + u8x16 md5 [n] when 0x04 + bits patch [n] when 0x08 +``` + +`n` is the **block table** size, so the arrays are indexed by block index and +include the `(attributes)` file's own row — whose checksum cannot be computed +and is left `0`, which reads back as "not recorded". + +Two things the pre-2.0 parser got wrong. It read the bytemask and then assumed +CRC32-plus-FILETIME regardless, so any file carrying MD5 digests was misread. +And it derived the count as `(length - 8) / 12 - 1`; the `- 1` has no basis in +the layout. Its likely origin is that StormLib *tolerates* an attributes file one +entry short — the tool that writes it is rarely the tool reading it — so +somebody met a short file and hardcoded the short case. + +**Decision.** `MpqAttributes.parse` computes the expected length from the +declared bytemask and accepts either `n` or `n - 1` entries, reporting which via +`truncated()`. A length matching neither is an error rather than a guess. Bits +outside the four known ones are preserved in `flags()` but their arrays cannot be +located, so parsing stops after the known prefix — as StormLib does. + +The patch-bit array is `(n + 6) / 8` bytes, which is StormLib's own formula: it +rounds up and then tolerates a spare byte, rather than the `(n + 7) / 8` you +would expect. + + +## 12. The hi-block table is plain; the hash and block tables may not be + +The hi-block table supplies bits 32 to 47 of each file position, one `u16` per +block entry, combined as StormLib's `MAKE_OFFSET64(hi, low)`. StormLib's comment +in `BuildFileTable_Classic` is explicit: *"Load the hi-block table. It is not +encrypted, nor compressed."* + +The hash and block tables are the opposite: always encrypted, and from format +version 3 optionally compressed. Nothing in the position fields says whether a +table is compressed — the version 3 header carries each table's *stored* length +separately, and a stored length shorter than the entry count implies is what +marks it compressed. + +**Decision.** `MpqHeader.hashTableStoredSize` / `blockTableStoredSize` return the +stored length, and `isHashTableCompressed` / `isBlockTableCompressed` compare it +against the plain length. `MpqArchive.loadTable` decrypts and *then* decompresses, +which is the order StormLib's `LoadMpqTable` uses because the writer compresses +before encrypting. + +A hi-block table whose position falls outside the file is dropped and the archive +flagged malformed, rather than refused: reading the low words alone is exactly +what a version 0 reader does, and the archive is otherwise fine. + + +## 13. Version 3 MD5 digests are reported, not enforced + +A version 3 header carries six MD5 digests — header, hash table, block table, +hi-block table, HET and BET. StormLib checks them and reports the result; it does +not refuse the archive. + +**Decision.** `MpqArchive.integrity()` returns `UNRECORDED`, `VERIFIED` or +`MISMATCHED`, and a mismatch is logged. Refusing to open would throw away an +archive whose tables may decode every file perfectly. An all-zero digest counts +as "not recorded" rather than as the digest of those bytes. + diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index 33c9db0..5c72a16 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -6,6 +6,7 @@ import systems.crigges.jmpq3.JMpqException; import systems.crigges.jmpq3.Listfile; import systems.crigges.jmpq3.MpqNames; +import systems.crigges.jmpq3.compression.CompressionUtil; import systems.crigges.jmpq3.security.MPQEncryption; import systems.crigges.jmpq3.security.MPQHashGenerator; @@ -78,6 +79,9 @@ private static int tableKey(String name) { private final MpqFileReader reader; private final short defaultLocale; + /** Whether the tables matched the digests a version 3 header records. */ + private final Integrity integrity; + /** Block table rows, indexed as the hash table addresses them. */ private final MpqFileEntry[] blocks; @@ -107,20 +111,31 @@ private MpqArchive(MpqSource source, MpqOpenOptions options) throws IOException this.source = source; this.defaultLocale = options.defaultLocale(); this.header = MpqHeader.parse(source, options.forceV0()); - if (header.hiBlockTablePosition() != 0) { - // A hi-block table holds the upper 16 bits of each file position, - // for archives whose data passes 4 GiB. Reading only the low word - // would seek to the wrong place, so refuse rather than silently - // misread. Supporting it belongs with the v2-v4 read work (P2-2). - throw new JMpqException("This archive uses a hi-block table, for file positions" - + " beyond 4 GiB, which is not supported yet."); - } - this.reader = new MpqFileReader(source, header); + this.reader = new MpqFileReader(source, header, options.verifySectorChecksums()); this.blocks = readBlockTable(); this.hashTable = readHashTable(); + this.integrity = checkTableDigests(); readNames(); } + /** + * Whether the archive's tables matched the MD5 digests it recorded for + * them. + *

+ * Only a version 3 archive records any, and StormLib treats a mismatch as + * something to report rather than as a reason to refuse the archive — the + * tables may still be perfectly readable. So this is exposed for a caller + * that wants to know, and never fails the open. + */ + public enum Integrity { + /** No digests were recorded, so nothing could be checked. */ + UNRECORDED, + /** Every recorded digest matched. */ + VERIFIED, + /** At least one recorded digest did not match its table. */ + MISMATCHED + } + /** * Opens an archive file, mapping it for reading. * @@ -191,6 +206,90 @@ public MpqHeader header() { return header; } + /** + * @return whether the archive's tables matched the digests it recorded for + * them. Only a version 3 archive records any. + */ + public Integrity integrity() { + return integrity; + } + + /** + * The user data header this archive sits behind, if any. + *

+ * Blizzard staples metadata in front of an archive this way — a StarCraft II + * map keeps its map info there. Exposing it lets a caller read that payload, + * and lets a rebuild preserve it rather than dropping it. + * + * @return the user data header, or empty when the archive starts the file. + */ + public Optional userData() { + return Optional.ofNullable(header.userData()); + } + + /** + * The archive's {@code (attributes)} file, parsed. + *

+ * Its arrays are indexed by block table index, so + * {@link MpqAttributes#crc32Of(int)} pairs with + * {@link MpqFileEntry#blockIndex()}. + * + * @return the attributes, or empty when the archive carries none or they + * cannot be read as attributes for an archive of this size. + */ + public Optional attributes() { + if (!contains(MpqAttributes.NAME)) { + return Optional.empty(); + } + try { + return Optional.of(MpqAttributes.parse(read(MpqAttributes.NAME), blocks.length)); + } catch (IOException | RuntimeException unreadable) { + // Attributes are advisory: an archive whose attributes will not + // parse is still a perfectly good archive, and the pre-2.0 code + // silently misparsed them rather than saying so. + log.warn("{} has an unreadable (attributes): {}", + source.origin(), unreadable.getMessage()); + return Optional.empty(); + } + } + + /** + * Checks the tables against the MD5 digests a version 3 header records. + *

+ * Reported rather than enforced, as StormLib does: a mismatch means the + * digests and the tables disagree, but the tables may still decode every + * file. Refusing the archive would lose data that is actually recoverable. + */ + private Integrity checkTableDigests() throws IOException { + final MpqHeader.Extended extended = header.extended(); + if (!extended.hasDigests()) { + return Integrity.UNRECORDED; + } + + boolean matched = header.verifyHeaderDigest(source); + matched &= MpqHeader.matchesDigest( + source.bytes(header.hashTableFileOffset(), (int) header.hashTableStoredSize()), + extended.md5HashTable()); + matched &= MpqHeader.matchesDigest( + source.bytes(header.blockTableFileOffset(), (int) header.blockTableStoredSize()), + extended.md5BlockTable()); + if (header.hasHiBlockTable()) { + final long bytes = (long) header.blockTableEntries() * MpqHeader.HI_BLOCK_ENTRY_SIZE; + if (source.contains(header.hiBlockTableFileOffset(), bytes)) { + matched &= MpqHeader.matchesDigest( + source.bytes(header.hiBlockTableFileOffset(), (int) bytes), + extended.md5HiBlockTable()); + } + } + + if (!matched) { + log.warn("{} does not match the MD5 digests in its own header;" + + " its tables may be damaged.", source.origin()); + return Integrity.MISMATCHED; + } + return Integrity.VERIFIED; + } + /** * @return the number of block table rows in use. */ @@ -404,6 +503,46 @@ private MpqFileEntry require(String name, short locale) throws IOException { () -> new JMpqException("No such file in " + source.origin() + ": <" + name + ">")); } + /** + * Loads one of the archive's tables: read, decrypt, and decompress if it + * was stored compressed. + *

+ * From format version 3 a table may be zlib-compressed, which the position + * fields alone cannot express — the header carries the stored length + * separately, and a stored length shorter than the entries imply is what + * marks it compressed. The order matters and is StormLib's + * {@code LoadMpqTable}: decrypt first, then decompress, because the writer + * compresses and then encrypts. + * + * @param fileOffset where the table starts. + * @param storedSize bytes it occupies in the file. + * @param plainSize bytes it occupies once decoded. + * @param key decryption key, or 0 for an unencrypted table. + * @return exactly {@code plainSize} bytes. + */ + private byte[] loadTable(long fileOffset, long storedSize, long plainSize, int key) + throws IOException { + if (plainSize > Integer.MAX_VALUE - 8 || storedSize > Integer.MAX_VALUE - 8) { + throw new JMpqException("A table of " + plainSize + " bytes is larger than can be" + + " held in memory."); + } + final byte[] stored = source.bytes(fileOffset, (int) storedSize); + + if (key != 0) { + new MPQEncryption(key, true).processSingle(ByteBuffer.wrap(stored)); + } + if (storedSize >= plainSize) { + return stored; + } + final byte[] plain = CompressionUtil.decompress(stored, (int) storedSize, (int) plainSize, + header.formatVersion()); + if (plain.length != plainSize) { + throw new JMpqException("A compressed table at " + fileOffset + " decoded to " + + plain.length + " bytes rather than the " + plainSize + " expected."); + } + return plain; + } + private MpqFileEntry[] readBlockTable() throws IOException { final int count = header.blockTableEntries(); // long arithmetic: a 2 GiB archive can describe enough block entries @@ -414,33 +553,69 @@ private MpqFileEntry[] readBlockTable() throws IOException { throw new JMpqException("Block table of " + count + " entries needs " + tableBytes + " bytes, more than can be held in memory."); } - final byte[] encrypted = source.bytes(header.blockTableFileOffset(), (int) tableBytes); + final ByteBuffer plain = ByteBuffer + .wrap(loadTable(header.blockTableFileOffset(), header.blockTableStoredSize(), + tableBytes, KEY_BLOCK_TABLE)) + .order(ByteOrder.LITTLE_ENDIAN); - final ByteBuffer plain = ByteBuffer.allocate(encrypted.length).order(ByteOrder.LITTLE_ENDIAN); - new MPQEncryption(KEY_BLOCK_TABLE, true).processFinal(ByteBuffer.wrap(encrypted), plain); - plain.rewind(); + final int[] highWords = readHiBlockTable(count); final MpqFileEntry[] entries = new MpqFileEntry[count]; for (int i = 0; i < count; i++) { - final long filePosition = Integer.toUnsignedLong(plain.getInt()); + long filePosition = Integer.toUnsignedLong(plain.getInt()); final int compressedSize = plain.getInt(); final int normalSize = plain.getInt(); final int flags = plain.getInt(); + if (highWords.length > 0) { + filePosition |= (long) highWords[i] << 32; + } entries[i] = new MpqFileEntry("", (short) 0, flags, filePosition, compressedSize, normalSize, i); } return entries; } + /** + * The hi-block table: the upper 16 bits of each file position, for archives + * whose data passes 4 GiB. + *

+ * One {@code u16} per block entry, combined as StormLib's + * {@code MAKE_OFFSET64(hi, low)}. Unlike the hash and block tables it is + * "not encrypted, nor compressed" — StormLib's own comment — so it is read + * straight through. + * + * @param blockCount how many entries to expect. + * @return one high word per block, or an empty array when the archive has + * no hi-block table. + */ + private int[] readHiBlockTable(int blockCount) throws IOException { + if (!header.hasHiBlockTable() || blockCount == 0) { + return new int[0]; + } + final long tableBytes = (long) blockCount * MpqHeader.HI_BLOCK_ENTRY_SIZE; + if (!source.contains(header.hiBlockTableFileOffset(), tableBytes)) { + // The archive claims a hi-block table it does not hold. Reading the + // low words alone at least matches what a version 0 reader sees. + log.warn("{} declares a hi-block table at {} that does not fit; ignoring it.", + source.origin(), header.hiBlockTableFileOffset()); + return new int[0]; + } + final int[] highWords = new int[blockCount]; + for (int i = 0; i < blockCount; i++) { + highWords[i] = source.u16(header.hiBlockTableFileOffset() + + (long) i * MpqHeader.HI_BLOCK_ENTRY_SIZE); + } + return highWords; + } + private HashTable readHashTable() throws IOException { // Bounded by MpqHeader at MAX_HASH_TABLE_ENTRIES, so this cannot // overflow: 0x80000 * 16 is 8 MiB. - final byte[] encrypted = source.bytes(header.hashTableFileOffset(), - header.hashTableEntries() * MpqHeader.HASH_ENTRY_SIZE); - - final ByteBuffer plain = ByteBuffer.allocate(encrypted.length).order(ByteOrder.LITTLE_ENDIAN); - new MPQEncryption(KEY_HASH_TABLE, true).processFinal(ByteBuffer.wrap(encrypted), plain); - plain.rewind(); + final long tableBytes = (long) header.hashTableEntries() * MpqHeader.HASH_ENTRY_SIZE; + final ByteBuffer plain = ByteBuffer + .wrap(loadTable(header.hashTableFileOffset(), header.hashTableStoredSize(), + tableBytes, KEY_HASH_TABLE)) + .order(ByteOrder.LITTLE_ENDIAN); final HashTable table = new HashTable(header.hashTableEntries()); table.readFromBuffer(plain); diff --git a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java index d617069..646ca25 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java @@ -54,13 +54,14 @@ public final class MpqArchiveWriter { private static final int KEY_BLOCK_TABLE = tableKey("(block table)"); /** - * Internal files the writer generates itself, so a caller cannot supply - * them: doing so would put two entries under one name. + * Internal files the writer always generates itself, so a caller cannot + * supply them: doing so would put two entries under one name. *

- * Only {@code (listfile)} qualifies. {@code (attributes)} and - * {@code (signature)} are not generated, so a caller holding their bytes is - * free to write them as ordinary files -- which is the only way to preserve - * them until attributes generation lands. + * Only {@code (listfile)} is unconditional. {@code (attributes)} is + * generated only when asked for, so supplying it stays legal otherwise and + * is refused at build time when both are requested. {@code (signature)} is + * never generated, so a caller holding those bytes may write them as an + * ordinary file. */ private static final List GENERATED = List.of("(listfile)"); @@ -72,6 +73,16 @@ public final class MpqArchiveWriter { private static final List NOT_CARRIED_OVER = List.of("(listfile)", "(attributes)", "(signature)"); + /** + * Flags the writer gives the internal files it generates, matching what + * StormLib gives them: compressed, encrypted, and with the key adjusted for + * position so moving the file invalidates it. + */ + private static final int INTERNAL_FILE_FLAGS = MpqFileEntry.FLAG_EXISTS + | MpqFileEntry.FLAG_COMPRESSED + | MpqFileEntry.FLAG_ENCRYPTED + | MpqFileEntry.FLAG_ADJUSTED_KEY; + private static int tableKey(String name) { final MPQHashGenerator hasher = MPQHashGenerator.getFileKeyGenerator(); hasher.process(name); @@ -380,10 +391,23 @@ private MpqImageBuffer build() throws IOException { // Name plus locale, because the hash table needs both and a path // may appear once per locale. - final List written = new ArrayList<>(pending.size() + 1); - final List blocks = new ArrayList<>(pending.size() + 1); + if (options.writeAttributes() && contains(MpqAttributes.NAME)) { + throw new JMpqException("Cannot both generate " + MpqAttributes.NAME + + " and write a supplied one: the archive would hold two entries under that" + + " name. Either drop the supplied file or turn attributes generation off."); + } + + final List written = new ArrayList<>(pending.size() + 2); + final List blocks = new ArrayList<>(pending.size() + 2); + // One CRC32 per block, in block order, for the (attributes) file. Left + // empty when attributes are not requested, so nothing is decoded for + // the sake of a checksum nobody asked for. + final List checksums = new ArrayList<>(pending.size() + 2); for (Pending file : pending.values()) { + // Taken before writing, because a verbatim copy never decodes the + // file and the checksum is over its decoded content. + checksums.add(options.writeAttributes() ? crc32(contentOf(file)) : 0); blocks.add(writeFile(image, base, file, sectorSize)); written.add(new Written(file.name(), file.locale())); } @@ -393,9 +417,21 @@ private MpqImageBuffer build() throws IOException { // enumerated, so a later rebuild would lose every name. final byte[] listfile = buildListfile(written); written.add(new Written("(listfile)", MpqOpenOptions.NEUTRAL_LOCALE)); + checksums.add(options.writeAttributes() ? crc32(listfile) : 0); blocks.add(writeEncoded(image, base, "(listfile)", listfile, sectorSize, - MpqFileEntry.FLAG_EXISTS | MpqFileEntry.FLAG_COMPRESSED - | MpqFileEntry.FLAG_ENCRYPTED | MpqFileEntry.FLAG_ADJUSTED_KEY)); + INTERNAL_FILE_FLAGS)); + } + + if (options.writeAttributes()) { + // Its own arrays have to be sized before it is written, and they + // cover every block table row -- including this file's own and any + // spare slots. Its own checksum stays 0, which cannot be computed + // without knowing it, and which readers treat as "not recorded". + final int rows = blocks.size() + 1 + options.extraBlockEntries(); + final byte[] attributes = buildAttributes(checksums, rows); + written.add(new Written(MpqAttributes.NAME, MpqOpenOptions.NEUTRAL_LOCALE)); + blocks.add(writeEncoded(image, base, MpqAttributes.NAME, attributes, sectorSize, + INTERNAL_FILE_FLAGS)); } final int hashCapacity = hashTableCapacity(written.size()); @@ -466,12 +502,50 @@ private BlockRow copyVerbatim(MpqImageBuffer image, int base, String name, private BlockRow writeEncoded(MpqImageBuffer image, int base, String name, byte[] content, int sectorSize, int requestedFlags) { final long position = (long) image.position() - base; - final int flags = requestedFlags == 0 ? MpqSectorWriter.flagsFor(content.length) : requestedFlags; + int flags = requestedFlags == 0 + ? MpqSectorWriter.flagsFor(content.length, options.sectorChecksums()) + : requestedFlags; + if (options.sectorChecksums() && content.length > 0) { + flags |= MpqFileEntry.FLAG_SECTOR_CRC; + } final int compressedSize = MpqSectorWriter.write(image, content, sectorSize, name, flags, position, options.recompression()); return new BlockRow(position, compressedSize, content.length, flags); } + /** + * Builds the {@code (attributes)} content. + *

+ * The arrays are indexed by block table row and cover every row the archive + * will have, so spare slots get a zero checksum and a zero timestamp -- + * which is what "not recorded" looks like to a reader. + * + * @param checksums one CRC32 per block written so far. + * @param rows total block table rows the archive will declare. + * @return the attributes file content. + */ + private byte[] buildAttributes(List checksums, int rows) { + final int[] crc = new int[rows]; + final long[] times = new long[rows]; + final long now = options.metadata().fileTime(); + for (int i = 0; i < rows; i++) { + final boolean real = i < checksums.size(); + crc[i] = real ? checksums.get(i) : 0; + times[i] = real ? now : 0; + } + return MpqAttributes.build(crc, times); + } + + /** + * @param content a file's decoded bytes. + * @return its zlib CRC32, the value {@code (attributes)} records. + */ + private static int crc32(byte[] content) { + final java.util.zip.CRC32 digest = new java.util.zip.CRC32(); + digest.update(content); + return (int) digest.getValue(); + } + private byte[] contentOf(Pending file) throws IOException { return switch (file.content()) { case Content.Bytes bytes -> bytes.value(); diff --git a/src/main/java/org/inwc3/jmpq/MpqAttributes.java b/src/main/java/org/inwc3/jmpq/MpqAttributes.java new file mode 100644 index 0000000..b800e44 --- /dev/null +++ b/src/main/java/org/inwc3/jmpq/MpqAttributes.java @@ -0,0 +1,342 @@ +package org.inwc3.jmpq; + +import systems.crigges.jmpq3.JMpqException; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Arrays; + +/** + * The optional {@code (attributes)} file: per-block CRC32, timestamp and MD5. + * + *

Layout

+ * 
+ * 0x00 u32  version, always 100
+ * 0x04 u32  bytemask of which arrays follow
+ * 0x08      u32   crc32     [blockCount]   when CRC32 is set
+ *           u64   fileTime  [blockCount]   when FILETIME is set
+ *           u8x16 md5       [blockCount]   when MD5 is set
+ *           bits  patch     [blockCount]   when PATCH_BIT is set
+ *
+ * Every array is indexed by block table index, not by file name, and + * holds one entry per block table row — including the {@code (attributes)} row + * itself, whose own checksum cannot be computed and is left zero. + * + *

Why this replaces the old parser

+ * The pre-2.0 {@code AttributesFile} assumed the CRC32 and FILETIME arrays were + * both present and no others, deriving the entry count as + * {@code (length - 8) / 12 - 1}. That is wrong in three ways: it ignores the + * bytemask it just read, it misreads any archive carrying MD5s, and the + * {@code - 1} hardcodes one of the several lengths StormLib tolerates rather + * than working out which one this file actually is. + *

+ * StormLib does accept a short attributes file: it is typically written by a + * different tool than the one reading it, and being one entry shy is common + * enough that refusing it would reject working archives. So the count is + * resolved by matching the declared bytemask against the plausible lengths, and + * a file matching none is reported rather than silently misparsed. + * + * @param version declared format version; 100 is the only known value. + * @param flags the bytemask, as stored. + * @param crc32 zlib CRC32 per block, empty when not present. + * @param fileTimes Windows FILETIME per block, empty when not present. + * @param md5 16 bytes per block, empty when not present. + * @param patchBits one flag per block, empty when not present. + * @param truncated whether the file held one fewer entry than the block table. + */ +public record MpqAttributes( + int version, + int flags, + int[] crc32, + long[] fileTimes, + byte[][] md5, + boolean[] patchBits, + boolean truncated) { + + /** The name under which an archive carries its attributes. */ + public static final String NAME = "(attributes)"; + + /** The only format version StormLib writes or accepts. */ + public static final int VERSION = 100; + + /** A zlib CRC32 per block follows. */ + public static final int HAS_CRC32 = 0x01; + + /** A Windows FILETIME per block follows. */ + public static final int HAS_FILETIME = 0x02; + + /** An MD5 digest per block follows. */ + public static final int HAS_MD5 = 0x04; + + /** A patch-marker bit per block follows. */ + public static final int HAS_PATCH_BIT = 0x08; + + /** Every bit this implementation understands. */ + public static final int KNOWN_FLAGS = HAS_CRC32 | HAS_FILETIME | HAS_MD5 | HAS_PATCH_BIT; + + /** Size of the fixed header. */ + private static final int HEADER_SIZE = 8; + + /** Difference between the FILETIME and Unix epochs, in milliseconds. */ + private static final long EPOCH_OFFSET_MILLIS = 11_644_473_600_000L; + + /** FILETIME ticks per millisecond. */ + private static final long TICKS_PER_MILLI = 10_000L; + + /** + * Converts a Unix millisecond timestamp to a Windows FILETIME. + * + * @param unixMillis milliseconds since 1970-01-01 UTC. + * @return 100-nanosecond intervals since 1601-01-01 UTC. + */ + public static long toFileTime(long unixMillis) { + return (unixMillis + EPOCH_OFFSET_MILLIS) * TICKS_PER_MILLI; + } + + /** + * Converts a Windows FILETIME back to Unix milliseconds. + * + * @param fileTime 100-nanosecond intervals since 1601-01-01 UTC. + * @return milliseconds since 1970-01-01 UTC. + */ + public static long toUnixMillis(long fileTime) { + return fileTime / TICKS_PER_MILLI - EPOCH_OFFSET_MILLIS; + } + + /** + * Bytes needed to hold {@code entries} attributes of the given shape. + * + * @param flags the bytemask. + * @param entries number of blocks described. + * @return the exact file length. + */ + public static long sizeFor(int flags, int entries) { + long size = HEADER_SIZE; + if ((flags & HAS_CRC32) != 0) { + size += 4L * entries; + } + if ((flags & HAS_FILETIME) != 0) { + size += 8L * entries; + } + if ((flags & HAS_MD5) != 0) { + size += 16L * entries; + } + if ((flags & HAS_PATCH_BIT) != 0) { + // StormLib rounds up and then allows a spare byte, rather than the + // (entries + 7) / 8 you would expect. + size += (entries + 6L) / 8; + } + return size; + } + + /** + * {@link #sizeFor} narrowed for an allocation. + *

+ * The entry count comes from a block table, so a large enough archive -- or + * a caller asking for a great many spare block slots -- can describe more + * attributes than fit in an array. Casting blind would wrap to a negative + * length and fail with a message about the wrong thing. + * + * @param flags the bytemask. + * @param entries number of blocks described. + * @return the size as an {@code int}. + */ + private static int inMemorySize(int flags, int entries) { + final long size = sizeFor(flags, entries); + if (size > Integer.MAX_VALUE - 8) { + throw new IllegalArgumentException("Attributes for " + entries + " blocks would need " + + size + " bytes, more than can be held in memory."); + } + return (int) size; + } + + /** + * Parses an attributes file. + * + * @param data the file content, already decoded. + * @param blockCount how many block table rows the archive has. + * @return the parsed attributes. + * @throws JMpqException if the content cannot be read as attributes for an + * archive of this size. + */ + public static MpqAttributes parse(byte[] data, int blockCount) throws JMpqException { + if (data.length < HEADER_SIZE) { + throw new JMpqException("An attributes file needs at least " + HEADER_SIZE + + " bytes, got " + data.length + "."); + } + final ByteBuffer in = ByteBuffer.wrap(data).order(ByteOrder.LITTLE_ENDIAN); + final int version = in.getInt(); + final int flags = in.getInt(); + + // An unknown bit means an array of unknown length, so nothing after the + // arrays we do understand can be located. Reading the known prefix and + // ignoring the rest is what StormLib does. + final int usable = flags & KNOWN_FLAGS; + + final int entries; + final boolean truncated; + if (sizeFor(usable, blockCount) == data.length) { + entries = blockCount; + truncated = false; + } else if (blockCount > 0 && sizeFor(usable, blockCount - 1) == data.length) { + entries = blockCount - 1; + truncated = true; + } else { + throw new JMpqException("An attributes file with flags 0x" + + Integer.toHexString(flags) + " for " + blockCount + " blocks should be " + + sizeFor(usable, blockCount) + " bytes, but is " + data.length + "."); + } + + final int[] crc32 = (usable & HAS_CRC32) != 0 ? new int[entries] : new int[0]; + for (int i = 0; i < crc32.length; i++) { + crc32[i] = in.getInt(); + } + final long[] fileTimes = (usable & HAS_FILETIME) != 0 ? new long[entries] : new long[0]; + for (int i = 0; i < fileTimes.length; i++) { + fileTimes[i] = in.getLong(); + } + final byte[][] md5 = (usable & HAS_MD5) != 0 ? new byte[entries][] : new byte[0][]; + for (int i = 0; i < md5.length; i++) { + md5[i] = new byte[16]; + in.get(md5[i]); + } + final boolean[] patchBits = + (usable & HAS_PATCH_BIT) != 0 ? new boolean[entries] : new boolean[0]; + final int bitsAt = in.position(); + for (int i = 0; i < patchBits.length; i++) { + patchBits[i] = (data[bitsAt + (i >>> 3)] & (0x80 >>> (i & 7))) != 0; + } + + return new MpqAttributes(version, flags, crc32, fileTimes, md5, patchBits, truncated); + } + + /** + * Builds a CRC32-plus-timestamp attributes file, which is the shape + * StormLib writes by default. + * + * @param crc32 one zlib CRC32 per block; 0 where unknown. + * @param fileTimes one Windows FILETIME per block. + * @return the file content. + */ + public static byte[] build(int[] crc32, long[] fileTimes) { + if (crc32.length != fileTimes.length) { + throw new IllegalArgumentException("Got " + crc32.length + " checksums but " + + fileTimes.length + " timestamps."); + } + final int flags = HAS_CRC32 | HAS_FILETIME; + final ByteBuffer out = ByteBuffer + .allocate(inMemorySize(flags, crc32.length)) + .order(ByteOrder.LITTLE_ENDIAN); + out.putInt(VERSION); + out.putInt(flags); + for (int value : crc32) { + out.putInt(value); + } + for (long value : fileTimes) { + out.putLong(value); + } + return out.array(); + } + + /** + * @return this attributes file serialised. Only the arrays this + * implementation understands are emitted, so a file that carried + * unknown ones does not round-trip byte for byte. + */ + public byte[] toByteArray() { + final int emitted = flags & KNOWN_FLAGS; + final ByteBuffer out = ByteBuffer + .allocate(inMemorySize(emitted, entries())) + .order(ByteOrder.LITTLE_ENDIAN); + out.putInt(version); + out.putInt(emitted); + for (int value : crc32) { + out.putInt(value); + } + for (long value : fileTimes) { + out.putLong(value); + } + for (byte[] digest : md5) { + out.put(digest); + } + if (patchBits.length > 0) { + final byte[] bits = new byte[(int) ((patchBits.length + 6L) / 8)]; + for (int i = 0; i < patchBits.length; i++) { + if (patchBits[i]) { + bits[i >>> 3] |= (byte) (0x80 >>> (i & 7)); + } + } + out.put(bits); + } + return out.array(); + } + + /** + * @return how many blocks this file describes. + */ + public int entries() { + if (crc32.length > 0) { + return crc32.length; + } + if (fileTimes.length > 0) { + return fileTimes.length; + } + if (md5.length > 0) { + return md5.length; + } + return patchBits.length; + } + + /** + * The recorded checksum for a block. + *

+ * Zero and {@code 0xFFFFFFFF} both mean "not recorded" — StormLib skips + * verification for either — so a caller comparing checksums must treat them + * as absent rather than as a mismatch. + * + * @param blockIndex block table index. + * @return the CRC32, or 0 when not recorded. + */ + public int crc32Of(int blockIndex) { + return blockIndex >= 0 && blockIndex < crc32.length ? crc32[blockIndex] : 0; + } + + /** + * @param blockIndex block table index. + * @return the timestamp, or 0 when not recorded. + */ + public long fileTimeOf(int blockIndex) { + return blockIndex >= 0 && blockIndex < fileTimes.length ? fileTimes[blockIndex] : 0; + } + + /** + * @param flag one of the {@code HAS_} constants. + * @return whether the file declares that array. + */ + public boolean has(int flag) { + return (flags & flag) == flag; + } + + @Override + public String toString() { + return "MpqAttributes[version=" + version + ", flags=0x" + Integer.toHexString(flags) + + ", entries=" + entries() + (truncated ? ", truncated" : "") + "]"; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof MpqAttributes that)) { + return false; + } + return version == that.version && flags == that.flags && truncated == that.truncated + && Arrays.equals(crc32, that.crc32) + && Arrays.equals(fileTimes, that.fileTimes) + && Arrays.deepEquals(md5, that.md5) + && Arrays.equals(patchBits, that.patchBits); + } + + @Override + public int hashCode() { + return Arrays.hashCode(crc32) * 31 + flags; + } +} diff --git a/src/main/java/org/inwc3/jmpq/MpqChecksums.java b/src/main/java/org/inwc3/jmpq/MpqChecksums.java new file mode 100644 index 0000000..5e8f98b --- /dev/null +++ b/src/main/java/org/inwc3/jmpq/MpqChecksums.java @@ -0,0 +1,68 @@ +package org.inwc3.jmpq; + +/** + * The Adler-32 variant MPQ sector checksums use. + * + *

Why this is not {@link java.util.zip.Adler32}

+ * StormLib computes sector checksums as {@code adler32(0, buffer, length)} — + * both when writing them ({@code SFileAddFile.cpp}) and when checking them + * ({@code ReadMpqSectors} in {@code SFileReadFile.cpp}). Passing zlib a seed of + * {@code 0} starts the accumulators at {@code s1 = 0, s2 = 0}, whereas a + * standard Adler-32 — and so {@code java.util.zip.Adler32}, which offers no way + * to seed it — starts at {@code s1 = 1}. The results differ by 1 in the low half + * and by the byte count in the high half, for every input. + *

+ * That is a difference no self-consistent test can see: a reader and a writer + * that both use the standard seed agree with each other perfectly and disagree + * with every archive StormLib ever wrote. It was caught by + * {@code tools/mpqref.py}, which computes the value independently, and is the + * reason that cross-check exists. + */ +final class MpqChecksums { + + /** Largest Adler-32 modulus below 65536. */ + private static final int BASE = 65521; + + /** + * Largest number of bytes that can be accumulated before {@code s2} could + * overflow a signed 32-bit int. zlib calls this {@code NMAX}. + */ + private static final int NMAX = 5552; + + private MpqChecksums() { + } + + /** + * @param data bytes to checksum. + * @return the sector checksum MPQ records: zlib's {@code adler32} seeded + * with 0 rather than the standard 1. + */ + static int adler32(byte[] data) { + return adler32(data, 0, data.length); + } + + /** + * @param data bytes to checksum. + * @param offset first byte to include. + * @param length how many bytes to include. + * @return the sector checksum MPQ records. + */ + static int adler32(byte[] data, int offset, int length) { + int s1 = 0; + int s2 = 0; + int at = offset; + int remaining = length; + + while (remaining > 0) { + final int block = Math.min(remaining, NMAX); + for (int i = 0; i < block; i++) { + s1 += data[at++] & 0xFF; + s2 += s1; + } + s1 %= BASE; + s2 %= BASE; + remaining -= block; + } + return (s2 << 16) | s1; + } +} diff --git a/src/main/java/org/inwc3/jmpq/MpqFileReader.java b/src/main/java/org/inwc3/jmpq/MpqFileReader.java index 4ebc982..1e25031 100644 --- a/src/main/java/org/inwc3/jmpq/MpqFileReader.java +++ b/src/main/java/org/inwc3/jmpq/MpqFileReader.java @@ -20,10 +20,12 @@ final class MpqFileReader { private final MpqSource source; private final MpqHeader header; + private final boolean verifyChecksums; - MpqFileReader(MpqSource source, MpqHeader header) { + MpqFileReader(MpqSource source, MpqHeader header, boolean verifyChecksums) { this.source = source; this.header = header; + this.verifyChecksums = verifyChecksums; } /** @@ -135,6 +137,7 @@ private void readSectors(MpqFileEntry entry, OutputStream target, long base, int final int[] offsets = readSectorOffsets(entry, base, key); final boolean imploded = entry.has(MpqFileEntry.FLAG_IMPLODED); final int sectorSize = header.sectorSize(); + final int[] checksums = readSectorChecksums(entry, offsets, base); int remaining = entry.normalSize(); for (int i = 0; i < dataSectorCount(entry); i++) { @@ -148,6 +151,7 @@ private void readSectors(MpqFileEntry entry, OutputStream target, long base, int final byte[] sector = source.bytes(base + start, end - start); decrypt(entry, sector, key + i); + verifyChecksum(entry, checksums, i, sector); final int expected = Math.min(remaining, sectorSize); final byte[] decoded = imploded @@ -158,6 +162,80 @@ private void readSectors(MpqFileEntry entry, OutputStream target, long base, int } } + /** + * P2-3: the per-sector checksum table of a {@code SECTOR_CRC} file. + *

+ * Despite the flag's name the checksums are Adler-32, seeded 0, + * taken over each sector as stored minus its encryption — that is, after + * decrypting but before decompressing. StormLib's {@code ReadMpqSectors} + * computes {@code adler32(0, pbInSector, dwRawBytesInThisSector)} at exactly + * that point, and its writer takes the same value over the compressed + * buffer, so the two agree. + *

+ * The chunk sits after the data sectors, delimited by the last two entries + * of the sector offset table, and is neither encrypted nor keyed even in an + * encrypted file: StormLib loads it with key 0. It is zlib + * compressed when that makes it smaller. + * + * @return one checksum per data sector, or an empty array when the file + * carries none or verification is off. + */ + private int[] readSectorChecksums(MpqFileEntry entry, int[] offsets, long base) + throws IOException { + if (!verifyChecksums || !entry.has(MpqFileEntry.FLAG_SECTOR_CRC)) { + return new int[0]; + } + final int sectors = dataSectorCount(entry); + // The chunk is delimited by the two entries past the data sectors. + final int start = offsets[sectors]; + final int end = offsets[sectors + 1]; + final int plainSize = sectors * 4; + if (start < 0 || end < start || end > entry.compressedSize() || end == start) { + // A file can carry the flag and no checksums; StormLib treats that + // as "nothing to check" rather than as damage. + return new int[0]; + } + + byte[] chunk = source.bytes(base + start, end - start); + if (chunk.length < plainSize) { + chunk = CompressionUtil.decompress(chunk, chunk.length, plainSize, + header.formatVersion()); + } + if (chunk.length < plainSize) { + return new int[0]; + } + + final ByteBuffer in = ByteBuffer.wrap(chunk).order(java.nio.ByteOrder.LITTLE_ENDIAN); + final int[] checksums = new int[sectors]; + for (int i = 0; i < sectors; i++) { + checksums[i] = in.getInt(); + } + return checksums; + } + + /** + * Compares one sector against its recorded checksum. + *

+ * Zero and {@code 0xFFFFFFFF} mean "not recorded" — StormLib skips both + * explicitly — so neither is a mismatch. + */ + private void verifyChecksum(MpqFileEntry entry, int[] checksums, int index, byte[] sector) + throws JMpqException { + if (index >= checksums.length) { + return; + } + final int expected = checksums[index]; + if (expected == 0 || expected == -1) { + return; + } + final int actual = MpqChecksums.adler32(sector); + if (actual != expected) { + throw new JMpqException("Sector " + index + " of <" + entry.name() + + "> has checksum 0x" + Integer.toHexString(actual) + " but the archive records 0x" + + Integer.toHexString(expected) + "; the file is damaged."); + } + } + /** * @return the sector offset table, decrypted if necessary. Encrypted tables * use the key one below the first sector's. @@ -228,14 +306,17 @@ byte[] storedBytesDecrypted(MpqFileEntry entry) throws IOException { } // Each chunk has its own key, so decrypt chunk by chunk using the - // offset table's own boundaries. Iterating every gap rather than only - // the data sectors also covers the checksum chunk of a SECTOR_CRC file. + // offset table's own boundaries. Only the data sectors: the checksum + // chunk of a SECTOR_CRC file is never encrypted — StormLib loads it + // with key 0 and writes it without encrypting — so "decrypting" it + // would corrupt it, and the caller then clears the encryption flags, + // which would make that permanent. final int[] offsets = readSectorOffsets(entry, base, key); final byte[] table = source.bytes(base, offsets.length * 4); new MPQEncryption(key - 1, true).processSingle(ByteBuffer.wrap(table)); System.arraycopy(table, 0, stored, 0, table.length); - for (int i = 0; i < offsets.length - 1; i++) { + for (int i = 0; i < dataSectorCount(entry); i++) { final int start = offsets[i]; final int end = offsets[i + 1]; if (start < 0 || end < start || end > stored.length) { diff --git a/src/main/java/org/inwc3/jmpq/MpqHeader.java b/src/main/java/org/inwc3/jmpq/MpqHeader.java index 781c329..2bcefcd 100644 --- a/src/main/java/org/inwc3/jmpq/MpqHeader.java +++ b/src/main/java/org/inwc3/jmpq/MpqHeader.java @@ -2,6 +2,10 @@ import systems.crigges.jmpq3.JMpqException; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Arrays; + /** * An MPQ archive header, parsed into an immutable model. * @@ -55,6 +59,9 @@ * @param hiBlockTablePosition hi-block table offset, or 0 when absent. * @param hetTablePosition HET table offset, or 0 when absent. * @param betTablePosition BET table offset, or 0 when absent. + * @param extended the version 3 additions, or {@link Extended#NONE}. + * @param userData the user data header this archive sits behind, or + * {@code null} when it starts the file. * @param malformed whether the header needed repair to be usable; the * archive is still readable, but its declared values * were not trustworthy. @@ -72,6 +79,8 @@ public record MpqHeader( long hiBlockTablePosition, long hetTablePosition, long betTablePosition, + Extended extended, + MpqUserData userData, boolean malformed) { /** {@code 'MPQ\x1A'}, the archive header signature. */ @@ -104,6 +113,92 @@ public record MpqHeader( /** Size of one block table entry. */ public static final int BLOCK_ENTRY_SIZE = 16; + /** Size of one hi-block table entry: the high word of a file position. */ + public static final int HI_BLOCK_ENTRY_SIZE = 2; + + /** + * The version 3 header additions: compressed table sizes and MD5 digests. + *

+ * From version 3 the tables may be stored compressed, which the position + * fields alone cannot express — you need the stored length to know where a + * table ends, and comparing it against the uncompressed length is the only + * way to tell whether it is compressed at all. The digests let a reader + * detect a damaged table before trusting it, which StormLib reports rather + * than treating as fatal. + * + * @param hashTableCompressedSize stored length of the hash table, or 0. + * @param blockTableCompressedSize stored length of the block table, or 0. + * @param hiBlockTableCompressedSize stored length of the hi-block table. + * @param hetTableCompressedSize stored length of the HET table, or 0. + * @param betTableCompressedSize stored length of the BET table, or 0. + * @param rawChunkSize chunk size the MD5s were taken over. + * @param md5BlockTable expected digest of the block table. + * @param md5HashTable expected digest of the hash table. + * @param md5HiBlockTable expected digest of the hi-block table. + * @param md5BetTable expected digest of the BET table. + * @param md5HetTable expected digest of the HET table. + * @param md5Header expected digest of the header itself. + */ + public record Extended( + long hashTableCompressedSize, + long blockTableCompressedSize, + long hiBlockTableCompressedSize, + long hetTableCompressedSize, + long betTableCompressedSize, + int rawChunkSize, + byte[] md5BlockTable, + byte[] md5HashTable, + byte[] md5HiBlockTable, + byte[] md5BetTable, + byte[] md5HetTable, + byte[] md5Header) { + + /** Length of an MD5 digest. */ + public static final int DIGEST_SIZE = 16; + + /** What a header below version 3 carries: nothing. */ + public static final Extended NONE = new Extended(0, 0, 0, 0, 0, 0, + new byte[0], new byte[0], new byte[0], new byte[0], new byte[0], new byte[0]); + + /** + * @return whether any digest was recorded, and so whether validating + * the tables against them is meaningful. + */ + public boolean hasDigests() { + return md5HashTable.length == DIGEST_SIZE || md5BlockTable.length == DIGEST_SIZE; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof Extended that)) { + return false; + } + return hashTableCompressedSize == that.hashTableCompressedSize + && blockTableCompressedSize == that.blockTableCompressedSize + && hiBlockTableCompressedSize == that.hiBlockTableCompressedSize + && hetTableCompressedSize == that.hetTableCompressedSize + && betTableCompressedSize == that.betTableCompressedSize + && rawChunkSize == that.rawChunkSize + && Arrays.equals(md5BlockTable, that.md5BlockTable) + && Arrays.equals(md5HashTable, that.md5HashTable) + && Arrays.equals(md5HiBlockTable, that.md5HiBlockTable) + && Arrays.equals(md5BetTable, that.md5BetTable) + && Arrays.equals(md5HetTable, that.md5HetTable) + && Arrays.equals(md5Header, that.md5Header); + } + + @Override + public int hashCode() { + return Long.hashCode(hashTableCompressedSize) * 31 + rawChunkSize; + } + + @Override + public String toString() { + return "Extended[rawChunkSize=" + rawChunkSize + + ", digests=" + (hasDigests() ? "present" : "absent") + "]"; + } + } + /** * @return the archive's sector size in bytes. */ @@ -112,13 +207,20 @@ public int sectorSize() { } /** - * @return whether this archive uses HET/BET tables, which this library - * reads but does not write. + * @return whether this archive carries HET/BET tables in addition to, or + * instead of, the classic hash and block tables. */ public boolean hasExtendedTables() { return hetTablePosition != 0 || betTablePosition != 0; } + /** + * @return whether a hi-block table extends file positions past 4 GiB. + */ + public boolean hasHiBlockTable() { + return hiBlockTablePosition != 0; + } + /** * @return absolute file offset of the hash table. */ @@ -133,6 +235,98 @@ public long blockTableFileOffset() { return headerOffset + blockTablePosition; } + /** + * @return absolute file offset of the hi-block table. + */ + public long hiBlockTableFileOffset() { + return headerOffset + hiBlockTablePosition; + } + + /** + * Stored length of the hash table. + *

+ * A version 3 archive may compress its tables, in which case the stored + * length is shorter than the entries imply. Below version 3, and whenever + * the field is absent or not shorter, the table is stored plain. + * + * @return bytes the hash table occupies in the file. + */ + public long hashTableStoredSize() { + final long plain = (long) hashTableEntries * HASH_ENTRY_SIZE; + final long declared = extended.hashTableCompressedSize(); + return declared > 0 && declared < plain ? declared : plain; + } + + /** + * @return bytes the block table occupies in the file. + */ + public long blockTableStoredSize() { + final long plain = (long) blockTableEntries * BLOCK_ENTRY_SIZE; + final long declared = extended.blockTableCompressedSize(); + return declared > 0 && declared < plain ? declared : plain; + } + + /** + * @return whether the hash table is stored compressed. + */ + public boolean isHashTableCompressed() { + return hashTableStoredSize() < (long) hashTableEntries * HASH_ENTRY_SIZE; + } + + /** + * @return whether the block table is stored compressed. + */ + public boolean isBlockTableCompressed() { + return blockTableStoredSize() < (long) blockTableEntries * BLOCK_ENTRY_SIZE; + } + + /** + * Checks the header against its own MD5 digest. + *

+ * The digest covers the header up to but not including the digest field + * itself, which sits at the very end of a version 3 header. + * + * @param source the archive bytes. + * @return true when no digest was recorded, or when it matches. + * @throws JMpqException if the header cannot be read. + */ + public boolean verifyHeaderDigest(MpqSource source) throws JMpqException { + if (extended.md5Header().length != Extended.DIGEST_SIZE) { + return true; + } + final int covered = SIZE_BY_VERSION[3] - Extended.DIGEST_SIZE; + return matchesDigest(source.bytes(headerOffset, covered), extended.md5Header()); + } + + /** + * @param data the bytes to digest. + * @param digest the expected MD5, or an empty array to skip the check. + * @return whether the digest matches, or true when there is nothing to + * compare against. + */ + static boolean matchesDigest(byte[] data, byte[] digest) { + if (digest.length != Extended.DIGEST_SIZE || isAllZero(digest)) { + // StormLib treats an all-zero digest as "not recorded" rather than + // as the digest of these bytes. + return true; + } + try { + return Arrays.equals(MessageDigest.getInstance("MD5").digest(data), digest); + } catch (NoSuchAlgorithmException impossible) { + // Every Java runtime is required to provide MD5. + throw new IllegalStateException("MD5 unavailable", impossible); + } + } + + private static boolean isAllZero(byte[] digest) { + for (byte value : digest) { + if (value != 0) { + return false; + } + } + return true; + } + /** * Locates and parses the archive header. * @@ -145,8 +339,12 @@ public long blockTableFileOffset() { * @throws JMpqException if no usable archive header can be found. */ public static MpqHeader parse(MpqSource source, boolean forceV0) throws JMpqException { - final long offset = findHeader(source, forceV0); - return parseAt(source, offset, forceV0); + final Located located = findHeader(source, forceV0); + return parseAt(source, located, forceV0); + } + + /** Where a header was found, and what preceded it. */ + private record Located(long offset, MpqUserData userData) { } /** @@ -156,30 +354,53 @@ public static MpqHeader parse(MpqSource source, boolean forceV0) throws JMpqExce * ({@code MPQ\x1B}) redirects to the real one, and the redirect target is * validated before being followed: protected archives plant user data * headers pointing nowhere. In {@code forceV0} mode user data headers are - * ignored entirely, as Warcraft III ignores them, and candidate headers are - * checked for plausibility so a decoy does not win. + * ignored entirely, as Warcraft III ignores them. + *

+ * P2-5b: a candidate that fails a cheap plausibility test does not end the + * scan. Protected archives plant decoy {@code MPQ\x1A} signatures precisely + * so that a reader commits to the first one it sees and then fails. Keeping + * the first candidate as a fallback means this can only ever find a header + * where the old scan found one, never fewer. */ - private static long findHeader(MpqSource source, boolean forceV0) throws JMpqException { + private static Located findHeader(MpqSource source, boolean forceV0) throws JMpqException { final long size = source.size(); + Located fallback = null; for (long position = 0; position + 4 <= size; position += ALIGNMENT) { final int signature = source.i32(position); if (signature == ARCHIVE_SIGNATURE) { - if (forceV0 && !isPlausible(source, position)) { - continue; + final Located candidate = new Located(position, null); + if (isPlausible(source, position)) { + return candidate; } - return position; + if (fallback == null) { + fallback = candidate; + } + continue; } - if (signature == USER_DATA_SIGNATURE && !forceV0 && source.contains(position + 8, 4)) { - final long redirected = position + source.u32(position + 8); + if (signature == USER_DATA_SIGNATURE && !forceV0) { + final MpqUserData userData = MpqUserData.readAt(source, position); + if (userData == null) { + continue; + } + final long redirected = userData.archiveHeaderOffset(); if (source.contains(redirected, 4) && source.i32(redirected) == ARCHIVE_SIGNATURE) { - return redirected; + final Located candidate = new Located(redirected, userData); + if (isPlausible(source, redirected)) { + return candidate; + } + if (fallback == null) { + fallback = candidate; + } } } } + if (fallback != null) { + return fallback; + } throw new JMpqException("No MPQ archive header in " + source.origin() + "."); } @@ -201,7 +422,8 @@ private static boolean isPlausible(MpqSource source, long position) throws JMpqE && blockTablePosition > 0 && hashTableEntries > 0 && sectorShift <= MAX_SECTOR_SIZE_SHIFT - && source.contains(position + hashTablePosition, 0) + && source.contains(position + hashTablePosition, + (long) hashTableEntries * HASH_ENTRY_SIZE) && source.contains(position + blockTablePosition, 0); } @@ -214,7 +436,9 @@ private static boolean isPlausible(MpqSource source, long position) throws JMpqE * ignores the field too. That is what makes the protected maps of issue #46 * readable. */ - private static MpqHeader parseAt(MpqSource source, long offset, boolean forceV0) throws JMpqException { + private static MpqHeader parseAt(MpqSource source, Located located, boolean forceV0) + throws JMpqException { + final long offset = located.offset(); int declaredHeaderSize = source.i32(offset + 0x04); int formatVersion = source.u16(offset + 0x0C); boolean malformed = false; @@ -261,6 +485,7 @@ private static MpqHeader parseAt(MpqSource source, long offset, boolean forceV0) long hiBlockTablePosition = 0; long hetTablePosition = 0; long betTablePosition = 0; + Extended extended = Extended.NONE; if (formatVersion >= 1) { hiBlockTablePosition = source.i64(offset + 0x20); @@ -273,6 +498,21 @@ private static MpqHeader parseAt(MpqSource source, long offset, boolean forceV0) betTablePosition = source.i64(offset + 0x34); hetTablePosition = source.i64(offset + 0x3C); } + if (formatVersion >= 3) { + extended = new Extended( + source.i64(offset + 0x44), + source.i64(offset + 0x4C), + source.i64(offset + 0x54), + source.i64(offset + 0x5C), + source.i64(offset + 0x64), + source.i32(offset + 0x6C), + source.bytes(offset + 0x70, Extended.DIGEST_SIZE), + source.bytes(offset + 0x80, Extended.DIGEST_SIZE), + source.bytes(offset + 0x90, Extended.DIGEST_SIZE), + source.bytes(offset + 0xA0, Extended.DIGEST_SIZE), + source.bytes(offset + 0xB0, Extended.DIGEST_SIZE), + source.bytes(offset + 0xC0, Extended.DIGEST_SIZE)); + } // The declared archive size is advisory: StormLib notes it "is ignored // by Storm.dll and can contain garbage value". Clamp it so it can never @@ -289,7 +529,21 @@ private static MpqHeader parseAt(MpqSource source, long offset, boolean forceV0) throw new JMpqException("Archive declares " + hashTableEntries + " hash table entries, above the " + MAX_HASH_TABLE_ENTRIES + " StormLib accepts."); } - if (!source.contains(offset + hashTablePosition, (long) hashTableEntries * HASH_ENTRY_SIZE)) { + + if (hiBlockTablePosition < 0 + || (hiBlockTablePosition != 0 && !source.contains(offset + hiBlockTablePosition, 0))) { + // A position outside the file cannot be a table. Dropping it leaves + // the low words, which is what a version 0 reader would use. + hiBlockTablePosition = 0; + malformed = true; + } + + final MpqHeader header = new MpqHeader(offset, headerSize, formatVersion, archiveSize, + sectorSizeShift, hashTablePosition, blockTablePosition, hashTableEntries, + blockTableEntries, hiBlockTablePosition, hetTablePosition, betTablePosition, + extended, located.userData(), malformed); + + if (!source.contains(offset + hashTablePosition, header.hashTableStoredSize())) { throw new JMpqException("Hash table at " + (offset + hashTablePosition) + " spanning " + hashTableEntries + " entries runs past the end of " + source.origin() + "."); } @@ -302,11 +556,17 @@ private static MpqHeader parseAt(MpqSource source, long offset, boolean forceV0) blockTableEntries = 0; malformed = true; } - final long blockTableBytes = (long) blockTableEntries * BLOCK_ENTRY_SIZE; - if (!source.contains(offset + blockTablePosition, blockTableBytes)) { + if (!source.contains(offset + blockTablePosition, header.blockTableStoredSize())) { // StormLib does exactly this: archives in the wild declare a block // table far larger than the file, and rejecting them would be - // stricter than the game. + // stricter than the game. A compressed table cannot be reinterpreted + // this way, because its entry count is not implied by its length. + if (header.isBlockTableCompressed()) { + throw new JMpqException("Compressed block table at " + + (offset + blockTablePosition) + " spanning " + + header.blockTableStoredSize() + " bytes runs past the end of " + + source.origin() + "."); + } final long fits = (source.size() - offset - blockTablePosition) / BLOCK_ENTRY_SIZE; blockTableEntries = (int) Math.max(0, fits); malformed = true; @@ -314,6 +574,7 @@ private static MpqHeader parseAt(MpqSource source, long offset, boolean forceV0) return new MpqHeader(offset, headerSize, formatVersion, archiveSize, sectorSizeShift, hashTablePosition, blockTablePosition, hashTableEntries, blockTableEntries, - hiBlockTablePosition, hetTablePosition, betTablePosition, malformed); + hiBlockTablePosition, hetTablePosition, betTablePosition, + extended, located.userData(), malformed); } } diff --git a/src/main/java/org/inwc3/jmpq/MpqOpenOptions.java b/src/main/java/org/inwc3/jmpq/MpqOpenOptions.java index 443f022..86f133e 100644 --- a/src/main/java/org/inwc3/jmpq/MpqOpenOptions.java +++ b/src/main/java/org/inwc3/jmpq/MpqOpenOptions.java @@ -11,16 +11,25 @@ * @param defaultLocale locale preferred by lookups that do not name one. 0 is * the neutral default and is what almost every archive * uses. + * @param verifySectorChecksums check each sector of a {@code SECTOR_CRC} file + * against its recorded Adler-32 while decoding, and fail + * the read on a mismatch rather than returning bytes known + * to be wrong. */ -public record MpqOpenOptions(boolean forceV0, short defaultLocale) { +public record MpqOpenOptions( + boolean forceV0, + short defaultLocale, + boolean verifySectorChecksums) { + /** The neutral locale, used when an archive stores no localised variants. */ public static final short NEUTRAL_LOCALE = 0; /** - * @return options that trust the header and prefer the neutral locale. + * @return options that trust the header, prefer the neutral locale, and + * verify sector checksums where an archive records them. */ public static MpqOpenOptions defaults() { - return new MpqOpenOptions(false, NEUTRAL_LOCALE); + return new MpqOpenOptions(false, NEUTRAL_LOCALE, true); } /** @@ -31,7 +40,7 @@ public static MpqOpenOptions defaults() { * @return options for reading Warcraft III maps. */ public static MpqOpenOptions warcraft3() { - return new MpqOpenOptions(true, NEUTRAL_LOCALE); + return new MpqOpenOptions(true, NEUTRAL_LOCALE, true); } /** @@ -39,7 +48,7 @@ public static MpqOpenOptions warcraft3() { * @return a copy of these options preferring {@code locale}. */ public MpqOpenOptions withLocale(short locale) { - return new MpqOpenOptions(forceV0, locale); + return new MpqOpenOptions(forceV0, locale, verifySectorChecksums); } /** @@ -47,6 +56,18 @@ public MpqOpenOptions withLocale(short locale) { * @return a copy of these options with that setting. */ public MpqOpenOptions withForceV0(boolean force) { - return new MpqOpenOptions(force, defaultLocale); + return new MpqOpenOptions(force, defaultLocale, verifySectorChecksums); + } + + /** + * Turning verification off makes a damaged archive readable, which is + * occasionally what you want: recovering what is still intact beats + * recovering nothing. It cannot make a sound archive read differently. + * + * @param verify whether to check recorded sector checksums. + * @return a copy of these options with that setting. + */ + public MpqOpenOptions withSectorChecksumVerification(boolean verify) { + return new MpqOpenOptions(forceV0, defaultLocale, verify); } } diff --git a/src/main/java/org/inwc3/jmpq/MpqSectorWriter.java b/src/main/java/org/inwc3/jmpq/MpqSectorWriter.java index 48d16cd..a582840 100644 --- a/src/main/java/org/inwc3/jmpq/MpqSectorWriter.java +++ b/src/main/java/org/inwc3/jmpq/MpqSectorWriter.java @@ -47,14 +47,20 @@ static int write(MpqImageBuffer image, byte[] content, int sectorSize, String na } final int dataSectors = MpqFileReader.sectorCount(content.length, sectorSize); - final int tableBytes = (dataSectors + 1) * 4; + final boolean checksums = + (flags & MpqFileEntry.FLAG_SECTOR_CRC) == MpqFileEntry.FLAG_SECTOR_CRC; + // A SECTOR_CRC file needs one more offset entry, delimiting the + // checksum chunk that follows the data sectors. + final int offsetEntries = dataSectors + 1 + (checksums ? 1 : 0); + final int tableBytes = offsetEntries * 4; // Worst case is the offset table plus every sector stored verbatim plus - // one type byte each. Nothing this encoder produces can exceed it, - // because a sector that does not shrink is stored raw. The pre-2.0 - // writer guessed content.length * 2 and mapped that much file, which - // overflowed for incompressible input. - final long worstCase = (long) tableBytes + content.length + dataSectors; + // one type byte each, plus an uncompressed checksum chunk. Nothing this + // encoder produces can exceed it, because a sector that does not shrink + // is stored raw. The pre-2.0 writer guessed content.length * 2 and + // mapped that much file, which overflowed for incompressible input. + final long worstCase = (long) tableBytes + content.length + dataSectors + + (checksums ? 4L * dataSectors : 0); if (worstCase > MpqImageBuffer.MAX_SIZE) { throw new IllegalArgumentException("File <" + name + "> is too large for an in-memory" + " build: " + content.length + " bytes would need " + worstCase + " of staging."); @@ -64,7 +70,8 @@ static int write(MpqImageBuffer image, byte[] content, int sectorSize, String na final int baseKey = MpqNames.sectorKey(name, flags, filePosition, content.length); final ByteBuffer region = image.reserve((int) worstCase); - final int[] offsets = new int[dataSectors + 1]; + final int[] offsets = new int[offsetEntries]; + final int[] adler = new int[checksums ? dataSectors : 0]; offsets[0] = tableBytes; region.position(tableBytes); @@ -75,6 +82,12 @@ static int write(MpqImageBuffer image, byte[] content, int sectorSize, String na System.arraycopy(content, from, raw, 0, length); final byte[] payload = encodeSector(raw, recompress); + if (checksums) { + // Over the stored sector before encrypting it, which is the + // same bytes a reader sees after decrypting. StormLib takes the + // checksum at exactly these two points. + adler[i] = MpqChecksums.adler32(payload); + } if (encrypt) { new MPQEncryption(baseKey + i, false).processSingle(ByteBuffer.wrap(payload)); } @@ -82,7 +95,15 @@ static int write(MpqImageBuffer image, byte[] content, int sectorSize, String na offsets[i + 1] = offsets[i] + payload.length; } - final int compressedSize = offsets[dataSectors]; + if (checksums) { + // Zlib compressed when that is smaller, and never encrypted: on the + // read side StormLib loads this chunk with key 0. + final byte[] chunk = encodeChecksums(adler, recompress); + region.put(chunk); + offsets[dataSectors + 1] = offsets[dataSectors] + chunk.length; + } + + final int compressedSize = offsets[offsetEntries - 1]; // Fill in the offset table now that the sizes are known. final ByteBuffer table = ByteBuffer.allocate(tableBytes).order(java.nio.ByteOrder.LITTLE_ENDIAN); @@ -101,6 +122,27 @@ static int write(MpqImageBuffer image, byte[] content, int sectorSize, String na return compressedSize; } + /** + * Encodes the per-sector checksum chunk of a {@code SECTOR_CRC} file. + *

+ * The chunk is a plain array of little-endian Adler-32 values, one per data + * sector, zlib compressed when that is smaller. A reader detects the + * compression the same way it does for a sector: by the stored length being + * shorter than the natural one. + * + * @param adler one checksum per data sector. + * @param recompress compression strategy. + * @return the chunk as stored. + */ + private static byte[] encodeChecksums(int[] adler, RecompressOptions recompress) { + final ByteBuffer plain = ByteBuffer.allocate(adler.length * 4) + .order(java.nio.ByteOrder.LITTLE_ENDIAN); + for (int value : adler) { + plain.putInt(value); + } + return encodeSector(plain.array(), recompress); + } + /** * @return the sector's stored form: a deflate type byte followed by * compressed data, or the raw bytes when compressing does not pay. @@ -149,11 +191,15 @@ public static int writeInto(ByteBuffer target, byte[] content, int sectorSize, S /** * @param contentLength the file's decoded size. + * @param checksums whether to record a per-sector checksum. * @return the flags a newly encoded file should carry. */ - static int flagsFor(int contentLength) { - return contentLength == 0 - ? MpqFileEntry.FLAG_EXISTS - : MpqFileEntry.FLAG_EXISTS | MpqFileEntry.FLAG_COMPRESSED; + static int flagsFor(int contentLength, boolean checksums) { + if (contentLength == 0) { + // An empty file has no sectors, so it can carry no checksums. + return MpqFileEntry.FLAG_EXISTS; + } + return MpqFileEntry.FLAG_EXISTS | MpqFileEntry.FLAG_COMPRESSED + | (checksums ? MpqFileEntry.FLAG_SECTOR_CRC : 0); } } diff --git a/src/main/java/org/inwc3/jmpq/MpqUserData.java b/src/main/java/org/inwc3/jmpq/MpqUserData.java new file mode 100644 index 0000000..b213520 --- /dev/null +++ b/src/main/java/org/inwc3/jmpq/MpqUserData.java @@ -0,0 +1,85 @@ +package org.inwc3.jmpq; + +/** + * A user data header, the {@code MPQ\x1B} block that can precede an archive. + * + *

Layout

+ * 
+ * 0x00 u32  signature 'MPQ\x1B'
+ * 0x04 u32  size of the user data area
+ * 0x08 u32  offset of the archive header, relative to this header
+ * 0x0C u32  size of this user data header
+ *
+ * + *

What it is for

+ * Blizzard uses it to staple metadata in front of an archive — a StarCraft II + * map keeps its map info here — so the archive proper starts further into the + * file. Readers that honour it find the header via {@link #headerOffset()}; + * Warcraft III ignores the block entirely, which is why + * {@link MpqOpenOptions#forceV0()} skips it. + *

+ * The pre-2.0 code detected the signature and then discarded everything but the + * redirect, with a TODO where the model should have been. Keeping it means a + * caller can read the user data area, and — more importantly — a rebuild can + * preserve it instead of silently dropping a map's metadata. + * + * @param offset where this header sits in the file. + * @param userDataSize declared size of the user data area that follows. + * @param headerOffset archive header offset, relative to {@link #offset}. + * @param headerSize declared size of this user data header. + */ +public record MpqUserData( + long offset, + int userDataSize, + int headerOffset, + int headerSize) { + + /** Size of the fixed part of a user data header. */ + public static final int SIZE = 16; + + /** + * Reads a user data header. + * + * @param source the archive bytes. + * @param offset where the {@code MPQ\x1B} signature was found. + * @return the parsed header, or {@code null} if the bytes do not hold one. + */ + static MpqUserData readAt(MpqSource source, long offset) { + try { + if (!source.contains(offset, SIZE) + || source.i32(offset) != MpqHeader.USER_DATA_SIGNATURE) { + return null; + } + return new MpqUserData(offset, + source.i32(offset + 0x04), + source.i32(offset + 0x08), + source.i32(offset + 0x0C)); + } catch (systems.crigges.jmpq3.JMpqException unreadable) { + return null; + } + } + + /** + * @return absolute file offset the archive header should be at. + */ + public long archiveHeaderOffset() { + return offset + Integer.toUnsignedLong(headerOffset); + } + + /** + * The user data payload, which is whatever the producing tool put there. + * + * @param source the archive bytes. + * @return the payload, truncated to what the file actually holds. + * @throws systems.crigges.jmpq3.JMpqException if the bytes cannot be read. + */ + public byte[] payload(MpqSource source) throws systems.crigges.jmpq3.JMpqException { + final long start = offset + SIZE; + final long declared = Integer.toUnsignedLong(userDataSize); + // The declared size is not trustworthy: it is a plain u32 written by + // another tool, so clamp it to the file rather than letting it drive + // the allocation. + final long available = Math.max(0, source.size() - start); + return source.bytes(start, (int) Math.min(declared, available)); + } +} diff --git a/src/main/java/org/inwc3/jmpq/MpqWriteOptions.java b/src/main/java/org/inwc3/jmpq/MpqWriteOptions.java index 38d6777..c229de0 100644 --- a/src/main/java/org/inwc3/jmpq/MpqWriteOptions.java +++ b/src/main/java/org/inwc3/jmpq/MpqWriteOptions.java @@ -37,6 +37,9 @@ * @param extraBlockEntries extra unused block table slots to emit beyond the * files written, or 0 for none. The other half of the * P1-8 extension point. + * @param metadata what per-file bookkeeping to record alongside the + * data: sector checksums and an {@code (attributes)} + * file. */ public record MpqWriteOptions( int formatVersion, @@ -45,7 +48,42 @@ public record MpqWriteOptions( boolean writeListfile, boolean keepPrefix, int hashTableCapacity, - int extraBlockEntries) { + int extraBlockEntries, + Metadata metadata) { + + /** + * Optional per-file bookkeeping an archive can carry (P2-3, P2-4). + *

+ * Both are off by default, because both change the bytes of every file + * written and neither is required for an archive to be valid. Warcraft III + * does not need either; StormLib-produced archives normally carry both. + * + * @param sectorChecksums record an Adler-32 per sector, so a reader can + * detect damage instead of returning wrong bytes. + * @param attributes emit an {@code (attributes)} file holding a CRC32 + * and timestamp per block. + * @param timestampMillis the timestamp to record, in Unix milliseconds, or + * {@link #NOW} to read the clock. Pinning it is what + * makes a build reproducible: otherwise two runs + * over identical input differ. + */ + public record Metadata(boolean sectorChecksums, boolean attributes, long timestampMillis) { + + /** Read the clock when the archive is written. */ + public static final long NOW = -1L; + + /** Neither checksums nor attributes. */ + public static final Metadata NONE = new Metadata(false, false, NOW); + + /** + * @return the FILETIME to record, resolving {@link #NOW} against the + * clock. + */ + public long fileTime() { + return MpqAttributes.toFileTime( + timestampMillis == NOW ? System.currentTimeMillis() : timestampMillis); + } + } /** Highest format version this library can write. */ public static final int MAX_WRITABLE_VERSION = 1; @@ -79,6 +117,9 @@ public record MpqWriteOptions( if (recompression == null) { throw new IllegalArgumentException("Recompression options are required."); } + if (metadata == null) { + throw new IllegalArgumentException("Metadata options are required."); + } } /** @@ -87,7 +128,7 @@ public record MpqWriteOptions( */ public static MpqWriteOptions defaults() { return new MpqWriteOptions(0, DEFAULT_SECTOR_SIZE_SHIFT, new RecompressOptions(false), - true, true, 0, 0); + true, true, 0, 0, Metadata.NONE); } /** @@ -117,7 +158,7 @@ public int headerSize() { */ public MpqWriteOptions withFormatVersion(int version) { return new MpqWriteOptions(version, sectorSizeShift, recompression, writeListfile, - keepPrefix, hashTableCapacity, extraBlockEntries); + keepPrefix, hashTableCapacity, extraBlockEntries, metadata); } /** @@ -126,7 +167,7 @@ public MpqWriteOptions withFormatVersion(int version) { */ public MpqWriteOptions withSectorSizeShift(int shift) { return new MpqWriteOptions(formatVersion, shift, recompression, writeListfile, - keepPrefix, hashTableCapacity, extraBlockEntries); + keepPrefix, hashTableCapacity, extraBlockEntries, metadata); } /** @@ -135,7 +176,7 @@ public MpqWriteOptions withSectorSizeShift(int shift) { */ public MpqWriteOptions withRecompression(RecompressOptions options) { return new MpqWriteOptions(formatVersion, sectorSizeShift, options, writeListfile, - keepPrefix, hashTableCapacity, extraBlockEntries); + keepPrefix, hashTableCapacity, extraBlockEntries, metadata); } /** @@ -144,7 +185,7 @@ public MpqWriteOptions withRecompression(RecompressOptions options) { */ public MpqWriteOptions withListfile(boolean write) { return new MpqWriteOptions(formatVersion, sectorSizeShift, recompression, write, - keepPrefix, hashTableCapacity, extraBlockEntries); + keepPrefix, hashTableCapacity, extraBlockEntries, metadata); } /** @@ -153,7 +194,7 @@ public MpqWriteOptions withListfile(boolean write) { */ public MpqWriteOptions withPrefix(boolean keep) { return new MpqWriteOptions(formatVersion, sectorSizeShift, recompression, writeListfile, - keep, hashTableCapacity, extraBlockEntries); + keep, hashTableCapacity, extraBlockEntries, metadata); } /** @@ -167,7 +208,7 @@ public MpqWriteOptions withPrefix(boolean keep) { */ public MpqWriteOptions withHashTableCapacity(int capacity) { return new MpqWriteOptions(formatVersion, sectorSizeShift, recompression, writeListfile, - keepPrefix, capacity, extraBlockEntries); + keepPrefix, capacity, extraBlockEntries, metadata); } /** @@ -181,6 +222,66 @@ public MpqWriteOptions withHashTableCapacity(int capacity) { */ public MpqWriteOptions withExtraBlockEntries(int extra) { return new MpqWriteOptions(formatVersion, sectorSizeShift, recompression, writeListfile, - keepPrefix, hashTableCapacity, extra); + keepPrefix, hashTableCapacity, extra, metadata); + } + + /** + * @return whether files should carry per-sector checksums. + */ + public boolean sectorChecksums() { + return metadata.sectorChecksums(); + } + + /** + * @return whether an {@code (attributes)} file should be emitted. + */ + public boolean writeAttributes() { + return metadata.attributes(); + } + + /** + * @param metadata what bookkeeping to record. + * @return a copy with those metadata settings. + */ + public MpqWriteOptions withMetadata(Metadata metadata) { + return new MpqWriteOptions(formatVersion, sectorSizeShift, recompression, writeListfile, + keepPrefix, hashTableCapacity, extraBlockEntries, metadata); + } + + /** + * Records an Adler-32 per sector, so a reader can tell damaged data from + * good rather than handing back wrong bytes (P2-3). + * + * @param record whether to emit sector checksums. + * @return a copy with that setting. + */ + public MpqWriteOptions withSectorChecksums(boolean record) { + return withMetadata(new Metadata(record, metadata.attributes(), + metadata.timestampMillis())); + } + + /** + * Emits an {@code (attributes)} file holding a CRC32 and timestamp per + * block (P2-4). + * + * @param record whether to emit attributes. + * @return a copy with that setting. + */ + public MpqWriteOptions withAttributes(boolean record) { + return withMetadata(new Metadata(metadata.sectorChecksums(), record, + metadata.timestampMillis())); + } + + /** + * Pins the timestamp recorded in {@code (attributes)}, which is what makes + * a build reproducible. + * + * @param unixMillis the timestamp, or {@link Metadata#NOW} to read the + * clock. + * @return a copy with that timestamp. + */ + public MpqWriteOptions withAttributesTimestamp(long unixMillis) { + return withMetadata(new Metadata(metadata.sectorChecksums(), metadata.attributes(), + unixMillis)); } } diff --git a/src/main/java/systems/crigges/jmpq3/AttributesFile.java b/src/main/java/systems/crigges/jmpq3/AttributesFile.java index cbcce30..e1e8fd5 100644 --- a/src/main/java/systems/crigges/jmpq3/AttributesFile.java +++ b/src/main/java/systems/crigges/jmpq3/AttributesFile.java @@ -1,19 +1,25 @@ package systems.crigges.jmpq3; +import org.inwc3.jmpq.MpqAttributes; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.File; -import java.io.IOException; import java.nio.ByteBuffer; import java.nio.ByteOrder; -import java.nio.file.Files; import java.util.ArrayList; import java.util.HashMap; import java.util.zip.CRC32; +/** + * The {@code (attributes)} file. + * + * @deprecated use {@link org.inwc3.jmpq.MpqAttributes}, which models the whole + * format rather than the one shape this class assumed. + */ +@Deprecated(since = "2.0", forRemoval = false) public class AttributesFile { - private final Logger log = LoggerFactory.getLogger(this.getClass().getName()); + private static final Logger log = LoggerFactory.getLogger(AttributesFile.class); + private final byte[] file; private final int[] crc32; @@ -22,6 +28,9 @@ public class AttributesFile { private final CRC32 crcGen = new CRC32(); + /** + * @param entries how many blocks to describe. + */ public AttributesFile(int entries) { this.file = new byte[8 + 12 * entries]; this.file[0] = 100; // Format Version @@ -30,76 +39,132 @@ public AttributesFile(int entries) { timestamps = new long[entries]; } + /** + * Parses an attributes file. + *

+ * P2-4: the entry count is now derived from the bytemask the file declares, + * rather than from an assumed CRC32-plus-timestamp layout with an + * unexplained entry subtracted. The old {@code (length - 8) / 12 - 1} was + * wrong three ways: it ignored the bytemask it had just read, it misread any + * file carrying MD5 digests, and the {@code - 1} hardcoded one of the + * several lengths StormLib tolerates instead of working out which one this + * file is. + *

+ * Without the archive's block count this can only infer the count from the + * length, so it takes the largest count that fits. {@link MpqAttributes} + * does it properly, given the block count it needs. + * + * @param file the file content. + */ public AttributesFile(byte[] file) { this.file = file; - ByteBuffer buffer = ByteBuffer.wrap(file); - buffer.order(ByteOrder.LITTLE_ENDIAN); - buffer.position(8); - int fileCount = (file.length - 8) / 12 - 1; - crc32 = new int[fileCount]; - timestamps = new long[fileCount]; - for (int i = 0; i < fileCount; i++) { + final ByteBuffer buffer = ByteBuffer.wrap(file).order(ByteOrder.LITTLE_ENDIAN); + buffer.position(4); + final int flags = file.length >= 8 ? buffer.getInt() : 0; + final int usable = flags & MpqAttributes.KNOWN_FLAGS; + + // A bytemask naming no array this implementation knows describes no + // entries. Without the guard the loop below never terminates, because + // every count then has the same size as every other. + int entries = 0; + if (usable != 0) { + while (MpqAttributes.sizeFor(usable, entries + 1) <= file.length) { + entries++; + } + } + + crc32 = (usable & MpqAttributes.HAS_CRC32) != 0 ? new int[entries] : new int[0]; + for (int i = 0; i < crc32.length; i++) { crc32[i] = buffer.getInt(); } - for (int i = 0; i < fileCount; i++) { + timestamps = (usable & MpqAttributes.HAS_FILETIME) != 0 ? new long[entries] : new long[0]; + for (int i = 0; i < timestamps.length; i++) { timestamps[i] = buffer.getLong(); } - log.debug("parsed attributes"); + log.debug("parsed attributes: flags 0x{}, {} entries", + Integer.toHexString(flags), entries); } + /** + * @param i block index. + * @param crc the block's CRC32. + * @param timestamp the block's Windows FILETIME. + */ public void setEntry(int i, int crc, long timestamp) { crc32[i] = crc; timestamps[i] = timestamp; } + /** + * @return the serialised file. + */ public byte[] buildFile() { ByteBuffer buffer = ByteBuffer.wrap(file); buffer.order(ByteOrder.LITTLE_ENDIAN); buffer.position(8); - for(int crc : crc32) { + for (int crc : crc32) { buffer.putInt(crc); } - for(long timestamp : timestamps) { + for (long timestamp : timestamps) { buffer.putLong(timestamp); } return buffer.array(); } + /** + * @return how many blocks are described. + */ public int entries() { - return crc32.length; + return Math.max(crc32.length, timestamps.length); } + /** + * @return the CRC32 array, which is empty when the file declared none. + */ public int[] getCrc32() { return crc32; } + /** + * @return the timestamp array, which is empty when the file declared none. + */ public long[] getTimestamps() { return timestamps; } + /** + * @return the raw file bytes. + */ public byte[] getFile() { return file; } + /** + * @param names names in block order. + */ public void setNames(ArrayList names) { int i = 0; - for(String name : names) { + for (String name : names) { refMap.put(name, i); i++; } } + /** + * @param name a file name. + * @return its index, or -1. + */ public int getEntry(String name) { - return refMap.getOrDefault(name, -1); - } - - private int getCrc32(File file) throws IOException { - return getCrc32(Files.readAllBytes(file.toPath())); + return refMap.getOrDefault(name, -1); } + /** + * @param bytes a file's decoded content. + * @return its zlib CRC32. + */ public int getCrc32(byte[] bytes) { crcGen.reset(); crcGen.update(bytes); return (int) crcGen.getValue(); } -} \ No newline at end of file +} diff --git a/src/main/java/systems/crigges/jmpq3/MpqFile.java b/src/main/java/systems/crigges/jmpq3/MpqFile.java index f3175cc..0bdfc7c 100644 --- a/src/main/java/systems/crigges/jmpq3/MpqFile.java +++ b/src/main/java/systems/crigges/jmpq3/MpqFile.java @@ -452,11 +452,16 @@ private byte[] storedBytesDecrypted() throws JMpqException { return stored; } + // Only the data sectors. The checksum chunk of a SECTOR_CRC file is + // never encrypted -- StormLib writes it plain and loads it with key 0 -- + // so decrypting it here would corrupt it, and because the caller then + // clears the encryption flags while keeping SECTOR_CRC, that corruption + // would be written back as authoritative. final int[] offsets = readSectorOffsets(); final byte[] table = readAt(0, offsets.length * 4); decrypt(table, baseKey - 1); System.arraycopy(table, 0, stored, 0, table.length); - for (int i = 0; i < offsets.length - 1; i++) { + for (int i = 0; i < dataSectorCount(); i++) { final int start = offsets[i]; final int end = offsets[i + 1]; validateSectorRange(i, start, end); diff --git a/src/test/java/systems/crigges/jmpq3test/LegacyApiTests.java b/src/test/java/systems/crigges/jmpq3test/LegacyApiTests.java index 01c72a4..11704f1 100644 --- a/src/test/java/systems/crigges/jmpq3test/LegacyApiTests.java +++ b/src/test/java/systems/crigges/jmpq3test/LegacyApiTests.java @@ -162,10 +162,11 @@ public void blocksAreReachableWithoutNames() throws IOException { /** * {@code (attributes)} round-trips through the legacy parser. *

- * Pinned as it behaves today rather than as it should: P2-4 covers honouring - * the bytemask properly, and notes that the entry count subtracts one for - * reasons nobody has justified. Pinning it first means that change will show - * up as a deliberate diff here. + * P2-4 has since replaced the fixed layout with one derived from the + * bytemask the file declares, so the entry count no longer loses one. The + * deprecated class keeps working; {@code MpqAttributesTests} covers what it + * now does, and {@link org.inwc3.jmpq.MpqAttributes} covers the format + * properly. */ @Test public void attributesFileRoundTrips() { @@ -181,9 +182,7 @@ public void attributesFileRoundTrips() { Assert.assertEquals(image[4], 3, "crc plus timestamp bytemask"); final AttributesFile read = new AttributesFile(image); - // The parser subtracts one from the entry count. Documented here as - // current behaviour, not endorsed; see P2-4. - Assert.assertEquals(read.entries(), entries - 1); + Assert.assertEquals(read.entries(), entries); for (int i = 0; i < read.entries(); i++) { Assert.assertEquals(read.getCrc32()[i], 0x1000 + i); } @@ -201,7 +200,7 @@ public void attributesCrcMatchesTheJdk() { Assert.assertEquals(attributes.getCrc32(new byte[0]), 0); } - /** Timestamps and names are addressable, which P2-4 will build on. */ + /** Timestamps and names are addressable. */ @Test public void attributesTimestampsAndNamesAreAddressable() { final AttributesFile attributes = new AttributesFile(3); diff --git a/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java b/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java new file mode 100644 index 0000000..92cbc7d --- /dev/null +++ b/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java @@ -0,0 +1,199 @@ +package systems.crigges.jmpq3test; + +import org.inwc3.jmpq.MpqAttributes; +import org.testng.Assert; +import org.testng.annotations.Test; +import systems.crigges.jmpq3.AttributesFile; +import systems.crigges.jmpq3.JMpqException; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; + +/** + * P2-4: the {@code (attributes)} file, read according to its own bytemask. + *

+ * The pre-2.0 parser read the bytemask and then ignored it, assuming a CRC32 + * array followed by a FILETIME array and nothing else, and subtracted one from + * the entry count for reasons nobody recorded. These tests pin what the format + * actually says, so a file carrying MD5 digests or patch bits is read rather + * than misread. + */ +public class MpqAttributesTests { + + private static final int BLOCKS = 5; + + /** Every array the format defines, at the offsets the format puts them. */ + @Test + public void everyDeclaredArrayIsRead() throws JMpqException { + final int flags = MpqAttributes.HAS_CRC32 | MpqAttributes.HAS_FILETIME + | MpqAttributes.HAS_MD5 | MpqAttributes.HAS_PATCH_BIT; + final ByteBuffer out = ByteBuffer + .allocate((int) MpqAttributes.sizeFor(flags, BLOCKS)) + .order(ByteOrder.LITTLE_ENDIAN); + out.putInt(MpqAttributes.VERSION); + out.putInt(flags); + for (int i = 0; i < BLOCKS; i++) { + out.putInt(0x1000 + i); + } + for (int i = 0; i < BLOCKS; i++) { + out.putLong(0x2000L + i); + } + for (int i = 0; i < BLOCKS; i++) { + final byte[] digest = new byte[16]; + digest[0] = (byte) i; + out.put(digest); + } + // Patch bits, most significant bit first: blocks 0 and 3 are patches. + out.put((byte) 0b1001_0000); + + final MpqAttributes attributes = MpqAttributes.parse(out.array(), BLOCKS); + + Assert.assertEquals(attributes.version(), MpqAttributes.VERSION); + Assert.assertEquals(attributes.entries(), BLOCKS); + Assert.assertFalse(attributes.truncated()); + for (int i = 0; i < BLOCKS; i++) { + Assert.assertEquals(attributes.crc32Of(i), 0x1000 + i, "crc " + i); + Assert.assertEquals(attributes.fileTimeOf(i), 0x2000L + i, "time " + i); + Assert.assertEquals(attributes.md5()[i][0], (byte) i, "md5 " + i); + } + Assert.assertEquals(attributes.patchBits(), new boolean[]{true, false, false, true, false}); + Assert.assertTrue(attributes.has(MpqAttributes.HAS_MD5)); + } + + /** + * A CRC32-only file. Under the old fixed layout its entries would have been + * read as {@code (length - 8) / 12 - 1}, which for five blocks is 0. + */ + @Test + public void aCrcOnlyFileIsNotReadAsCrcPlusTimestamps() throws JMpqException { + final ByteBuffer out = ByteBuffer + .allocate((int) MpqAttributes.sizeFor(MpqAttributes.HAS_CRC32, BLOCKS)) + .order(ByteOrder.LITTLE_ENDIAN); + out.putInt(MpqAttributes.VERSION); + out.putInt(MpqAttributes.HAS_CRC32); + for (int i = 0; i < BLOCKS; i++) { + out.putInt(0xABC0 + i); + } + + final MpqAttributes attributes = MpqAttributes.parse(out.array(), BLOCKS); + Assert.assertEquals(attributes.entries(), BLOCKS); + Assert.assertEquals(attributes.crc32().length, BLOCKS); + Assert.assertEquals(attributes.fileTimes().length, 0, "no timestamps were declared"); + Assert.assertEquals(attributes.crc32Of(4), 0xABC4); + Assert.assertEquals(attributes.fileTimeOf(4), 0, "absent means 0, not out of bounds"); + } + + /** + * StormLib tolerates an attributes file one entry short, because the tool + * that wrote it is rarely the tool reading it. That tolerance is the only + * defensible origin of the old parser's {@code - 1}, which applied it + * always rather than when the length called for it. + */ + @Test + public void oneEntryShortIsAcceptedAndReported() throws JMpqException { + final int flags = MpqAttributes.HAS_CRC32 | MpqAttributes.HAS_FILETIME; + final byte[] full = MpqAttributes.build(new int[BLOCKS], new long[BLOCKS]); + + Assert.assertEquals(MpqAttributes.parse(full, BLOCKS).entries(), BLOCKS); + Assert.assertFalse(MpqAttributes.parse(full, BLOCKS).truncated()); + + // The same bytes, for an archive that has one more block than they cover. + final MpqAttributes shortened = MpqAttributes.parse(full, BLOCKS + 1); + Assert.assertEquals(shortened.entries(), BLOCKS); + Assert.assertTrue(shortened.truncated()); + Assert.assertEquals(shortened.flags(), flags); + } + + /** A length matching no plausible entry count is reported, not guessed at. */ + @Test + public void anImplausibleLengthIsRejected() { + final byte[] full = MpqAttributes.build(new int[BLOCKS], new long[BLOCKS]); + final JMpqException thrown = Assert.expectThrows(JMpqException.class, + () -> MpqAttributes.parse(full, BLOCKS + 40)); + Assert.assertTrue(thrown.getMessage().contains("should be"), thrown.getMessage()); + + Assert.expectThrows(JMpqException.class, () -> MpqAttributes.parse(new byte[4], 1)); + } + + /** + * An unknown bit means an array of unknown length, so nothing past the + * known arrays can be located. The known prefix is still read. + */ + @Test + public void unknownFlagsDoNotStopTheKnownArraysBeingRead() throws JMpqException { + final byte[] file = MpqAttributes.build(new int[]{7, 8}, new long[]{9, 10}); + // Set a bit no version of the format defines. + file[4] |= 0x40; + + final MpqAttributes attributes = MpqAttributes.parse(file, 2); + Assert.assertEquals(attributes.crc32Of(0), 7); + Assert.assertEquals(attributes.fileTimeOf(1), 10); + Assert.assertTrue(attributes.has(0x40), "the flag is preserved as stored"); + // Re-emitting drops what could not be understood, and says so by + // emitting only the known bits. + Assert.assertEquals(MpqAttributes.parse(attributes.toByteArray(), 2).flags(), + MpqAttributes.HAS_CRC32 | MpqAttributes.HAS_FILETIME); + } + + /** The default shape round-trips through build and parse unchanged. */ + @Test + public void theDefaultShapeRoundTrips() throws JMpqException { + final int[] crc = {1, 2, 3}; + final long[] times = {100, 200, 300}; + final MpqAttributes parsed = MpqAttributes.parse(MpqAttributes.build(crc, times), 3); + + Assert.assertEquals(parsed.crc32(), crc); + Assert.assertEquals(parsed.fileTimes(), times); + Assert.assertEquals(parsed.flags(), + MpqAttributes.HAS_CRC32 | MpqAttributes.HAS_FILETIME); + Assert.assertEquals(parsed, MpqAttributes.parse(parsed.toByteArray(), 3)); + Assert.expectThrows(IllegalArgumentException.class, + () -> MpqAttributes.build(new int[2], new long[3])); + } + + /** FILETIME conversion has to survive a round trip at millisecond scale. */ + @Test + public void fileTimeConversionRoundTrips() { + final long millis = 1_600_000_000_000L; + Assert.assertEquals(MpqAttributes.toUnixMillis(MpqAttributes.toFileTime(millis)), millis); + // 1601-01-01, the FILETIME epoch. + Assert.assertEquals(MpqAttributes.toFileTime(-11_644_473_600_000L), 0L); + } + + /** + * The deprecated parser now derives its count from the bytemask too, so a + * CRC-plus-timestamp file reports every entry it holds rather than one + * fewer. + */ + @Test + public void theDeprecatedParserNoLongerLosesAnEntry() { + final int entries = 4; + final AttributesFile written = new AttributesFile(entries); + for (int i = 0; i < entries; i++) { + written.setEntry(i, 0x1000 + i, 0x2000L + i); + } + + final AttributesFile read = new AttributesFile(written.buildFile()); + Assert.assertEquals(read.entries(), entries, "the unexplained -1 is gone"); + for (int i = 0; i < entries; i++) { + Assert.assertEquals(read.getCrc32()[i], 0x1000 + i); + Assert.assertEquals(read.getTimestamps()[i], 0x2000L + i); + } + } + + /** + * A bytemask naming nothing this implementation knows describes no entries. + * Worth its own test: the length-driven count only terminates because of + * that, and without the guard the deprecated parser spins forever. + */ + @Test + public void aBytemaskNamingNothingKnownDescribesNothing() throws JMpqException { + final ByteBuffer out = ByteBuffer.allocate(64).order(ByteOrder.LITTLE_ENDIAN); + out.putInt(MpqAttributes.VERSION); + out.putInt(0x40); + + Assert.assertEquals(new AttributesFile(out.array()).entries(), 0); + // 64 bytes is not 8, so the strict parser reports the mismatch instead. + Assert.expectThrows(JMpqException.class, () -> MpqAttributes.parse(out.array(), 3)); + } +} diff --git a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java new file mode 100644 index 0000000..43a5018 --- /dev/null +++ b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java @@ -0,0 +1,547 @@ +package systems.crigges.jmpq3test; + +import org.inwc3.jmpq.MpqArchive; +import org.inwc3.jmpq.MpqArchiveWriter; +import org.inwc3.jmpq.MpqAttributes; +import org.inwc3.jmpq.MpqFileEntry; +import org.inwc3.jmpq.MpqHeader; +import org.inwc3.jmpq.MpqOpenOptions; +import org.inwc3.jmpq.MpqUserData; +import org.inwc3.jmpq.MpqWriteOptions; +import org.testng.Assert; +import org.testng.annotations.Test; +import systems.crigges.jmpq3.JMpqException; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Random; +import java.util.zip.CRC32; + +/** + * Phase 2: sector checksums, generated attributes, the hi-block table and the + * user data header. + */ +public class Phase2FormatTests { + + /** Enough content for several sectors at the default 4 KiB. */ + private static byte[] incompressible(int length, long seed) { + final byte[] content = new byte[length]; + new Random(seed).nextBytes(content); + return content; + } + + private static int crc32(byte[] content) { + final CRC32 digest = new CRC32(); + digest.update(content); + return (int) digest.getValue(); + } + + // ------------------------------------------------------- P2-3 sector CRC + + /** + * A checksummed archive reads back byte for byte, and says so in its flags. + *

+ * The {@code (listfile)} matters as much as the caller's files here: the + * writer encrypts internal files, so this is the case where a file's sectors + * are encrypted while its checksum chunk is not — StormLib writes that chunk + * without encrypting and loads it with key 0. Getting that wrong decodes the + * sectors correctly and the checksums as noise. + */ + @Test + public void checksummedFilesRoundTrip() throws IOException { + final byte[] big = incompressible(11_000, 1); + final byte[] small = "small enough for one sector".getBytes(StandardCharsets.UTF_8); + + final byte[] image = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withSectorChecksums(true)) + .put("big.bin", big) + .put("small.txt", small) + .toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.read("big.bin"), big); + Assert.assertEquals(archive.read("small.txt"), small); + + for (MpqFileEntry entry : archive.entries()) { + Assert.assertTrue(entry.has(MpqFileEntry.FLAG_SECTOR_CRC), + entry.name() + " should carry checksums: " + entry.flagsToString()); + } + // The encrypted-sectors-plain-checksums combination, exercised. + final MpqFileEntry listfile = archive.entry("(listfile)").orElseThrow(); + Assert.assertTrue(listfile.isEncrypted()); + Assert.assertTrue(listfile.has(MpqFileEntry.FLAG_SECTOR_CRC)); + Assert.assertTrue(new String(archive.read("(listfile)"), StandardCharsets.UTF_8) + .contains("big.bin")); + } + } + + /** An empty file has no sectors, so it cannot carry a checksum. */ + @Test + public void anEmptyFileCarriesNoChecksum() throws IOException { + final byte[] image = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withSectorChecksums(true)) + .put("empty.txt", new byte[0]) + .toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + final MpqFileEntry entry = archive.entry("empty.txt").orElseThrow(); + Assert.assertFalse(entry.has(MpqFileEntry.FLAG_SECTOR_CRC), entry.flagsToString()); + Assert.assertEquals(archive.read("empty.txt").length, 0); + } + } + + /** + * The point of the whole feature: damaged data is reported instead of being + * handed back. Flipping a byte inside the first sector's payload is caught + * before decompression, because that is where the checksum is taken. + */ + @Test + public void damageIsDetectedRatherThanReturned() throws IOException { + final byte[] content = incompressible(9_000, 2); + final byte[] image = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withSectorChecksums(true)) + .put("data.bin", content) + .toByteArray(); + + final int corruptAt = payloadStart(image, "data.bin") + 3; + image[corruptAt] ^= 0x5A; + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + final JMpqException thrown = Assert.expectThrows(JMpqException.class, + () -> archive.read("data.bin")); + Assert.assertTrue(thrown.getMessage().contains("checksum"), thrown.getMessage()); + } + + // Turning verification off recovers whatever is still intact, which is + // occasionally what you want. It must not throw, and must not silently + // pretend the bytes are right. + try (MpqArchive archive = MpqArchive.open(image, + MpqOpenOptions.defaults().withSectorChecksumVerification(false))) { + Assert.assertNotEquals(archive.read("data.bin"), content, + "the byte really was corrupted"); + } + } + + /** + * A verbatim copy keeps the checksums valid. + *

+ * This is the path where getting the checksum chunk wrong is permanent: the + * copy clears the encryption flags but keeps {@code SECTOR_CRC}, so a chunk + * mangled on the way through is written as authoritative and every later + * read of that file fails. + */ + @Test + public void aVerbatimCopyKeepsChecksumsValid() throws IOException { + final byte[] content = incompressible(13_000, 3); + final byte[] first = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withSectorChecksums(true)) + .put("carried.bin", content) + .toByteArray(); + + final byte[] second; + try (MpqArchive archive = MpqArchive.open(first, MpqOpenOptions.defaults())) { + Assert.assertTrue(archive.entry("carried.bin").orElseThrow() + .has(MpqFileEntry.FLAG_SECTOR_CRC)); + second = MpqArchiveWriter.from(archive, MpqWriteOptions.defaults()).toByteArray(); + } + + try (MpqArchive archive = MpqArchive.open(second, MpqOpenOptions.defaults())) { + final MpqFileEntry entry = archive.entry("carried.bin").orElseThrow(); + Assert.assertTrue(entry.has(MpqFileEntry.FLAG_SECTOR_CRC), + "the copy preserved the flag, so it must preserve valid checksums"); + Assert.assertFalse(entry.isEncrypted(), "the copy is stored plain"); + Assert.assertEquals(archive.read("carried.bin"), content); + } + } + + /** Where a file's first sector payload begins, past its offset table. */ + private static int payloadStart(byte[] image, String name) throws IOException { + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + final MpqFileEntry entry = archive.entry(name).orElseThrow(); + final int sectors = (entry.normalSize() + archive.header().sectorSize() - 1) + / archive.header().sectorSize(); + final int tableBytes = (sectors + 1 + 1) * 4; + return (int) (archive.header().headerOffset() + entry.filePosition() + tableBytes); + } + } + + /** + * The seed, pinned against known values. + *

+ * StormLib checksums sectors with {@code adler32(0, ...)}, and a standard + * Adler-32 starts at 1 instead. The two differ by 1 in the low half and by + * the byte count in the high half — for every input, which means a reader + * and writer that both get it wrong agree with each other and with nothing + * else. That is exactly what happened here, and only + * {@code tools/mpqref.py} noticed. These constants come from + * {@code zlib.adler32(data, 0)}. + */ + @Test + public void sectorChecksumsUseTheSeedStormLibUses() throws Exception { + Assert.assertEquals(adler32("abc".getBytes(StandardCharsets.UTF_8)), 0x024A0126, + "seeding with 1 would give 0x024D0127"); + Assert.assertEquals(adler32(new byte[0]), 0); + final byte[] long_ = new byte[10_000]; + java.util.Arrays.fill(long_, (byte) 'a'); + Assert.assertEquals(adler32(long_), 0x78ABCDE2, + "long enough to cross the block boundary the accumulator folds at"); + + // And it is not what java.util.zip.Adler32 produces, which is the trap. + final java.util.zip.Adler32 standard = new java.util.zip.Adler32(); + standard.update("abc".getBytes(StandardCharsets.UTF_8)); + Assert.assertNotEquals((int) standard.getValue(), 0x024A0126); + } + + /** Reaches the package-private checksum used by both reader and writer. */ + private static int adler32(byte[] data) throws Exception { + final Class type = Class.forName("org.inwc3.jmpq.MpqChecksums"); + final java.lang.reflect.Method method = type.getDeclaredMethod("adler32", byte[].class); + method.setAccessible(true); + return (int) method.invoke(null, (Object) data); + } + + // ------------------------------------------------------- P2-4 attributes + + /** + * Generated attributes describe every block, with a CRC32 taken over each + * file's decoded content — which is what StormLib records and what issue + * #11 asked for. + */ + @Test + public void generatedAttributesDescribeEveryBlock() throws IOException { + final long pinned = 1_600_000_000_000L; + final Map files = new LinkedHashMap<>(); + files.put("one.txt", "first".getBytes(StandardCharsets.UTF_8)); + files.put("two.bin", incompressible(6_000, 4)); + files.put("three.txt", "third".getBytes(StandardCharsets.UTF_8)); + + final MpqArchiveWriter writer = MpqArchiveWriter.create(MpqWriteOptions.defaults() + .withAttributes(true) + .withAttributesTimestamp(pinned)); + files.forEach(writer::put); + final byte[] image = writer.toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + final MpqAttributes attributes = archive.attributes().orElseThrow(); + Assert.assertEquals(attributes.version(), MpqAttributes.VERSION); + Assert.assertEquals(attributes.entries(), archive.header().blockTableEntries()); + Assert.assertFalse(attributes.truncated()); + + for (Map.Entry file : files.entrySet()) { + final int block = archive.entry(file.getKey()).orElseThrow().blockIndex(); + Assert.assertEquals(attributes.crc32Of(block), crc32(file.getValue()), + "crc of " + file.getKey()); + Assert.assertEquals(attributes.fileTimeOf(block), + MpqAttributes.toFileTime(pinned), "timestamp of " + file.getKey()); + } + + // The listfile is described too; the attributes file cannot describe + // itself, so its own slot stays at "not recorded". + final int listfile = archive.entry("(listfile)").orElseThrow().blockIndex(); + Assert.assertEquals(attributes.crc32Of(listfile), + crc32(archive.read("(listfile)"))); + final int own = archive.entry(MpqAttributes.NAME).orElseThrow().blockIndex(); + Assert.assertEquals(attributes.crc32Of(own), 0, "its own checksum cannot exist"); + } + } + + /** Spare block slots get a zero checksum, which reads as "not recorded". */ + @Test + public void spareBlockSlotsAreDescribedAsUnrecorded() throws IOException { + final byte[] image = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withAttributes(true).withExtraBlockEntries(5)) + .put("a.txt", "a".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + final MpqAttributes attributes = archive.attributes().orElseThrow(); + // One file, the listfile, the attributes file, and five spares. + Assert.assertEquals(attributes.entries(), 3 + 5); + Assert.assertEquals(archive.header().blockTableEntries(), 3 + 5); + for (int i = 3; i < 8; i++) { + Assert.assertEquals(attributes.crc32Of(i), 0, "spare slot " + i); + Assert.assertEquals(attributes.fileTimeOf(i), 0L, "spare slot " + i); + } + } + } + + /** A pinned timestamp makes the build reproducible. */ + @Test + public void aPinnedTimestampMakesTheBuildReproducible() throws IOException { + final MpqWriteOptions options = MpqWriteOptions.defaults() + .withAttributes(true) + .withAttributesTimestamp(1_700_000_000_000L); + + final byte[] first = MpqArchiveWriter.create(options) + .put("a.txt", "a".getBytes(StandardCharsets.UTF_8)).toByteArray(); + final byte[] second = MpqArchiveWriter.create(options) + .put("a.txt", "a".getBytes(StandardCharsets.UTF_8)).toByteArray(); + + Assert.assertEquals(first, second, "identical input must give identical bytes"); + } + + /** + * Generating attributes and supplying them is refused rather than producing + * two entries under one name. Supplying them alone stays legal, which is how + * a caller preserved them before generation existed. + */ + @Test + public void generatingAndSupplyingAttributesIsRefused() throws IOException { + final byte[] supplied = MpqAttributes.build(new int[2], new long[2]); + + final MpqArchiveWriter both = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withAttributes(true)) + .put(MpqAttributes.NAME, supplied); + final JMpqException thrown = Assert.expectThrows(JMpqException.class, both::toByteArray); + Assert.assertTrue(thrown.getMessage().contains("two entries"), thrown.getMessage()); + + final byte[] image = MpqArchiveWriter.create(MpqWriteOptions.defaults()) + .put(MpqAttributes.NAME, supplied) + .toByteArray(); + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.read(MpqAttributes.NAME), supplied); + } + } + + /** + * Attributes are advisory: an archive carrying ones that will not parse is + * still a good archive, and must open rather than being rejected. + */ + @Test + public void unparseableAttributesDoNotStopTheArchiveOpening() throws IOException { + final byte[] image = MpqArchiveWriter.create(MpqWriteOptions.defaults()) + .put(MpqAttributes.NAME, new byte[]{100, 0, 0, 0, 3, 0, 0, 0, 1, 2, 3}) + .put("real.txt", "kept".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertTrue(archive.attributes().isEmpty()); + Assert.assertEquals(archive.read("real.txt"), "kept".getBytes(StandardCharsets.UTF_8)); + } + } + + // ---------------------------------------------------- P2-2 hi-block table + + /** + * A hi-block table of zeroes changes nothing, which is the only shape a + * small archive can legitimately have: the table supplies bits 32 to 47 of + * each file position, so a non-zero entry means an archive past 4 GiB. + */ + @Test + public void aZeroHiBlockTableLeavesPositionsAlone() throws IOException { + final byte[] plain = MpqArchiveWriter.create(MpqWriteOptions.defaults() + .withFormatVersion(1)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + final byte[] withTable = attachHiBlockTable(plain, new int[]{0, 0}); + + try (MpqArchive archive = MpqArchive.open(withTable, MpqOpenOptions.defaults())) { + Assert.assertTrue(archive.header().hasHiBlockTable()); + Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8)); + } + } + + /** + * A non-zero entry really is applied. There is no way to store data 4 GiB + * into a test fixture, so the proof is that the read is attempted there: + * the failure names an offset above 4 GiB, which it can only do if the high + * word reached the file position. + */ + @Test + public void aNonZeroHiBlockEntryMovesTheFilePosition() throws IOException { + final byte[] plain = MpqArchiveWriter.create(MpqWriteOptions.defaults() + .withFormatVersion(1)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + final byte[] withTable = attachHiBlockTable(plain, new int[]{1, 0}); + + try (MpqArchive archive = MpqArchive.open(withTable, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.entry("a.txt").orElseThrow().filePosition() >>> 32, 1L, + "the high word belongs in bits 32 and up"); + final JMpqException thrown = Assert.expectThrows(JMpqException.class, + () -> archive.read("a.txt")); + Assert.assertTrue(thrown.getMessage().contains("outside"), thrown.getMessage()); + } + } + + /** + * An archive claiming a hi-block table it does not hold is read with the low + * words alone, which is what a version 0 reader would do anyway. Refusing it + * would lose an archive that is entirely readable. + */ + @Test + public void aHiBlockTableOutsideTheFileIsIgnored() throws IOException { + final byte[] plain = MpqArchiveWriter.create(MpqWriteOptions.defaults() + .withFormatVersion(1)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + final ByteBuffer edit = ByteBuffer.wrap(plain).order(ByteOrder.LITTLE_ENDIAN); + final int headerAt = headerOffset(plain); + edit.putLong(headerAt + 0x20, 0x7FFF_FFFFL); + + try (MpqArchive archive = MpqArchive.open(plain, MpqOpenOptions.defaults())) { + Assert.assertFalse(archive.header().hasHiBlockTable(), "dropped as implausible"); + Assert.assertTrue(archive.header().malformed()); + Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8)); + } + } + + /** + * Appends a hi-block table to a version 1 archive and points the header at + * it. The table is neither encrypted nor compressed, per StormLib. + */ + private static byte[] attachHiBlockTable(byte[] image, int[] highWords) { + final byte[] out = new byte[image.length + highWords.length * 2]; + System.arraycopy(image, 0, out, 0, image.length); + + final int headerAt = headerOffset(image); + final ByteBuffer edit = ByteBuffer.wrap(out).order(ByteOrder.LITTLE_ENDIAN); + for (int i = 0; i < highWords.length; i++) { + edit.putShort(image.length + i * 2, (short) highWords[i]); + } + edit.putLong(headerAt + 0x20, image.length - headerAt); + return out; + } + + private static int headerOffset(byte[] image) { + for (int at = 0; at + 4 <= image.length; at += MpqHeader.ALIGNMENT) { + if (ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN).getInt(at) + == MpqHeader.ARCHIVE_SIGNATURE) { + return at; + } + } + throw new AssertionError("no header in the test fixture"); + } + + // ------------------------------------------------- P2-1 user data header + + /** + * A user data header redirects to the archive, and its payload is readable + * rather than discarded. The pre-2.0 code detected the signature, followed + * the redirect, and threw the rest away with a TODO where the model should + * have been. + */ + @Test + public void aUserDataHeaderIsParsedAndItsPayloadKept() throws IOException { + final byte[] payload = "user data goes here".getBytes(StandardCharsets.UTF_8); + final byte[] inner = MpqArchiveWriter.create(MpqWriteOptions.defaults().withPrefix(false)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + final byte[] image = withUserData(inner, payload); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + final MpqUserData userData = archive.userData().orElseThrow(); + Assert.assertEquals(userData.offset(), 0); + Assert.assertEquals(userData.headerSize(), MpqUserData.SIZE); + Assert.assertEquals(userData.archiveHeaderOffset(), MpqHeader.ALIGNMENT); + Assert.assertEquals(archive.header().headerOffset(), MpqHeader.ALIGNMENT); + Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8)); + } + + // Warcraft III ignores user data headers, and so does forceV0. The + // archive header is found by scanning instead, at the same place. + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.warcraft3())) { + Assert.assertTrue(archive.userData().isEmpty()); + Assert.assertEquals(archive.header().headerOffset(), MpqHeader.ALIGNMENT); + Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8)); + } + } + + /** Wraps an archive behind a user data header at offset 0. */ + private static byte[] withUserData(byte[] archive, byte[] payload) { + final byte[] out = new byte[MpqHeader.ALIGNMENT + archive.length]; + final ByteBuffer edit = ByteBuffer.wrap(out).order(ByteOrder.LITTLE_ENDIAN); + edit.putInt(MpqHeader.USER_DATA_SIGNATURE); + edit.putInt(payload.length); + edit.putInt(MpqHeader.ALIGNMENT); + edit.putInt(MpqUserData.SIZE); + edit.put(payload); + System.arraycopy(archive, 0, out, MpqHeader.ALIGNMENT, archive.length); + return out; + } + + // --------------------------------------------- P2-5b decoy header scanning + + /** + * A decoy {@code MPQ\x1A} in front of the real archive no longer ends the + * scan. Protected maps plant these precisely so a reader commits to the + * first signature it sees and then fails on tables that are not there. + */ + @Test + public void aDecoyHeaderDoesNotEndTheScan() throws IOException { + final byte[] real = MpqArchiveWriter.create(MpqWriteOptions.defaults().withPrefix(false)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + final byte[] image = new byte[MpqHeader.ALIGNMENT + real.length]; + final ByteBuffer edit = ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN); + // A header-shaped decoy whose tables point far outside the file. + edit.putInt(0, MpqHeader.ARCHIVE_SIGNATURE); + edit.putInt(4, 32); + edit.putInt(0x10, 0x7FFF_0000); + edit.putInt(0x14, 0x7FFF_1000); + edit.putInt(0x18, 16); + System.arraycopy(real, 0, image, MpqHeader.ALIGNMENT, real.length); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.header().headerOffset(), MpqHeader.ALIGNMENT, + "the scan should have walked past the decoy"); + Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8)); + } + } + + // ------------------------------------------- reference cross-verification + + /** + * Exports checksummed and attributed archives for + * {@code tools/mpqref.py verify}, which now checks the Adler-32 of every + * sector itself. That is the only independent confirmation available that + * the values this writer records are the values the format calls for, rather + * than merely values this library agrees with itself about. + */ + @Test + public void exportForReferenceVerification() throws IOException { + final Path out = Path.of("build", "phase2"); + final Path archives = out.resolve("archives"); + Files.createDirectories(archives); + + final Map files = new LinkedHashMap<>(); + files.put("small.txt", "one sector only".getBytes(StandardCharsets.UTF_8)); + files.put("multi.bin", incompressible(20_000, 7)); + files.put("compressible.txt", "abcabcabc".repeat(2_000).getBytes(StandardCharsets.UTF_8)); + files.put("empty.txt", new byte[0]); + + final StringBuilder expected = new StringBuilder("# archive\tname\tsize\tmd5\n"); + + final MpqWriteOptions[] shapes = { + MpqWriteOptions.defaults().withSectorChecksums(true), + MpqWriteOptions.defaults().withAttributes(true).withAttributesTimestamp(0), + MpqWriteOptions.defaults().withSectorChecksums(true).withAttributes(true) + .withAttributesTimestamp(0).withFormatVersion(1), + }; + + for (int shape = 0; shape < shapes.length; shape++) { + final MpqArchiveWriter writer = MpqArchiveWriter.create(shapes[shape]); + files.forEach(writer::put); + final String name = "phase2-" + shape + ".mpq"; + Files.write(archives.resolve(name), writer.toByteArray()); + + for (Map.Entry file : files.entrySet()) { + expected.append(name).append('\t').append(file.getKey()).append('\t') + .append(file.getValue().length).append('\t') + .append(TestHelper.md5(file.getValue())).append('\n'); + } + } + + Files.writeString(out.resolve("expected.tsv"), expected.toString()); + } +} diff --git a/tools/mpqref.py b/tools/mpqref.py index d484f83..1a0dfaf 100644 --- a/tools/mpqref.py +++ b/tools/mpqref.py @@ -476,6 +476,8 @@ def read_block(self, block, name): raise Corrupt("%s sector %d spans [%d, %d) outside %d stored bytes" % (name, i, offsets[i], offsets[i + 1], block.compressed_size)) + checksums = self._sector_checksums(block, raw, offsets, data_sectors) + out = bytearray() codecs = set() remaining = block.normal_size @@ -483,6 +485,11 @@ def read_block(self, block, name): sector = raw[offsets[i]: offsets[i + 1]] if encrypted: sector = decrypt(sector, (key + i) & MASK32) + if i < len(checksums) and checksums[i] not in (0, 0xFFFFFFFF): + actual = zlib.adler32(sector, 0) & MASK32 + if actual != checksums[i]: + raise Corrupt("%s sector %d has adler32 %08x but the archive records %08x" + % (name, i, actual, checksums[i])) expected = min(remaining, self.sector_size) if block.has(FLAG_IMPLODE): raise Unsupported("pkware") @@ -496,6 +503,29 @@ def read_block(self, block, name): return bytes(out), ",".join(sorted(codecs)) + def _sector_checksums(self, block, raw, offsets, data_sectors): + """Per-sector Adler-32 values of a SECTOR_CRC file. + + StormLib names the flag after CRC but computes adler32(0, ...) over the + sector as stored minus its encryption, and the chunk holding the values + sits after the data sectors, delimited by the last two offset entries. + The chunk is zlib compressed when that is smaller, and is never + encrypted -- StormLib loads it with key 0 even for an encrypted file. + """ + if not block.has(FLAG_SECTOR_CRC) or data_sectors == 0: + return [] + start = offsets[data_sectors] + end = offsets[data_sectors + 1] + if end <= start: + return [] + chunk = raw[start:end] + plain_size = data_sectors * 4 + if len(chunk) < plain_size: + chunk, _ = decompress_sector(chunk, plain_size, self.format_version) + if len(chunk) < plain_size: + return [] + return list(struct.unpack("<%dI" % data_sectors, chunk[:plain_size])) + # -- reporting ------------------------------------------------------ def entries(self, extra_names=()): From af766e259e42e5bca83688153beb5408dc631d21 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 10:27:11 +0200 Subject: [PATCH 02/13] Apply requested sector checksums to carried-over files as well A file copied verbatim keeps the source's flags, so asking for checksums quietly meant "on whichever files happened to be re-encoded anyway" and produced a half-checksummed archive. Checksums are computed per stored sector, so adding them requires re-encoding; a file that already carries them can still be copied. --- .../java/org/inwc3/jmpq/MpqArchiveWriter.java | 31 +++++++++++++------ .../crigges/jmpq3test/Phase2FormatTests.java | 29 +++++++++++++++++ 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java index 646ca25..6d96ae1 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java @@ -465,23 +465,36 @@ private record Written(String name, short locale) { private BlockRow writeFile(MpqImageBuffer image, int base, Pending file, int sectorSize) throws IOException { if (file.content() instanceof Content.Existing existing - && canCopyVerbatim(existing.archive(), sectorSize)) { + && canCopyVerbatim(existing, sectorSize)) { return copyVerbatim(image, base, file.name(), existing); } return writeEncoded(image, base, file.name(), contentOf(file), sectorSize, 0); } /** - * Whether a file from {@code source} can keep its stored bytes. + * Whether a file carried over from another archive can keep its stored + * bytes. *

- * Only when the sector size matches. Copying a sector offset table into an - * archive with a different sector size leaves the table describing the old - * geometry, and the file becomes unreadable — the exact bug the golden - * harness caught in the pre-2.0 recompression path. + * The sector size must match. Copying a sector offset table into an archive + * with a different sector size leaves the table describing the old geometry, + * and the file becomes unreadable — the exact bug the golden harness caught + * in the pre-2.0 recompression path. + *

+ * A file that does not already carry sector checksums cannot be copied when + * they were asked for, either: the checksums are computed per stored sector, + * so adding them means re-encoding. Without this, asking for checksums + * quietly meant "on the files that happen to be re-encoded anyway". The + * reverse is fine — checksums already present stay present and stay valid, + * whether or not this archive asked for them. */ - private boolean canCopyVerbatim(MpqArchive source, int sectorSize) { - return source.header().sectorSize() == sectorSize - && !options.recompression().recompress; + private boolean canCopyVerbatim(Content.Existing existing, int sectorSize) { + if (existing.archive().header().sectorSize() != sectorSize + || options.recompression().recompress) { + return false; + } + return !options.sectorChecksums() + || existing.entry().has(MpqFileEntry.FLAG_SECTOR_CRC) + || existing.entry().normalSize() == 0; } private BlockRow copyVerbatim(MpqImageBuffer image, int base, String name, diff --git a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java index 43a5018..dcbbb16 100644 --- a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java @@ -160,6 +160,35 @@ public void aVerbatimCopyKeepsChecksumsValid() throws IOException { } } + /** + * Asking for checksums applies them to carried-over files too, by + * re-encoding rather than copying. Otherwise the option quietly means "on + * whichever files happened to be re-encoded anyway", and the archive ends + * up half checksummed. + */ + @Test + public void checksumsAreAddedToCarriedOverFilesToo() throws IOException { + final byte[] content = incompressible(9_500, 5); + final byte[] without = MpqArchiveWriter.create(MpqWriteOptions.defaults()) + .put("carried.bin", content) + .toByteArray(); + + try (MpqArchive source = MpqArchive.open(without, MpqOpenOptions.defaults())) { + Assert.assertFalse(source.entry("carried.bin").orElseThrow() + .has(MpqFileEntry.FLAG_SECTOR_CRC), "nothing to carry over yet"); + + final byte[] with = MpqArchiveWriter + .from(source, MpqWriteOptions.defaults().withSectorChecksums(true)) + .toByteArray(); + + try (MpqArchive rebuilt = MpqArchive.open(with, MpqOpenOptions.defaults())) { + Assert.assertTrue(rebuilt.entry("carried.bin").orElseThrow() + .has(MpqFileEntry.FLAG_SECTOR_CRC), "the rebuild should have added them"); + Assert.assertEquals(rebuilt.read("carried.bin"), content); + } + } + } + /** Where a file's first sector payload begins, past its offset table. */ private static int payloadStart(byte[] image, String name) throws IOException { try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { From 9740074d0114d264c102158ff95b0c868ede098d Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 10:38:19 +0200 Subject: [PATCH 03/13] Address review: digest recording, patch-bit bounds, HET/BET coverage Every version 3 header carries all six MD5 fields, so their presence says nothing about whether a digest was recorded. hasDigests() looked at the lengths, which are always 16, so an archive that left the fields blank reported VERIFIED -- agreement with digests nobody computed. It now applies the same all-zero convention matchesDigest already used. Integrity.VERIFIED promises every recorded digest matched, so it has to cover the HET and BET tables too. This library does not read those tables but it can check their digests, and skipping them let an archive whose HET table was the damaged one report clean. The patch-bit array turns out to have two legal lengths, because StormLib disagrees with itself: GetSizeOfAttributesFile sizes it (n + 6) / 8 while the loader sizes it (n + 7) / 8. Those differ when n is congruent to 1 modulo 8 -- a one-block archive is allotted zero bytes for one bit. Parsing now accepts either length, bits the file does not reach read as unset instead of out of bounds, and what we emit is the length that holds every bit so a write cannot leave the buffer. Version3IntegrityTests builds a version 3 archive by relocating a version 1 one, which also gives the v2-v4 read path its first end-to-end coverage. --- docs/mpq-format-notes.md | 36 ++- src/main/java/org/inwc3/jmpq/MpqArchive.java | 47 +++- .../java/org/inwc3/jmpq/MpqAttributes.java | 96 +++++-- src/main/java/org/inwc3/jmpq/MpqHeader.java | 42 ++- .../crigges/jmpq3test/MpqAttributesTests.java | 87 ++++++ .../jmpq3test/Version3IntegrityTests.java | 266 ++++++++++++++++++ 6 files changed, 534 insertions(+), 40 deletions(-) create mode 100644 src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java diff --git a/docs/mpq-format-notes.md b/docs/mpq-format-notes.md index dd6f790..c1cecfa 100644 --- a/docs/mpq-format-notes.md +++ b/docs/mpq-format-notes.md @@ -315,9 +315,24 @@ declared bytemask and accepts either `n` or `n - 1` entries, reporting which via outside the four known ones are preserved in `flags()` but their arrays cannot be located, so parsing stops after the known prefix — as StormLib does. -The patch-bit array is `(n + 6) / 8` bytes, which is StormLib's own formula: it -rounds up and then tolerates a spare byte, rather than the `(n + 7) / 8` you -would expect. +### The patch-bit array has two lengths, because StormLib disagrees with itself + +`GetSizeOfAttributesFile` sizes it as `(dwBlockTableSize + 6) / 8`, and the +loader sizes the same array as `(dwAttributesEntries + 7) / 8`. Those differ +whenever `n` is congruent to 1 modulo 8: a one-block archive is allotted **zero** +bytes for one bit, and a nine-block archive gets one byte for nine bits. + +This is not a reading error on our part — both expressions are in StormLib, in +functions that describe the same array. So both lengths occur in the wild. + +**Decision.** `MpqAttributes.parse` accepts either length (and either, combined +with the tolerated one-entry-short count, so four lengths in total are legal for +one archive). Bits the file does not physically reach are read as unset rather +than read out of bounds. What we *emit* is `(n + 7) / 8`, the length that holds +every bit, so a write can never leave the buffer. + +The bit order is most-significant-first, per StormLib's +`dwBitMask = (dwBitMask << 0x07) | (dwBitMask >> 0x01)` starting from `0x80`. ## 12. The hi-block table is plain; the hash and block tables may not be @@ -352,6 +367,17 @@ not refuse the archive. **Decision.** `MpqArchive.integrity()` returns `UNRECORDED`, `VERIFIED` or `MISMATCHED`, and a mismatch is logged. Refusing to open would throw away an -archive whose tables may decode every file perfectly. An all-zero digest counts -as "not recorded" rather than as the digest of those bytes. +archive whose tables may decode every file perfectly. + +Two things follow from the digests being optional. An all-zero field counts as +"not recorded" rather than as the digest of those bytes — and since *every* +version 3 header carries all six fields, whether a digest exists can only be +decided by looking at its contents, never by its presence. A header that left +them blank must report `UNRECORDED`; reporting `VERIFIED` would claim agreement +with digests nobody computed. + +And `VERIFIED` has to mean *every* recorded digest, including the HET and BET +tables. This library does not read those tables, but it does check their digests: +skipping them would let an archive whose HET table is the damaged one report +clean. diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index 5c72a16..3170a1c 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -253,6 +253,26 @@ public Optional attributes() { } } + /** + * Compares one region of the file against a recorded digest. + *

+ * A region that does not fit in the file cannot be digested, and is not + * counted as a mismatch: the header is describing something that is not + * there, which the table readers report on their own terms. + * + * @param offset where the region starts. + * @param length how long it is. + * @param digest the expected digest, or blank to skip. + * @return whether it matched, or true when there was nothing to compare. + */ + private boolean matchesRegion(long offset, long length, byte[] digest) throws IOException { + if (length <= 0 || length > Integer.MAX_VALUE - 8 + || !source.contains(offset, length)) { + return true; + } + return MpqHeader.matchesDigest(source.bytes(offset, (int) length), digest); + } + /** * Checks the tables against the MD5 digests a version 3 header records. *

@@ -267,19 +287,26 @@ private Integrity checkTableDigests() throws IOException { } boolean matched = header.verifyHeaderDigest(source); - matched &= MpqHeader.matchesDigest( - source.bytes(header.hashTableFileOffset(), (int) header.hashTableStoredSize()), + matched &= matchesRegion(header.hashTableFileOffset(), header.hashTableStoredSize(), extended.md5HashTable()); - matched &= MpqHeader.matchesDigest( - source.bytes(header.blockTableFileOffset(), (int) header.blockTableStoredSize()), + matched &= matchesRegion(header.blockTableFileOffset(), header.blockTableStoredSize(), extended.md5BlockTable()); if (header.hasHiBlockTable()) { - final long bytes = (long) header.blockTableEntries() * MpqHeader.HI_BLOCK_ENTRY_SIZE; - if (source.contains(header.hiBlockTableFileOffset(), bytes)) { - matched &= MpqHeader.matchesDigest( - source.bytes(header.hiBlockTableFileOffset(), (int) bytes), - extended.md5HiBlockTable()); - } + matched &= matchesRegion(header.hiBlockTableFileOffset(), + (long) header.blockTableEntries() * MpqHeader.HI_BLOCK_ENTRY_SIZE, + extended.md5HiBlockTable()); + } + // The extended tables are not read by this library, but their digests + // are still recorded, and Integrity.VERIFIED claims every recorded + // digest matched. Skipping them would make that claim false for an + // archive whose HET or BET table is the damaged one. + if (header.hetTablePosition() != 0) { + matched &= matchesRegion(header.hetTableFileOffset(), + extended.hetTableCompressedSize(), extended.md5HetTable()); + } + if (header.betTablePosition() != 0) { + matched &= matchesRegion(header.betTableFileOffset(), + extended.betTableCompressedSize(), extended.md5BetTable()); } if (!matched) { diff --git a/src/main/java/org/inwc3/jmpq/MpqAttributes.java b/src/main/java/org/inwc3/jmpq/MpqAttributes.java index b800e44..80434db 100644 --- a/src/main/java/org/inwc3/jmpq/MpqAttributes.java +++ b/src/main/java/org/inwc3/jmpq/MpqAttributes.java @@ -111,6 +111,10 @@ public static long toUnixMillis(long fileTime) { * @return the exact file length. */ public static long sizeFor(int flags, int entries) { + return sizeFor(flags, entries, patchBitBytesStormLibWrites(entries)); + } + + private static long sizeFor(int flags, int entries, long patchBytes) { long size = HEADER_SIZE; if ((flags & HAS_CRC32) != 0) { size += 4L * entries; @@ -122,13 +126,37 @@ public static long sizeFor(int flags, int entries) { size += 16L * entries; } if ((flags & HAS_PATCH_BIT) != 0) { - // StormLib rounds up and then allows a spare byte, rather than the - // (entries + 7) / 8 you would expect. - size += (entries + 6L) / 8; + size += patchBytes; } return size; } + /** + * Patch-bit bytes as StormLib counts them in + * {@code GetSizeOfAttributesFile}: {@code (n + 6) / 8}. + *

+ * That is one byte short of holding {@code n} bits whenever {@code n} is + * congruent to 1 modulo 8 -- a one-block archive is allotted zero bytes for + * one bit. StormLib is inconsistent with itself here: its loader sizes the + * same array as {@code (n + 7) / 8}. Both lengths therefore occur, so + * parsing accepts either and nothing indexes past what a file holds. + * + * @param entries number of blocks described. + * @return the byte count StormLib writes. + */ + private static long patchBitBytesStormLibWrites(int entries) { + return (entries + 6L) / 8; + } + + /** + * @param entries number of blocks described. + * @return bytes actually needed to hold that many bits, which is what this + * implementation emits so a write can never leave the buffer. + */ + private static long patchBitBytesNeeded(int entries) { + return (entries + 7L) / 8; + } + /** * {@link #sizeFor} narrowed for an allocation. *

@@ -142,7 +170,11 @@ public static long sizeFor(int flags, int entries) { * @return the size as an {@code int}. */ private static int inMemorySize(int flags, int entries) { - final long size = sizeFor(flags, entries); + return inMemorySize(flags, entries, patchBitBytesStormLibWrites(entries)); + } + + private static int inMemorySize(int flags, int entries, long patchBytes) { + final long size = sizeFor(flags, entries, patchBytes); if (size > Integer.MAX_VALUE - 8) { throw new IllegalArgumentException("Attributes for " + entries + " blocks would need " + size + " bytes, more than can be held in memory."); @@ -150,6 +182,35 @@ private static int inMemorySize(int flags, int entries) { return (int) size; } + /** + * Works out how many blocks a file of this length describes. + *

+ * Four lengths are legal for one archive: the block count or one fewer -- + * StormLib tolerates a short file, because the tool writing it is rarely the + * tool reading it -- each with either patch-bit length, since StormLib + * writes one and reads the other. A length matching none is reported rather + * than guessed at. + * + * @param usable the bytemask, restricted to arrays we understand. + * @param blockCount block table rows the archive has. + * @param length the file length. + * @return the entry count that length implies. + * @throws JMpqException if no candidate matches. + */ + private static int resolveEntryCount(int usable, int blockCount, int length) + throws JMpqException { + final int fewest = Math.max(0, blockCount - 1); + for (int entries = blockCount; entries >= fewest; entries--) { + if (sizeFor(usable, entries, patchBitBytesStormLibWrites(entries)) == length + || sizeFor(usable, entries, patchBitBytesNeeded(entries)) == length) { + return entries; + } + } + throw new JMpqException("An attributes file with flags 0x" + + Integer.toHexString(usable) + " for " + blockCount + " blocks should be " + + sizeFor(usable, blockCount) + " bytes, but is " + length + "."); + } + /** * Parses an attributes file. * @@ -173,19 +234,8 @@ public static MpqAttributes parse(byte[] data, int blockCount) throws JMpqExcept // ignoring the rest is what StormLib does. final int usable = flags & KNOWN_FLAGS; - final int entries; - final boolean truncated; - if (sizeFor(usable, blockCount) == data.length) { - entries = blockCount; - truncated = false; - } else if (blockCount > 0 && sizeFor(usable, blockCount - 1) == data.length) { - entries = blockCount - 1; - truncated = true; - } else { - throw new JMpqException("An attributes file with flags 0x" - + Integer.toHexString(flags) + " for " + blockCount + " blocks should be " - + sizeFor(usable, blockCount) + " bytes, but is " + data.length + "."); - } + final int entries = resolveEntryCount(usable, blockCount, data.length); + final boolean truncated = entries != blockCount; final int[] crc32 = (usable & HAS_CRC32) != 0 ? new int[entries] : new int[0]; for (int i = 0; i < crc32.length; i++) { @@ -204,7 +254,11 @@ public static MpqAttributes parse(byte[] data, int blockCount) throws JMpqExcept (usable & HAS_PATCH_BIT) != 0 ? new boolean[entries] : new boolean[0]; final int bitsAt = in.position(); for (int i = 0; i < patchBits.length; i++) { - patchBits[i] = (data[bitsAt + (i >>> 3)] & (0x80 >>> (i & 7))) != 0; + final int at = bitsAt + (i >>> 3); + // A file written to StormLib own size formula can be a byte short of + // its own bit count. The entries it does not reach are left unmarked, + // which beats refusing the file or reading past its end. + patchBits[i] = at < data.length && (data[at] & (0x80 >>> (i & 7))) != 0; } return new MpqAttributes(version, flags, crc32, fileTimes, md5, patchBits, truncated); @@ -245,8 +299,10 @@ public static byte[] build(int[] crc32, long[] fileTimes) { */ public byte[] toByteArray() { final int emitted = flags & KNOWN_FLAGS; + // Sized to hold every bit rather than to StormLib short formula, so a + // block count congruent to 1 modulo 8 cannot write past the buffer. final ByteBuffer out = ByteBuffer - .allocate(inMemorySize(emitted, entries())) + .allocate(inMemorySize(emitted, entries(), patchBitBytesNeeded(entries()))) .order(ByteOrder.LITTLE_ENDIAN); out.putInt(version); out.putInt(emitted); @@ -260,7 +316,7 @@ public byte[] toByteArray() { out.put(digest); } if (patchBits.length > 0) { - final byte[] bits = new byte[(int) ((patchBits.length + 6L) / 8)]; + final byte[] bits = new byte[(int) patchBitBytesNeeded(patchBits.length)]; for (int i = 0; i < patchBits.length; i++) { if (patchBits[i]) { bits[i >>> 3] |= (byte) (0x80 >>> (i & 7)); diff --git a/src/main/java/org/inwc3/jmpq/MpqHeader.java b/src/main/java/org/inwc3/jmpq/MpqHeader.java index 2bcefcd..cb634d6 100644 --- a/src/main/java/org/inwc3/jmpq/MpqHeader.java +++ b/src/main/java/org/inwc3/jmpq/MpqHeader.java @@ -161,11 +161,29 @@ public record Extended( new byte[0], new byte[0], new byte[0], new byte[0], new byte[0], new byte[0]); /** - * @return whether any digest was recorded, and so whether validating - * the tables against them is meaningful. + * Whether any digest was actually recorded. + *

+ * Every version 3 header has all six fields present, so their lengths + * say nothing: an archive that simply left them blank still carries + * sixteen zero bytes each. Only a non-zero digest is a recorded one, + * which is the same convention {@link #matchesDigest} applies, and + * without it a blank version 3 header reported its tables as verified + * against digests nobody had computed. + * + * @return whether validating the tables is meaningful. */ public boolean hasDigests() { - return md5HashTable.length == DIGEST_SIZE || md5BlockTable.length == DIGEST_SIZE; + return isRecorded(md5HashTable) || isRecorded(md5BlockTable) + || isRecorded(md5HiBlockTable) || isRecorded(md5HetTable) + || isRecorded(md5BetTable) || isRecorded(md5Header); + } + + /** + * @param digest a digest field. + * @return whether it holds a digest rather than being absent or blank. + */ + static boolean isRecorded(byte[] digest) { + return digest.length == DIGEST_SIZE && !isAllZero(digest); } @Override @@ -242,6 +260,20 @@ public long hiBlockTableFileOffset() { return headerOffset + hiBlockTablePosition; } + /** + * @return absolute file offset of the HET table. + */ + public long hetTableFileOffset() { + return headerOffset + hetTablePosition; + } + + /** + * @return absolute file offset of the BET table. + */ + public long betTableFileOffset() { + return headerOffset + betTablePosition; + } + /** * Stored length of the hash table. *

@@ -305,7 +337,7 @@ public boolean verifyHeaderDigest(MpqSource source) throws JMpqException { * compare against. */ static boolean matchesDigest(byte[] data, byte[] digest) { - if (digest.length != Extended.DIGEST_SIZE || isAllZero(digest)) { + if (!Extended.isRecorded(digest)) { // StormLib treats an all-zero digest as "not recorded" rather than // as the digest of these bytes. return true; @@ -318,7 +350,7 @@ static boolean matchesDigest(byte[] data, byte[] digest) { } } - private static boolean isAllZero(byte[] digest) { + static boolean isAllZero(byte[] digest) { for (byte value : digest) { if (value != 0) { return false; diff --git a/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java b/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java index 92cbc7d..d9a94b4 100644 --- a/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java +++ b/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java @@ -181,6 +181,93 @@ public void theDeprecatedParserNoLongerLosesAnEntry() { } } + /** + * StormLib sizes the patch-bit array as {@code (n + 6) / 8} but loads it as + * {@code (n + 7) / 8}, so for a block count congruent to 1 modulo 8 the file + * it writes is one byte short of its own bit count -- a one-block archive is + * allotted zero bytes for one bit. + *

+ * Both lengths therefore occur, and neither may be read past. Reading the + * short form used to throw {@link ArrayIndexOutOfBoundsException} on an + * otherwise length-valid file. + */ + @Test + public void bothPatchBitLengthsAreAcceptedAndNeitherIsReadPast() throws JMpqException { + // One block: StormLib allots (1 + 6) / 8 = 0 bytes for the bit. + final byte[] oneBlockShort = patchBitFile(1, 0); + Assert.assertEquals(oneBlockShort.length, 8); + final MpqAttributes one = MpqAttributes.parse(oneBlockShort, 1); + Assert.assertEquals(one.entries(), 1); + Assert.assertEquals(one.patchBits(), new boolean[]{false}, + "the bit was never stored, so it is not set"); + + // Nine blocks: (9 + 6) / 8 = 1 byte, one short of the nine bits. + final byte[] nineShort = patchBitFile(9, 1); + nineShort[8] = (byte) 0b1000_0001; + final MpqAttributes nine = MpqAttributes.parse(nineShort, 9); + Assert.assertEquals(nine.entries(), 9); + Assert.assertTrue(nine.patchBits()[0]); + Assert.assertTrue(nine.patchBits()[7]); + Assert.assertFalse(nine.patchBits()[8], "the ninth bit had nowhere to live"); + + // The same nine blocks written to the length that actually holds them. + final byte[] nineFull = patchBitFile(9, 2); + nineFull[8] = (byte) 0b1000_0001; + nineFull[9] = (byte) 0b1000_0000; + final MpqAttributes read = MpqAttributes.parse(nineFull, 9); + Assert.assertEquals(read.entries(), 9); + Assert.assertTrue(read.patchBits()[8], "now it does"); + } + + /** + * What this implementation emits is the length that holds every bit, so a + * write can never leave the buffer, and it must parse back unchanged. + */ + @Test + public void emittedPatchBitsRoundTripAtEveryAwkwardCount() throws JMpqException { + for (int entries : new int[]{1, 7, 8, 9, 16, 17}) { + final boolean[] bits = new boolean[entries]; + bits[entries - 1] = true; + bits[0] = true; + final MpqAttributes attributes = new MpqAttributes(MpqAttributes.VERSION, + MpqAttributes.HAS_PATCH_BIT, new int[0], new long[0], new byte[0][], + bits, false); + + final MpqAttributes read = MpqAttributes.parse(attributes.toByteArray(), entries); + Assert.assertEquals(read.entries(), entries, "entries for " + entries); + Assert.assertEquals(read.patchBits(), bits, "bits for " + entries); + } + } + + /** Patch bits sit after the arrays that precede them, not at a fixed offset. */ + @Test + public void patchBitsAreReadAfterTheArraysBeforeThem() throws JMpqException { + final int entries = 9; + final int flags = MpqAttributes.HAS_CRC32 | MpqAttributes.HAS_PATCH_BIT; + final ByteBuffer out = ByteBuffer + .allocate((int) MpqAttributes.sizeFor(flags, entries)) + .order(ByteOrder.LITTLE_ENDIAN); + out.putInt(MpqAttributes.VERSION); + out.putInt(flags); + for (int i = 0; i < entries; i++) { + out.putInt(0x500 + i); + } + out.put((byte) 0b0100_0000); + + final MpqAttributes attributes = MpqAttributes.parse(out.array(), entries); + Assert.assertEquals(attributes.crc32Of(8), 0x508); + Assert.assertFalse(attributes.patchBits()[0]); + Assert.assertTrue(attributes.patchBits()[1]); + } + + /** A patch-bit-only attributes file of the given length in bit bytes. */ + private static byte[] patchBitFile(int entries, int patchBytes) { + final ByteBuffer out = ByteBuffer.allocate(8 + patchBytes).order(ByteOrder.LITTLE_ENDIAN); + out.putInt(MpqAttributes.VERSION); + out.putInt(MpqAttributes.HAS_PATCH_BIT); + return out.array(); + } + /** * A bytemask naming nothing this implementation knows describes no entries. * Worth its own test: the length-driven count only terminates because of diff --git a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java new file mode 100644 index 0000000..dea6dca --- /dev/null +++ b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java @@ -0,0 +1,266 @@ +package systems.crigges.jmpq3test; + +import org.inwc3.jmpq.MpqArchive; +import org.inwc3.jmpq.MpqArchiveWriter; +import org.inwc3.jmpq.MpqHeader; +import org.inwc3.jmpq.MpqOpenOptions; +import org.inwc3.jmpq.MpqWriteOptions; +import org.testng.Assert; +import org.testng.annotations.Test; +import systems.crigges.jmpq3.security.MPQEncryption; +import systems.crigges.jmpq3.security.MPQHashGenerator; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; + +/** + * The version 3 MD5 digests, and what {@link MpqArchive.Integrity} may claim. + *

+ * The library cannot write version 3, so these fixtures are built by rewriting a + * version 1 archive's header as a 208-byte one and shifting everything after it. + * That is still a synthetic fixture rather than a StormLib-generated archive — + * see P2-2 in {@code AUDIT.md} — but it does exercise the real parse and digest + * paths rather than only their inputs. + */ +public class Version3IntegrityTests { + + /** A version 3 header is this much longer than a version 0 one. */ + private static final int SHIFT = 208 - 32; + + private static final int DIGEST_SIZE = 16; + + /** Offsets of the six digests within a version 3 header. */ + private static final int MD5_BLOCK_TABLE = 0x70; + private static final int MD5_HASH_TABLE = 0x80; + private static final int MD5_HI_BLOCK_TABLE = 0x90; + private static final int MD5_BET_TABLE = 0xA0; + private static final int MD5_HET_TABLE = 0xB0; + private static final int MD5_HEADER = 0xC0; + + /** + * All six digest fields exist in every version 3 header, so their presence + * says nothing. An archive that left them blank has not recorded anything, + * and reporting its tables as verified against digests nobody computed is a + * claim the archive never made. + */ + @Test + public void blankDigestsAreNotRecordedDigests() throws IOException { + final byte[] image = version3(source(), false, false); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.header().formatVersion(), 3); + Assert.assertFalse(archive.header().extended().hasDigests()); + Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.UNRECORDED); + Assert.assertEquals(archive.read("a.txt"), content()); + } + } + + /** With the digests filled in and the tables intact, everything matches. */ + @Test + public void recordedDigestsThatMatchReportVerified() throws IOException { + final byte[] image = version3(source(), true, false); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertTrue(archive.header().extended().hasDigests()); + Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.VERIFIED); + Assert.assertEquals(archive.read("a.txt"), content()); + } + } + + /** + * A damaged table is reported rather than refused. StormLib does the same: + * the tables may still decode every file, and refusing the archive would + * throw away data that is actually recoverable. + */ + @Test + public void aTableThatDoesNotMatchItsDigestIsReportedNotRefused() throws IOException { + final byte[] image = version3(source(), true, false); + + // Flip a byte of the hash table, leaving its digest claiming otherwise. + final ByteBuffer header = ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN); + final int hashTableAt = header.getInt(0x10); + image[hashTableAt] ^= 0x01; + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.MISMATCHED); + } + } + + /** + * The extended tables count too. This library does not read HET or BET, but + * their digests are still recorded, and {@code VERIFIED} promises that every + * recorded digest matched — so an archive whose HET table is the damaged one + * must not report clean. + */ + @Test + public void aDamagedHetTableIsNotReportedAsVerified() throws IOException { + final byte[] intact = version3(source(), true, true); + try (MpqArchive archive = MpqArchive.open(intact, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.VERIFIED, + "the HET digest was computed over the region, so it must match"); + } + + final byte[] damaged = version3(source(), true, true); + final ByteBuffer header = ByteBuffer.wrap(damaged).order(ByteOrder.LITTLE_ENDIAN); + final int hetAt = (int) header.getLong(0x3C); + damaged[hetAt] ^= 0x01; + + try (MpqArchive archive = MpqArchive.open(damaged, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.MISMATCHED, + "a recorded HET digest that does not match is still a mismatch"); + } + } + + // ------------------------------------------------------------- fixtures + + private static byte[] content() { + return "version three content".getBytes(StandardCharsets.UTF_8); + } + + /** + * The archive to rewrite. No list file, because the writer encrypts that one + * with a position-adjusted key and this fixture moves every file. + */ + private static byte[] source() throws IOException { + return MpqArchiveWriter + .create(MpqWriteOptions.defaults() + .withFormatVersion(1) + .withPrefix(false) + .withListfile(false)) + .put("a.txt", content()) + .toByteArray(); + } + + /** + * Rewrites a version 1 archive as a version 3 one. + *

+ * The header grows from 44 to 208 bytes at the same offset, so everything + * after it moves by {@link #SHIFT} and every position recorded in the header + * and the block table moves with it. The block table has to be decrypted to + * be adjusted and then re-encrypted, which is why the source archive must + * have no files keyed on their own position. + * + * @param v1 the source archive, header at offset 0. + * @param digests whether to record the MD5 digests. + * @param extendedTables whether to plant a HET/BET region and record its + * digest, to check that those are covered too. + * @return a version 3 archive. + */ + private static byte[] version3(byte[] v1, boolean digests, boolean extendedTables) { + final ByteBuffer in = ByteBuffer.wrap(v1).order(ByteOrder.LITTLE_ENDIAN); + Assert.assertEquals(in.getInt(0), MpqHeader.ARCHIVE_SIGNATURE, "header must be at 0"); + + final long hashPosition = Integer.toUnsignedLong(in.getInt(0x10)) + SHIFT; + final long blockPosition = Integer.toUnsignedLong(in.getInt(0x14)) + SHIFT; + final int hashEntries = in.getInt(0x18); + final int blockEntries = in.getInt(0x1C); + final int sectorShift = in.getShort(0x0E) & 0xFF; + + // A spare region standing in for HET/BET. Its contents are never parsed; + // only its digest is, which is the whole point. + final byte[] extended = new byte[64]; + for (int i = 0; i < extended.length; i++) { + extended[i] = (byte) (i * 7 + 1); + } + + final int bodyLength = v1.length - 32; + final int extendedAt = 208 + bodyLength; + final byte[] out = new byte[extendedAt + (extendedTables ? extended.length : 0)]; + System.arraycopy(v1, 32, out, 208, bodyLength); + if (extendedTables) { + System.arraycopy(extended, 0, out, extendedAt, extended.length); + } + + final ByteBuffer header = ByteBuffer.wrap(out).order(ByteOrder.LITTLE_ENDIAN); + header.putInt(0x00, MpqHeader.ARCHIVE_SIGNATURE); + header.putInt(0x04, 208); + header.putInt(0x08, out.length); + header.putShort(0x0C, (short) 3); + header.putShort(0x0E, (short) sectorShift); + header.putInt(0x10, (int) hashPosition); + header.putInt(0x14, (int) blockPosition); + header.putInt(0x18, hashEntries); + header.putInt(0x1C, blockEntries); + header.putLong(0x20, 0); + header.putLong(0x2C, out.length); + if (extendedTables) { + // BET at 0x34, HET at 0x3C, both pointing at the spare region. + header.putLong(0x34, extendedAt); + header.putLong(0x3C, extendedAt); + header.putLong(0x5C, extended.length); + header.putLong(0x64, extended.length); + } + // Stored lengths equal to the plain lengths: not compressed. + header.putLong(0x44, (long) hashEntries * MpqHeader.HASH_ENTRY_SIZE); + header.putLong(0x4C, (long) blockEntries * MpqHeader.BLOCK_ENTRY_SIZE); + + shiftBlockTable(out, (int) blockPosition, blockEntries); + + if (digests) { + recordDigests(out, (int) hashPosition, hashEntries, (int) blockPosition, blockEntries, + extendedTables ? extendedAt : -1, extended.length); + } + return out; + } + + /** Adds {@link #SHIFT} to every file position in the encrypted block table. */ + private static void shiftBlockTable(byte[] image, int at, int entries) { + final int length = entries * MpqHeader.BLOCK_ENTRY_SIZE; + final byte[] table = new byte[length]; + System.arraycopy(image, at, table, 0, length); + + new MPQEncryption(tableKey("(block table)"), true) + .processSingle(ByteBuffer.wrap(table)); + + final ByteBuffer rows = ByteBuffer.wrap(table).order(ByteOrder.LITTLE_ENDIAN); + for (int i = 0; i < entries; i++) { + final int position = rows.getInt(i * MpqHeader.BLOCK_ENTRY_SIZE); + rows.putInt(i * MpqHeader.BLOCK_ENTRY_SIZE, position + SHIFT); + } + + new MPQEncryption(tableKey("(block table)"), false) + .processSingle(ByteBuffer.wrap(table)); + System.arraycopy(table, 0, image, at, length); + } + + private static void recordDigests(byte[] image, int hashAt, int hashEntries, + int blockAt, int blockEntries, + int extendedAt, int extendedLength) { + put(image, MD5_HASH_TABLE, md5(image, hashAt, hashEntries * MpqHeader.HASH_ENTRY_SIZE)); + put(image, MD5_BLOCK_TABLE, md5(image, blockAt, blockEntries * MpqHeader.BLOCK_ENTRY_SIZE)); + put(image, MD5_HI_BLOCK_TABLE, new byte[DIGEST_SIZE]); + if (extendedAt >= 0) { + put(image, MD5_HET_TABLE, md5(image, extendedAt, extendedLength)); + put(image, MD5_BET_TABLE, md5(image, extendedAt, extendedLength)); + } else { + put(image, MD5_HET_TABLE, new byte[DIGEST_SIZE]); + put(image, MD5_BET_TABLE, new byte[DIGEST_SIZE]); + } + // The header digest covers the header up to but not including itself. + put(image, MD5_HEADER, md5(image, 0, MD5_HEADER)); + } + + private static void put(byte[] image, int at, byte[] digest) { + System.arraycopy(digest, 0, image, at, DIGEST_SIZE); + } + + private static byte[] md5(byte[] image, int at, int length) { + try { + final MessageDigest digest = MessageDigest.getInstance("MD5"); + digest.update(image, at, length); + return digest.digest(); + } catch (NoSuchAlgorithmException impossible) { + throw new AssertionError(impossible); + } + } + + private static int tableKey(String name) { + final MPQHashGenerator hasher = MPQHashGenerator.getFileKeyGenerator(); + hasher.process(name); + return hasher.getHash(); + } +} From 83daf9b444d379eea1a0d3abd2810c6e2ea84015 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 10:52:37 +0200 Subject: [PATCH 04/13] Address review: compressed-table screening and unhashable digests The header scan screened candidates on room for the uncompressed hash table, which rules out a valid version 3 archive whose compressed table sits at the end of the file -- and once such a header is rejected, a decoy planted earlier wins the scan, which is the reverse of what the check is for. This was a regression introduced with the scan change earlier in this branch. The stored length is now used where the header declares one. A recorded digest whose region is absent, empty or outside the file counted as a match, so VERIFIED could be reported with nothing hashed. Since the HET and BET tables are not otherwise read or validated, that path was the only thing between a damaged extended table and a clean report. It is now a mismatch, and the table-absent case is passed in explicitly rather than inferred from a zero position. The fixtures grew a genuinely compressed hash table on the way, which compressed tables had no coverage of at all before now. --- src/main/java/org/inwc3/jmpq/MpqArchive.java | 64 ++-- .../java/org/inwc3/jmpq/MpqAttributes.java | 10 +- src/main/java/org/inwc3/jmpq/MpqHeader.java | 26 +- .../jmpq3test/Version3IntegrityTests.java | 297 ++++++++++++------ 4 files changed, 278 insertions(+), 119 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index 3170a1c..b1fd916 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -256,20 +256,36 @@ public Optional attributes() { /** * Compares one region of the file against a recorded digest. *

- * A region that does not fit in the file cannot be digested, and is not - * counted as a mismatch: the header is describing something that is not - * there, which the table readers report on their own terms. + * A recorded digest whose region is absent, empty or outside the file counts + * as a mismatch rather than a pass. Nothing was hashed, so the archive + * cannot be said to agree with its own digests — and for the HET and BET + * tables, which this library does not otherwise read or validate, letting + * that path succeed would be the only thing standing between a damaged + * extended table and a clean bill of health. * - * @param offset where the region starts. - * @param length how long it is. - * @param digest the expected digest, or blank to skip. - * @return whether it matched, or true when there was nothing to compare. + * @param present whether the header says this table exists at all. + * @param offset where the region starts. + * @param length how long it is. + * @param digest the expected digest, or blank to skip. + * @return whether it matched, or true when nothing was recorded. */ - private boolean matchesRegion(long offset, long length, byte[] digest) throws IOException { - if (length <= 0 || length > Integer.MAX_VALUE - 8 - || !source.contains(offset, length)) { + private boolean matchesRegion(boolean present, long offset, long length, byte[] digest) + throws IOException { + if (!MpqHeader.Extended.isRecorded(digest)) { + // Nothing was recorded, so there is nothing to agree or disagree + // with. This is the usual case for a header that left the optional + // digests blank. return true; } + if (!present || length <= 0 || length > Integer.MAX_VALUE - 8 + || !source.contains(offset, length)) { + // A digest was recorded for bytes the header cannot actually point + // at. Counting that as a match would let VERIFIED be reported when + // nothing was hashed, which is the one thing it must not mean. + log.warn("{} records a digest for a {} byte region at {} that is not in the file.", + source.origin(), length, offset); + return false; + } return MpqHeader.matchesDigest(source.bytes(offset, (int) length), digest); } @@ -287,27 +303,21 @@ private Integrity checkTableDigests() throws IOException { } boolean matched = header.verifyHeaderDigest(source); - matched &= matchesRegion(header.hashTableFileOffset(), header.hashTableStoredSize(), - extended.md5HashTable()); - matched &= matchesRegion(header.blockTableFileOffset(), header.blockTableStoredSize(), - extended.md5BlockTable()); - if (header.hasHiBlockTable()) { - matched &= matchesRegion(header.hiBlockTableFileOffset(), - (long) header.blockTableEntries() * MpqHeader.HI_BLOCK_ENTRY_SIZE, - extended.md5HiBlockTable()); - } + matched &= matchesRegion(true, header.hashTableFileOffset(), + header.hashTableStoredSize(), extended.md5HashTable()); + matched &= matchesRegion(true, header.blockTableFileOffset(), + header.blockTableStoredSize(), extended.md5BlockTable()); + matched &= matchesRegion(header.hasHiBlockTable(), header.hiBlockTableFileOffset(), + (long) header.blockTableEntries() * MpqHeader.HI_BLOCK_ENTRY_SIZE, + extended.md5HiBlockTable()); // The extended tables are not read by this library, but their digests // are still recorded, and Integrity.VERIFIED claims every recorded // digest matched. Skipping them would make that claim false for an // archive whose HET or BET table is the damaged one. - if (header.hetTablePosition() != 0) { - matched &= matchesRegion(header.hetTableFileOffset(), - extended.hetTableCompressedSize(), extended.md5HetTable()); - } - if (header.betTablePosition() != 0) { - matched &= matchesRegion(header.betTableFileOffset(), - extended.betTableCompressedSize(), extended.md5BetTable()); - } + matched &= matchesRegion(header.hetTablePosition() != 0, header.hetTableFileOffset(), + extended.hetTableCompressedSize(), extended.md5HetTable()); + matched &= matchesRegion(header.betTablePosition() != 0, header.betTableFileOffset(), + extended.betTableCompressedSize(), extended.md5BetTable()); if (!matched) { log.warn("{} does not match the MD5 digests in its own header;" diff --git a/src/main/java/org/inwc3/jmpq/MpqAttributes.java b/src/main/java/org/inwc3/jmpq/MpqAttributes.java index 80434db..b6861a3 100644 --- a/src/main/java/org/inwc3/jmpq/MpqAttributes.java +++ b/src/main/java/org/inwc3/jmpq/MpqAttributes.java @@ -104,11 +104,17 @@ public static long toUnixMillis(long fileTime) { } /** - * Bytes needed to hold {@code entries} attributes of the given shape. + * The file length StormLib would write for this shape. + *

+ * Exact for every combination except {@link #HAS_PATCH_BIT}, where the + * format has no single answer: StormLib sizes that array one way and reads + * it another, so a patch-bit file may legally be this length or one byte + * longer. {@link #parse} accepts both, and {@link #toByteArray} emits the + * longer one, because it is the only one that holds every bit. * * @param flags the bytemask. * @param entries number of blocks described. - * @return the exact file length. + * @return the length StormLib writes. */ public static long sizeFor(int flags, int entries) { return sizeFor(flags, entries, patchBitBytesStormLibWrites(entries)); diff --git a/src/main/java/org/inwc3/jmpq/MpqHeader.java b/src/main/java/org/inwc3/jmpq/MpqHeader.java index cb634d6..6ee51c0 100644 --- a/src/main/java/org/inwc3/jmpq/MpqHeader.java +++ b/src/main/java/org/inwc3/jmpq/MpqHeader.java @@ -455,10 +455,34 @@ private static boolean isPlausible(MpqSource source, long position) throws JMpqE && hashTableEntries > 0 && sectorShift <= MAX_SECTOR_SIZE_SHIFT && source.contains(position + hashTablePosition, - (long) hashTableEntries * HASH_ENTRY_SIZE) + candidateHashTableBytes(source, position, hashTableEntries)) && source.contains(position + blockTablePosition, 0); } + /** + * How many bytes a candidate header's hash table actually occupies. + *

+ * From version 3 a hash table may be stored compressed, so requiring room + * for the uncompressed table would rule out a valid header whose compressed + * table sits near the end of the file — and if a decoy header came first, + * the scan would then settle on the decoy. That is the reverse of what this + * check is for, so the stored length is used where the header declares one. + * + * @param source the archive bytes. + * @param position the candidate header offset. + * @param entries declared hash table entries. + * @return bytes to require at the hash table position. + */ + private static long candidateHashTableBytes(MpqSource source, long position, int entries) + throws JMpqException { + final long plain = (long) entries * HASH_ENTRY_SIZE; + if (source.u16(position + 0x0C) < 3 || !source.contains(position, SIZE_BY_VERSION[3])) { + return plain; + } + final long stored = source.i64(position + 0x44); + return stored > 0 && stored < plain ? stored : plain; + } + /** * Parses the header at a known offset. *

diff --git a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java index dea6dca..0d4bf95 100644 --- a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java @@ -7,6 +7,8 @@ import org.inwc3.jmpq.MpqWriteOptions; import org.testng.Assert; import org.testng.annotations.Test; +import systems.crigges.jmpq3.compression.CompressionUtil; +import systems.crigges.jmpq3.compression.RecompressOptions; import systems.crigges.jmpq3.security.MPQEncryption; import systems.crigges.jmpq3.security.MPQHashGenerator; @@ -18,18 +20,17 @@ import java.security.NoSuchAlgorithmException; /** - * The version 3 MD5 digests, and what {@link MpqArchive.Integrity} may claim. + * Version 3 archives: compressed tables and the MD5 digests. *

- * The library cannot write version 3, so these fixtures are built by rewriting a - * version 1 archive's header as a 208-byte one and shifting everything after it. - * That is still a synthetic fixture rather than a StormLib-generated archive — - * see P2-2 in {@code AUDIT.md} — but it does exercise the real parse and digest - * paths rather than only their inputs. + * The library cannot write version 3, so these fixtures are built by relaying a + * version 1 archive out behind a 208-byte header. They are still synthetic + * rather than StormLib-generated — see P2-2 in {@code AUDIT.md} — but they do + * drive the real header parse, table load and digest paths rather than only + * their inputs. */ public class Version3IntegrityTests { - /** A version 3 header is this much longer than a version 0 one. */ - private static final int SHIFT = 208 - 32; + private static final int V3_HEADER_SIZE = 208; private static final int DIGEST_SIZE = 16; @@ -41,15 +42,20 @@ public class Version3IntegrityTests { private static final int MD5_HET_TABLE = 0xB0; private static final int MD5_HEADER = 0xC0; + /** Compression-type byte for deflate. */ + private static final byte TYPE_DEFLATE = 0x02; + + // ------------------------------------------------------------- digests + /** * All six digest fields exist in every version 3 header, so their presence - * says nothing. An archive that left them blank has not recorded anything, - * and reporting its tables as verified against digests nobody computed is a - * claim the archive never made. + * says nothing. An archive that left them blank recorded nothing, and + * reporting its tables verified against digests nobody computed is a claim + * the archive never made. */ @Test public void blankDigestsAreNotRecordedDigests() throws IOException { - final byte[] image = version3(source(), false, false); + final byte[] image = build(new Shape(false, false, false, false)); try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { Assert.assertEquals(archive.header().formatVersion(), 3); @@ -62,7 +68,7 @@ public void blankDigestsAreNotRecordedDigests() throws IOException { /** With the digests filled in and the tables intact, everything matches. */ @Test public void recordedDigestsThatMatchReportVerified() throws IOException { - final byte[] image = version3(source(), true, false); + final byte[] image = build(new Shape(true, false, false, false)); try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { Assert.assertTrue(archive.header().extended().hasDigests()); @@ -78,12 +84,9 @@ public void recordedDigestsThatMatchReportVerified() throws IOException { */ @Test public void aTableThatDoesNotMatchItsDigestIsReportedNotRefused() throws IOException { - final byte[] image = version3(source(), true, false); - - // Flip a byte of the hash table, leaving its digest claiming otherwise. - final ByteBuffer header = ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN); - final int hashTableAt = header.getInt(0x10); - image[hashTableAt] ^= 0x01; + final byte[] image = build(new Shape(true, false, false, false)); + final int blockTableAt = ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN).getInt(0x14); + image[blockTableAt] ^= 0x01; try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.MISMATCHED); @@ -92,38 +95,107 @@ public void aTableThatDoesNotMatchItsDigestIsReportedNotRefused() throws IOExcep /** * The extended tables count too. This library does not read HET or BET, but - * their digests are still recorded, and {@code VERIFIED} promises that every - * recorded digest matched — so an archive whose HET table is the damaged one - * must not report clean. + * their digests are recorded, and {@code VERIFIED} promises every recorded + * digest matched — so an archive whose HET table is the damaged one must not + * report clean. */ @Test public void aDamagedHetTableIsNotReportedAsVerified() throws IOException { - final byte[] intact = version3(source(), true, true); - try (MpqArchive archive = MpqArchive.open(intact, MpqOpenOptions.defaults())) { + try (MpqArchive archive = MpqArchive.open(build(new Shape(true, true, false, false)), + MpqOpenOptions.defaults())) { Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.VERIFIED, - "the HET digest was computed over the region, so it must match"); + "the HET digest was taken over the region, so it must match"); } - final byte[] damaged = version3(source(), true, true); - final ByteBuffer header = ByteBuffer.wrap(damaged).order(ByteOrder.LITTLE_ENDIAN); - final int hetAt = (int) header.getLong(0x3C); + final byte[] damaged = build(new Shape(true, true, false, false)); + final int hetAt = (int) ByteBuffer.wrap(damaged).order(ByteOrder.LITTLE_ENDIAN).getLong(0x3C); damaged[hetAt] ^= 0x01; try (MpqArchive archive = MpqArchive.open(damaged, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.MISMATCHED); + } + } + + /** + * A digest recorded for bytes the header cannot point at is a failure, not a + * pass. Nothing was hashed, so the archive cannot be said to agree with its + * own digests — and since the extended tables are not otherwise read or + * validated, letting this path succeed would be the only thing between a + * damaged HET table and a clean report. + */ + @Test + public void aDigestRecordedForBytesThatAreNotThereIsAMismatch() throws IOException { + final byte[] image = build(new Shape(true, false, false, true)); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertTrue(archive.header().extended().hasDigests()); Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.MISMATCHED, - "a recorded HET digest that does not match is still a mismatch"); + "a recorded digest over a region outside the file cannot have matched"); + // Still a readable archive: the classic tables are untouched. + Assert.assertEquals(archive.read("a.txt"), content()); } } - // ------------------------------------------------------------- fixtures + // --------------------------------------------------- compressed tables + + /** A compressed hash table is decompressed after being decrypted. */ + @Test + public void aCompressedHashTableIsRead() throws IOException { + final byte[] image = build(new Shape(true, false, true, false)); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertTrue(archive.header().isHashTableCompressed(), + "the fixture is meant to have a compressed hash table"); + Assert.assertTrue(archive.header().hashTableStoredSize() + < (long) archive.header().hashTableEntries() * MpqHeader.HASH_ENTRY_SIZE); + Assert.assertEquals(archive.integrity(), MpqArchive.Integrity.VERIFIED); + Assert.assertEquals(archive.read("a.txt"), content()); + } + } + + /** + * The header scan must not rule out a valid header whose compressed hash + * table sits at the end of the file. + *

+ * Screening candidates on room for the uncompressed table rejects + * exactly this archive, and then a decoy planted earlier in the file wins + * the scan — the reverse of what the plausibility check exists for. + */ + @Test + public void aDecoyDoesNotBeatAValidCompressedTableHeader() throws IOException { + final byte[] real = build(new Shape(true, false, true, false)); + final byte[] image = behindADecoy(real); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.header().headerOffset(), MpqHeader.ALIGNMENT, + "the decoy must not have won"); + Assert.assertTrue(archive.header().isHashTableCompressed()); + Assert.assertEquals(archive.read("a.txt"), content()); + } + } + + // ------------------------------------------------------------ fixtures + + /** + * @param digests record the MD5 digests. + * @param extendedTables plant a HET/BET region and record its digest. + * @param compressedHashTable store the hash table compressed, at the end of + * the file, where requiring room for its uncompressed + * form would run past the end. + * @param unreachableDigest record a HET digest for a region outside the file. + */ + private record Shape(boolean digests, boolean extendedTables, + boolean compressedHashTable, boolean unreachableDigest) { + } private static byte[] content() { - return "version three content".getBytes(StandardCharsets.UTF_8); + return "version three content, long enough to occupy a sector" + .getBytes(StandardCharsets.UTF_8); } /** - * The archive to rewrite. No list file, because the writer encrypts that one - * with a position-adjusted key and this fixture moves every file. + * The archive to relay. No list file, because the writer keys that one on + * its own position and this fixture moves every file. */ private static byte[] source() throws IOException { return MpqArchiveWriter @@ -132,34 +204,46 @@ private static byte[] source() throws IOException { .withPrefix(false) .withListfile(false)) .put("a.txt", content()) + .put("b.txt", "second file".getBytes(StandardCharsets.UTF_8)) .toByteArray(); } /** - * Rewrites a version 1 archive as a version 3 one. + * Relays a version 1 archive behind a version 3 header. *

- * The header grows from 44 to 208 bytes at the same offset, so everything - * after it moves by {@link #SHIFT} and every position recorded in the header - * and the block table moves with it. The block table has to be decrypted to - * be adjusted and then re-encrypted, which is why the source archive must - * have no files keyed on their own position. - * - * @param v1 the source archive, header at offset 0. - * @param digests whether to record the MD5 digests. - * @param extendedTables whether to plant a HET/BET region and record its - * digest, to check that those are covered too. - * @return a version 3 archive. + * The header grows to 208 bytes, so the file data moves and every position + * recorded in the header and the block table moves with it. The block table + * has to be decrypted to be adjusted and re-encrypted, which is why the + * source must hold no file keyed on its own position. The tables are also + * reordered so the hash table lands last, which is what lets the compressed + * variant end the file. */ - private static byte[] version3(byte[] v1, boolean digests, boolean extendedTables) { + private static byte[] build(Shape shape) throws IOException { + final byte[] v1 = source(); final ByteBuffer in = ByteBuffer.wrap(v1).order(ByteOrder.LITTLE_ENDIAN); Assert.assertEquals(in.getInt(0), MpqHeader.ARCHIVE_SIGNATURE, "header must be at 0"); - final long hashPosition = Integer.toUnsignedLong(in.getInt(0x10)) + SHIFT; - final long blockPosition = Integer.toUnsignedLong(in.getInt(0x14)) + SHIFT; + final int sourceHeaderSize = in.getInt(0x04); + final int sourceHashAt = in.getInt(0x10); + final int sourceBlockAt = in.getInt(0x14); final int hashEntries = in.getInt(0x18); final int blockEntries = in.getInt(0x1C); final int sectorShift = in.getShort(0x0E) & 0xFF; + final int hashLength = hashEntries * MpqHeader.HASH_ENTRY_SIZE; + final int blockLength = blockEntries * MpqHeader.BLOCK_ENTRY_SIZE; + final int dataLength = sourceHashAt - sourceHeaderSize; + final int shift = V3_HEADER_SIZE - sourceHeaderSize; + + final byte[] blockTable = slice(v1, sourceBlockAt, blockLength); + shiftFilePositions(blockTable, blockEntries, shift); + + byte[] hashTable = slice(v1, sourceHashAt, hashLength); + if (shape.compressedHashTable()) { + hashTable = compressTable(hashTable, tableKey("(hash table)")); + Assert.assertTrue(hashTable.length < hashLength, "the fixture must actually shrink"); + } + // A spare region standing in for HET/BET. Its contents are never parsed; // only its digest is, which is the whole point. final byte[] extended = new byte[64]; @@ -167,81 +251,116 @@ private static byte[] version3(byte[] v1, boolean digests, boolean extendedTable extended[i] = (byte) (i * 7 + 1); } - final int bodyLength = v1.length - 32; - final int extendedAt = 208 + bodyLength; - final byte[] out = new byte[extendedAt + (extendedTables ? extended.length : 0)]; - System.arraycopy(v1, 32, out, 208, bodyLength); - if (extendedTables) { + final int dataAt = V3_HEADER_SIZE; + final int extendedAt = dataAt + dataLength; + final int blockAt = extendedAt + (shape.extendedTables() ? extended.length : 0); + final int hashAt = blockAt + blockLength; + final byte[] out = new byte[hashAt + hashTable.length]; + + System.arraycopy(v1, sourceHeaderSize, out, dataAt, dataLength); + if (shape.extendedTables()) { System.arraycopy(extended, 0, out, extendedAt, extended.length); } + System.arraycopy(blockTable, 0, out, blockAt, blockLength); + System.arraycopy(hashTable, 0, out, hashAt, hashTable.length); final ByteBuffer header = ByteBuffer.wrap(out).order(ByteOrder.LITTLE_ENDIAN); header.putInt(0x00, MpqHeader.ARCHIVE_SIGNATURE); - header.putInt(0x04, 208); + header.putInt(0x04, V3_HEADER_SIZE); header.putInt(0x08, out.length); header.putShort(0x0C, (short) 3); header.putShort(0x0E, (short) sectorShift); - header.putInt(0x10, (int) hashPosition); - header.putInt(0x14, (int) blockPosition); + header.putInt(0x10, hashAt); + header.putInt(0x14, blockAt); header.putInt(0x18, hashEntries); header.putInt(0x1C, blockEntries); header.putLong(0x20, 0); header.putLong(0x2C, out.length); - if (extendedTables) { - // BET at 0x34, HET at 0x3C, both pointing at the spare region. + header.putLong(0x44, hashTable.length); + header.putLong(0x4C, blockLength); + if (shape.extendedTables()) { header.putLong(0x34, extendedAt); header.putLong(0x3C, extendedAt); header.putLong(0x5C, extended.length); header.putLong(0x64, extended.length); } - // Stored lengths equal to the plain lengths: not compressed. - header.putLong(0x44, (long) hashEntries * MpqHeader.HASH_ENTRY_SIZE); - header.putLong(0x4C, (long) blockEntries * MpqHeader.BLOCK_ENTRY_SIZE); - - shiftBlockTable(out, (int) blockPosition, blockEntries); + if (shape.unreachableDigest()) { + // A HET table the header points well past the end of the file. + header.putLong(0x3C, out.length + 0x1000L); + header.putLong(0x5C, 64); + } - if (digests) { - recordDigests(out, (int) hashPosition, hashEntries, (int) blockPosition, blockEntries, - extendedTables ? extendedAt : -1, extended.length); + if (shape.digests()) { + put(out, MD5_HASH_TABLE, md5(out, hashAt, hashTable.length)); + put(out, MD5_BLOCK_TABLE, md5(out, blockAt, blockLength)); + put(out, MD5_HI_BLOCK_TABLE, new byte[DIGEST_SIZE]); + if (shape.extendedTables()) { + put(out, MD5_HET_TABLE, md5(out, extendedAt, extended.length)); + put(out, MD5_BET_TABLE, md5(out, extendedAt, extended.length)); + } else if (shape.unreachableDigest()) { + // Recorded, and deliberately unverifiable. + final byte[] digest = new byte[DIGEST_SIZE]; + digest[0] = 0x42; + put(out, MD5_HET_TABLE, digest); + put(out, MD5_BET_TABLE, new byte[DIGEST_SIZE]); + } else { + put(out, MD5_HET_TABLE, new byte[DIGEST_SIZE]); + put(out, MD5_BET_TABLE, new byte[DIGEST_SIZE]); + } + // The header digest covers the header up to but not including itself. + put(out, MD5_HEADER, md5(out, 0, MD5_HEADER)); } return out; } - /** Adds {@link #SHIFT} to every file position in the encrypted block table. */ - private static void shiftBlockTable(byte[] image, int at, int entries) { - final int length = entries * MpqHeader.BLOCK_ENTRY_SIZE; - final byte[] table = new byte[length]; - System.arraycopy(image, at, table, 0, length); + /** Puts a header-shaped decoy in front of a real archive. */ + private static byte[] behindADecoy(byte[] real) { + final byte[] out = new byte[MpqHeader.ALIGNMENT + real.length]; + final ByteBuffer decoy = ByteBuffer.wrap(out).order(ByteOrder.LITTLE_ENDIAN); + decoy.putInt(0x00, MpqHeader.ARCHIVE_SIGNATURE); + decoy.putInt(0x04, 32); + decoy.putInt(0x10, 0x7FFF_0000); + decoy.putInt(0x14, 0x7FFF_1000); + decoy.putInt(0x18, 16); + System.arraycopy(real, 0, out, MpqHeader.ALIGNMENT, real.length); + return out; + } + + /** Compresses a table the way a version 3 archive stores one. */ + private static byte[] compressTable(byte[] stored, int key) { + final byte[] plain = stored.clone(); + new MPQEncryption(key, true).processSingle(ByteBuffer.wrap(plain)); + final byte[] deflated = CompressionUtil.compress(plain, new RecompressOptions(true)); + final byte[] out = new byte[deflated.length + 1]; + out[0] = TYPE_DEFLATE; + System.arraycopy(deflated, 0, out, 1, deflated.length); + + // Compressed first, then encrypted, which is why a reader decrypts + // first and then decompresses. + new MPQEncryption(key, false).processSingle(ByteBuffer.wrap(out)); + return out; + } + + /** Adds {@code shift} to every file position in an encrypted block table. */ + private static void shiftFilePositions(byte[] table, int entries, int shift) { new MPQEncryption(tableKey("(block table)"), true) .processSingle(ByteBuffer.wrap(table)); final ByteBuffer rows = ByteBuffer.wrap(table).order(ByteOrder.LITTLE_ENDIAN); for (int i = 0; i < entries; i++) { - final int position = rows.getInt(i * MpqHeader.BLOCK_ENTRY_SIZE); - rows.putInt(i * MpqHeader.BLOCK_ENTRY_SIZE, position + SHIFT); + final int at = i * MpqHeader.BLOCK_ENTRY_SIZE; + rows.putInt(at, rows.getInt(at) + shift); } new MPQEncryption(tableKey("(block table)"), false) .processSingle(ByteBuffer.wrap(table)); - System.arraycopy(table, 0, image, at, length); } - private static void recordDigests(byte[] image, int hashAt, int hashEntries, - int blockAt, int blockEntries, - int extendedAt, int extendedLength) { - put(image, MD5_HASH_TABLE, md5(image, hashAt, hashEntries * MpqHeader.HASH_ENTRY_SIZE)); - put(image, MD5_BLOCK_TABLE, md5(image, blockAt, blockEntries * MpqHeader.BLOCK_ENTRY_SIZE)); - put(image, MD5_HI_BLOCK_TABLE, new byte[DIGEST_SIZE]); - if (extendedAt >= 0) { - put(image, MD5_HET_TABLE, md5(image, extendedAt, extendedLength)); - put(image, MD5_BET_TABLE, md5(image, extendedAt, extendedLength)); - } else { - put(image, MD5_HET_TABLE, new byte[DIGEST_SIZE]); - put(image, MD5_BET_TABLE, new byte[DIGEST_SIZE]); - } - // The header digest covers the header up to but not including itself. - put(image, MD5_HEADER, md5(image, 0, MD5_HEADER)); + private static byte[] slice(byte[] source, int at, int length) { + final byte[] out = new byte[length]; + System.arraycopy(source, at, out, 0, length); + return out; } private static void put(byte[] image, int at, byte[] digest) { From e0049b56529cbc379d884d1fedb1de1c974e6346 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 10:59:35 +0200 Subject: [PATCH 05/13] Address review: screen out decoys this library cannot parse A decoy with in-range table positions but a format version above MAX_FORMAT_VERSION passed every part of the plausibility test, so the scan committed to it and then failed in parseAt as unsupported -- with a valid header possibly still ahead of it in the file. Same class as the previous finding: the screen was not rejecting headers that cannot be used. The version screen is skipped under forceV0, which exists precisely to read archives whose declared version is garbage. For the same reason the compressed hash-table size is only consulted for a candidate that will actually be read as version 3; under forceV0 those bytes mean nothing, and the check now says version 3 rather than version 3 or above. Both directions are covered, and I checked the decoy test fails when the screen is removed. Format notes gain section 14 on what the scan requires and why strengthening it needs care. --- docs/mpq-format-notes.md | 28 ++++++++++ src/main/java/org/inwc3/jmpq/MpqHeader.java | 32 ++++++++--- .../jmpq3test/Version3IntegrityTests.java | 55 +++++++++++++++++++ 3 files changed, 107 insertions(+), 8 deletions(-) diff --git a/docs/mpq-format-notes.md b/docs/mpq-format-notes.md index c1cecfa..7d4f268 100644 --- a/docs/mpq-format-notes.md +++ b/docs/mpq-format-notes.md @@ -358,6 +358,14 @@ A hi-block table whose position falls outside the file is dropped and the archiv flagged malformed, rather than refused: reading the low words alone is exactly what a version 0 reader does, and the archive is otherwise fine. +**This interacts with the header scan.** The plausibility check of §14 screens a +candidate on whether its hash table fits in the file, and it has to use the +*stored* length for the same reason: a valid version 3 archive whose compressed +hash table ends the file has no room for the uncompressed form, so screening on +that rejects the real header — and once rejected, a decoy planted earlier in the +file wins the scan. Strengthening the check without accounting for compression +turns it into the thing it was written to prevent. + ## 13. Version 3 MD5 digests are reported, not enforced @@ -381,3 +389,23 @@ tables. This library does not read those tables, but it does check their digests skipping them would let an archive whose HET table is the damaged one report clean. + +## 14. What makes a candidate header plausible + +Protected archives plant decoy `MPQ\x1A` signatures so a reader commits to the +first one it finds and then fails on tables that are not there. StormLib screens +candidates (`ERROR_FAKE_MPQ_HEADER`) rather than trusting the first hit. + +**Decision.** `MpqHeader.findHeader` applies a cheap test to every candidate — +archive headers and user-data redirects alike — and keeps scanning past one that +fails. The test requires a non-zero hash and block table position, a non-zero +hash entry count, a sector shift that does not overflow, and room in the file for +the hash table at its *stored* length (see §12). + +Two properties keep this safe to have strengthened: + +- The **first candidate is retained as a fallback**. If nothing in the file looks + plausible, that candidate is used anyway, so the scan can only ever find a + header where a naive scan found one — never fewer. +- The test only rejects on things that make an archive unreadable regardless, so + a candidate it rejects would have failed at table-parse time in any case. diff --git a/src/main/java/org/inwc3/jmpq/MpqHeader.java b/src/main/java/org/inwc3/jmpq/MpqHeader.java index 6ee51c0..3004d1d 100644 --- a/src/main/java/org/inwc3/jmpq/MpqHeader.java +++ b/src/main/java/org/inwc3/jmpq/MpqHeader.java @@ -403,7 +403,7 @@ private static Located findHeader(MpqSource source, boolean forceV0) throws JMpq if (signature == ARCHIVE_SIGNATURE) { final Located candidate = new Located(position, null); - if (isPlausible(source, position)) { + if (isPlausible(source, position, forceV0)) { return candidate; } if (fallback == null) { @@ -420,7 +420,7 @@ private static Located findHeader(MpqSource source, boolean forceV0) throws JMpq final long redirected = userData.archiveHeaderOffset(); if (source.contains(redirected, 4) && source.i32(redirected) == ARCHIVE_SIGNATURE) { final Located candidate = new Located(redirected, userData); - if (isPlausible(source, redirected)) { + if (isPlausible(source, redirected, forceV0)) { return candidate; } if (fallback == null) { @@ -441,10 +441,20 @@ private static Located findHeader(MpqSource source, boolean forceV0) throws JMpq * {@code ERROR_FAKE_MPQ_HEADER} checks: a header whose table positions fall * outside the file cannot be the real one. */ - private static boolean isPlausible(MpqSource source, long position) throws JMpqException { + private static boolean isPlausible(MpqSource source, long position, boolean forceV0) + throws JMpqException { if (!source.contains(position, SIZE_BY_VERSION[0])) { return false; } + // A version this library cannot parse cannot be the header it goes on to + // use, so accepting the candidate only ends the scan early and then + // fails in parseAt -- with a valid header possibly still ahead of it. + // Unless forceV0 is set, in which case the declared version is ignored + // on purpose and a garbage one is exactly what is expected. + if (!forceV0 && source.u16(position + 0x0C) > MAX_FORMAT_VERSION) { + return false; + } + final long hashTablePosition = source.u32(position + 0x10); final long blockTablePosition = source.u32(position + 0x14); final int hashTableEntries = source.i32(position + 0x18) & 0x0FFFFFFF; @@ -455,7 +465,7 @@ private static boolean isPlausible(MpqSource source, long position) throws JMpqE && hashTableEntries > 0 && sectorShift <= MAX_SECTOR_SIZE_SHIFT && source.contains(position + hashTablePosition, - candidateHashTableBytes(source, position, hashTableEntries)) + candidateHashTableBytes(source, position, hashTableEntries, forceV0)) && source.contains(position + blockTablePosition, 0); } @@ -468,15 +478,21 @@ private static boolean isPlausible(MpqSource source, long position) throws JMpqE * the scan would then settle on the decoy. That is the reverse of what this * check is for, so the stored length is used where the header declares one. * - * @param source the archive bytes. + * Only format version 3 has the field, and only if this candidate is going + * to be read as version 3 -- under {@code forceV0} it will be read as + * version 0, where those bytes mean nothing. + * + * @param source the archive bytes. * @param position the candidate header offset. * @param entries declared hash table entries. + * @param forceV0 whether the archive will be read as version 0 regardless. * @return bytes to require at the hash table position. */ - private static long candidateHashTableBytes(MpqSource source, long position, int entries) - throws JMpqException { + private static long candidateHashTableBytes(MpqSource source, long position, int entries, + boolean forceV0) throws JMpqException { final long plain = (long) entries * HASH_ENTRY_SIZE; - if (source.u16(position + 0x0C) < 3 || !source.contains(position, SIZE_BY_VERSION[3])) { + if (forceV0 || source.u16(position + 0x0C) != 3 + || !source.contains(position, SIZE_BY_VERSION[3])) { return plain; } final long stored = source.i64(position + 0x44); diff --git a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java index 0d4bf95..bc07309 100644 --- a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java @@ -174,6 +174,61 @@ public void aDecoyDoesNotBeatAValidCompressedTableHeader() throws IOException { } } + // --------------------------------------------------- unparseable decoys + + /** + * A decoy declaring a format version this library cannot parse must not end + * the scan. + *

+ * Its table positions are in range, so every other part of the plausibility + * test passes; accepting it stops the scan short of the real header and then + * fails in {@code parseAt} with "not supported", with the valid archive + * sitting untouched further down the file. + */ + @Test + public void aDecoyDeclaringAnUnsupportedVersionDoesNotEndTheScan() throws IOException { + final byte[] real = build(new Shape(false, false, false, false)); + final byte[] image = behindADecoy(real); + + // Give the decoy in-range tables and an impossible version. + final ByteBuffer decoy = ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN); + decoy.putShort(0x0C, (short) 9); + decoy.putInt(0x10, 0x100); + decoy.putInt(0x14, 0x180); + decoy.putInt(0x18, 4); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.header().headerOffset(), MpqHeader.ALIGNMENT, + "a version that cannot be parsed cannot be the header to use"); + Assert.assertEquals(archive.header().formatVersion(), 3); + Assert.assertEquals(archive.read("a.txt"), content()); + } + } + + /** + * The version screen must not apply under {@code forceV0}, which exists + * precisely to read archives whose declared version is garbage — the + * protected maps of issue #46. Warcraft III ignores the field, so a corrupt + * one must not stop the archive opening. + */ + @Test + public void forceV0StillAcceptsAGarbageVersion() throws IOException { + final byte[] image = source(); + ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN).putShort(0x0C, (short) 0x1234); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.warcraft3())) { + Assert.assertEquals(archive.header().formatVersion(), 0, "read as version 0"); + Assert.assertTrue(archive.header().malformed()); + Assert.assertEquals(archive.read("a.txt"), content()); + } + + // Without it, the version is taken at face value and reported. + final systems.crigges.jmpq3.JMpqException thrown = Assert.expectThrows( + systems.crigges.jmpq3.JMpqException.class, + () -> MpqArchive.open(image, MpqOpenOptions.defaults()).close()); + Assert.assertTrue(thrown.getMessage().contains("not supported"), thrown.getMessage()); + } + // ------------------------------------------------------------ fixtures /** From 21f4d7579003ad1fee9a67227835188929c43613 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 11:03:12 +0200 Subject: [PATCH 06/13] Screen the whole family of parser rejections, not one at a time Two review rounds went on this method one condition at a time, both times because the screen accepted a candidate parseAt would refuse. So the rest are done together: the hash table entry maximum and, for a version 3 candidate, a compressed block table that runs past the end. The invariant is now written down where it belongs. isPlausible must reject only what parseAt rejects -- rejecting more is not a stricter filter but a bug, because the scan then moves on and can settle on an earlier decoy. Everything the parser merely repairs (a wrong header size, an oversized block table, an out-of-range hi-block position) is deliberately not screened, and there is now a test asserting that too, so tightening this again cannot quietly start losing readable archives. One screen is documented as unable to change the outcome today: a candidate without room for its own header is necessarily the last one in the file, so no later header exists to reach. It is kept to make the screen a faithful mirror rather than a set of conditions that happen to matter, and the test says why it is not covered. --- src/main/java/org/inwc3/jmpq/MpqHeader.java | 80 +++++++++++++++---- .../jmpq3test/Version3IntegrityTests.java | 73 +++++++++++++++++ 2 files changed, 139 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqHeader.java b/src/main/java/org/inwc3/jmpq/MpqHeader.java index 3004d1d..b7a0de0 100644 --- a/src/main/java/org/inwc3/jmpq/MpqHeader.java +++ b/src/main/java/org/inwc3/jmpq/MpqHeader.java @@ -438,35 +438,87 @@ private static Located findHeader(MpqSource source, boolean forceV0) throws JMpq /** * Cheap plausibility test for a candidate header, mirroring StormLib's - * {@code ERROR_FAKE_MPQ_HEADER} checks: a header whose table positions fall - * outside the file cannot be the real one. + * {@code ERROR_FAKE_MPQ_HEADER} checks. + * + *

The invariant

+ * This must reject only headers that {@link #parseAt} would reject anyway. + * Rejecting anything more is not a stricter filter, it is a bug: the scan + * moves on and can settle on a decoy planted earlier in the file, which is + * the reverse of the point. Two rounds of review found exactly that, both + * times because a condition here was tightened past what the parser + * actually requires. + *

+ * So the checks below are a screen of the parser's own rejections, in the + * same order and with the same thresholds, and nothing else. Everything the + * parser merely repairs — a wrong header size, an oversized block + * table, an out-of-range hi-block position — is deliberately not screened + * here, because such a header is still perfectly usable. */ private static boolean isPlausible(MpqSource source, long position, boolean forceV0) throws JMpqException { if (!source.contains(position, SIZE_BY_VERSION[0])) { return false; } - // A version this library cannot parse cannot be the header it goes on to - // use, so accepting the candidate only ends the scan early and then - // fails in parseAt -- with a valid header possibly still ahead of it. - // Unless forceV0 is set, in which case the declared version is ignored - // on purpose and a garbage one is exactly what is expected. - if (!forceV0 && source.u16(position + 0x0C) > MAX_FORMAT_VERSION) { + + // The version the parser will settle on: forceV0 ignores what the header + // declares, which is the whole point of it. + final int version = forceV0 ? 0 : source.u16(position + 0x0C); + if (version > MAX_FORMAT_VERSION) { + return false; + } + // The parser reads the whole header for that version before anything + // else, and cannot repair its way out of the bytes not being there. + // This cannot currently change which header is chosen: a candidate + // without room for its own header is necessarily the last one in the + // file, so there is no later header to reach and the fallback returns + // this one anyway. It is here to keep the screen a faithful mirror of + // the parser rather than a set of conditions that happen to matter. + if (!source.contains(position, SIZE_BY_VERSION[version])) { return false; } - final long hashTablePosition = source.u32(position + 0x10); - final long blockTablePosition = source.u32(position + 0x14); - final int hashTableEntries = source.i32(position + 0x18) & 0x0FFFFFFF; final int sectorShift = source.u16(position + 0x0E) & 0xFF; + if (sectorShift > MAX_SECTOR_SIZE_SHIFT) { + return false; + } + + final int hashTableEntries = source.i32(position + 0x18) & 0x0FFFFFFF; + if (hashTableEntries <= 0 || hashTableEntries > MAX_HASH_TABLE_ENTRIES) { + return false; + } + final long hashTablePosition = source.u32(position + 0x10); + final long blockTablePosition = source.u32(position + 0x14); return hashTablePosition > 0 && blockTablePosition > 0 - && hashTableEntries > 0 - && sectorShift <= MAX_SECTOR_SIZE_SHIFT && source.contains(position + hashTablePosition, candidateHashTableBytes(source, position, hashTableEntries, forceV0)) - && source.contains(position + blockTablePosition, 0); + // A block table that runs past the end is clamped rather than + // refused, so only its position has to be in the file -- unless it + // is compressed, where the stored length is all there is to go on. + && source.contains(position + blockTablePosition, + candidateCompressedBlockTableBytes(source, position, forceV0)); + } + + /** + * Stored length of a candidate's block table, but only when compressing it + * makes that length load-bearing. + * + * @param source the archive bytes. + * @param position the candidate header offset. + * @param forceV0 whether the archive will be read as version 0 regardless. + * @return bytes to require at the block table position, or 0 to require only + * that the position itself is in the file. + */ + private static long candidateCompressedBlockTableBytes(MpqSource source, long position, + boolean forceV0) throws JMpqException { + if (forceV0 || source.u16(position + 0x0C) != 3 + || !source.contains(position, SIZE_BY_VERSION[3])) { + return 0; + } + final long plain = (long) source.i32(position + 0x1C) * BLOCK_ENTRY_SIZE; + final long stored = source.i64(position + 0x4C); + return stored > 0 && stored < plain ? stored : 0; } /** diff --git a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java index bc07309..573e0dc 100644 --- a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java @@ -229,6 +229,79 @@ public void forceV0StillAcceptsAGarbageVersion() throws IOException { Assert.assertTrue(thrown.getMessage().contains("not supported"), thrown.getMessage()); } + /** + * The remaining rejections the parser makes, screened for as a family. + *

+ * Two review rounds went on this one condition at a time, so the rest are + * done together: a candidate the parser would refuse must not end the scan, + * whichever of its refusals applies. Each decoy here is otherwise perfectly + * plausible and differs only in the field that dooms it. + */ + @Test + public void everyDecoyTheParserWouldRefuseIsSkipped() throws IOException { + // A hash table entry count above the maximum StormLib accepts. + assertScanSkipsDecoy(decoy -> decoy.putInt(0x18, 0x0010_0000), "oversized hash table"); + + // Not covered here: a version 3 header without 208 bytes behind it. Such + // a candidate is necessarily the last one in the file -- anything after + // it would be past the end -- so there is never a later valid header for + // the scan to reach, and the screen cannot change the outcome. See the + // note in isPlausible. + + // A sector size shift that would mean sectors beyond an int. + assertScanSkipsDecoy(decoy -> decoy.putShort(0x0E, (short) 30), "impossible sector size"); + + // A zero hash table position, which no real archive has. + assertScanSkipsDecoy(decoy -> decoy.putInt(0x10, 0), "no hash table"); + } + + /** + * Plants an otherwise-plausible decoy at offset 0, breaks the one field the + * caller names, and requires the scan to reach the real header at 0x200. + */ + private void assertScanSkipsDecoy(java.util.function.Consumer damage, String what) + throws IOException { + final byte[] image = behindADecoy(build(new Shape(false, false, false, false))); + + final ByteBuffer decoy = ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN); + // In-range tables, so only the damaged field can disqualify it. + decoy.putInt(0x04, 32); + decoy.putShort(0x0C, (short) 0); + decoy.putShort(0x0E, (short) 3); + decoy.putInt(0x10, 0x100); + decoy.putInt(0x14, 0x180); + decoy.putInt(0x18, 4); + decoy.putInt(0x1C, 1); + damage.accept(decoy); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.header().headerOffset(), MpqHeader.ALIGNMENT, + "the scan should have walked past the decoy with " + what); + Assert.assertEquals(archive.read("a.txt"), content(), what); + } + } + + /** + * The other half of the invariant: a header the parser only repairs + * must still be accepted, or tightening the screen would start losing + * archives that open perfectly well today. + */ + @Test + public void aHeaderTheParserWouldOnlyRepairIsStillPlausible() throws IOException { + final byte[] image = source(); + final ByteBuffer header = ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN); + + // A wrong header size and an absurd block table count are both repaired, + // so neither may disqualify the header. + header.putInt(0x04, 999); + header.putInt(0x1C, 0x0100_0000); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertTrue(archive.header().malformed()); + Assert.assertEquals(archive.read("a.txt"), content()); + } + } + // ------------------------------------------------------------ fixtures /** From 160ee8aa2bfb1821b142c1020f6c7b7bcbafdf33 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 11:12:17 +0200 Subject: [PATCH 07/13] Address review: clamped block count, and reject unknown attributes versions The block-table range check validated against a header model still holding the raw negative count, so its byte length came out negative, the check always failed, and the recovery path replaced the clamped zero with every 16-byte row between the table and EOF -- reading trailing bytes as live block entries. The half-built model is gone; the check now uses the clamped count, and the compressed-or-plain rule has one implementation instead of being spelled out in three places. An (attributes) file declaring a version other than 100 was parsed with the version 100 layout, so a same-length future or corrupt file produced plausible-looking checksums and timestamps describing nothing. 100 is the only version the format has had, so anything else is now reported. The archive still opens either way -- attributes are advisory. Both verified to fail with the fix backed out. Full suite: 195 tests. --- .../java/org/inwc3/jmpq/MpqAttributes.java | 9 ++++ src/main/java/org/inwc3/jmpq/MpqHeader.java | 46 +++++++++++++------ .../crigges/jmpq3test/MpqAttributesTests.java | 25 ++++++++++ .../crigges/jmpq3test/Phase2FormatTests.java | 36 +++++++++++++++ 4 files changed, 102 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqAttributes.java b/src/main/java/org/inwc3/jmpq/MpqAttributes.java index b6861a3..9024690 100644 --- a/src/main/java/org/inwc3/jmpq/MpqAttributes.java +++ b/src/main/java/org/inwc3/jmpq/MpqAttributes.java @@ -235,6 +235,15 @@ public static MpqAttributes parse(byte[] data, int blockCount) throws JMpqExcept final int version = in.getInt(); final int flags = in.getInt(); + if (version != VERSION) { + // 100 is the only version there has ever been, so a different one + // means the body is not laid out the way this code reads it. A + // same-length file would otherwise parse into plausible-looking + // checksums and timestamps that describe nothing. + throw new JMpqException("An attributes file declares version " + version + + "; only " + VERSION + " is known."); + } + // An unknown bit means an array of unknown length, so nothing after the // arrays we do understand can be located. Reading the known prefix and // ignoring the rest is what StormLib does. diff --git a/src/main/java/org/inwc3/jmpq/MpqHeader.java b/src/main/java/org/inwc3/jmpq/MpqHeader.java index b7a0de0..a6d6000 100644 --- a/src/main/java/org/inwc3/jmpq/MpqHeader.java +++ b/src/main/java/org/inwc3/jmpq/MpqHeader.java @@ -284,8 +284,19 @@ public long betTableFileOffset() { * @return bytes the hash table occupies in the file. */ public long hashTableStoredSize() { - final long plain = (long) hashTableEntries * HASH_ENTRY_SIZE; - final long declared = extended.hashTableCompressedSize(); + return storedSize(extended.hashTableCompressedSize(), + (long) hashTableEntries * HASH_ENTRY_SIZE); + } + + /** + * Resolves a table's stored length. + * + * @param declared the compressed size the header records, or 0. + * @param plain the length the entry count implies. + * @return {@code declared} when it names a shorter, compressed table, + * otherwise {@code plain}. + */ + private static long storedSize(long declared, long plain) { return declared > 0 && declared < plain ? declared : plain; } @@ -293,9 +304,8 @@ public long hashTableStoredSize() { * @return bytes the block table occupies in the file. */ public long blockTableStoredSize() { - final long plain = (long) blockTableEntries * BLOCK_ENTRY_SIZE; - final long declared = extended.blockTableCompressedSize(); - return declared > 0 && declared < plain ? declared : plain; + return storedSize(extended.blockTableCompressedSize(), + (long) blockTableEntries * BLOCK_ENTRY_SIZE); } /** @@ -662,12 +672,9 @@ private static MpqHeader parseAt(MpqSource source, Located located, boolean forc malformed = true; } - final MpqHeader header = new MpqHeader(offset, headerSize, formatVersion, archiveSize, - sectorSizeShift, hashTablePosition, blockTablePosition, hashTableEntries, - blockTableEntries, hiBlockTablePosition, hetTablePosition, betTablePosition, - extended, located.userData(), malformed); - - if (!source.contains(offset + hashTablePosition, header.hashTableStoredSize())) { + if (!source.contains(offset + hashTablePosition, + storedSize(extended.hashTableCompressedSize(), + (long) hashTableEntries * HASH_ENTRY_SIZE))) { throw new JMpqException("Hash table at " + (offset + hashTablePosition) + " spanning " + hashTableEntries + " entries runs past the end of " + source.origin() + "."); } @@ -680,15 +687,26 @@ private static MpqHeader parseAt(MpqSource source, Located located, boolean forc blockTableEntries = 0; malformed = true; } - if (!source.contains(offset + blockTablePosition, header.blockTableStoredSize())) { + + // Worked out from the clamped count, not from a half-built header. An + // earlier version validated against a model still holding the raw + // negative count, so its stored size came out negative, the range check + // always failed, and the recovery below then replaced the clamped zero + // with every 16-byte row between the table and the end of the file -- + // turning trailing bytes into live block entries. + final long blockTablePlain = (long) blockTableEntries * BLOCK_ENTRY_SIZE; + final long blockTableStored = + storedSize(extended.blockTableCompressedSize(), blockTablePlain); + + if (!source.contains(offset + blockTablePosition, blockTableStored)) { // StormLib does exactly this: archives in the wild declare a block // table far larger than the file, and rejecting them would be // stricter than the game. A compressed table cannot be reinterpreted // this way, because its entry count is not implied by its length. - if (header.isBlockTableCompressed()) { + if (blockTableStored < blockTablePlain) { throw new JMpqException("Compressed block table at " + (offset + blockTablePosition) + " spanning " - + header.blockTableStoredSize() + " bytes runs past the end of " + + blockTableStored + " bytes runs past the end of " + source.origin() + "."); } final long fits = (source.size() - offset - blockTablePosition) / BLOCK_ENTRY_SIZE; diff --git a/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java b/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java index d9a94b4..8f1441c 100644 --- a/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java +++ b/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java @@ -268,6 +268,31 @@ private static byte[] patchBitFile(int entries, int patchBytes) { return out.array(); } + /** + * A version other than 100 means the body is not laid out the way this code + * reads it. + *

+ * 100 is the only version the format has ever had, so a different one is + * either a future format or corruption. Parsing it anyway turns a + * same-length file into plausible-looking checksums and timestamps that + * describe nothing, which is worse than reporting it unreadable — the + * archive still opens either way, since attributes are advisory. + */ + @Test + public void anUnknownVersionIsRejectedRatherThanReinterpreted() { + final byte[] file = MpqAttributes.build(new int[]{1, 2}, new long[]{3, 4}); + file[0] = (byte) 200; + + final JMpqException thrown = Assert.expectThrows(JMpqException.class, + () -> MpqAttributes.parse(file, 2)); + Assert.assertTrue(thrown.getMessage().contains("version 200"), thrown.getMessage()); + + // Version 0, which is what a zeroed or truncated file looks like. + final byte[] zeroed = MpqAttributes.build(new int[]{1, 2}, new long[]{3, 4}); + zeroed[0] = 0; + Assert.expectThrows(JMpqException.class, () -> MpqAttributes.parse(zeroed, 2)); + } + /** * A bytemask naming nothing this implementation knows describes no entries. * Worth its own test: the length-driven count only terminates because of diff --git a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java index dcbbb16..ec57cec 100644 --- a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java @@ -451,6 +451,42 @@ private static int headerOffset(byte[] image) { throw new AssertionError("no header in the test fixture"); } + /** + * A negative declared block count means no block table, not "every 16 bytes + * to the end of the file". + *

+ * The count is clamped to zero, and the range check that decides whether to + * reinterpret an oversized table has to use the clamped value. Validating + * against the raw negative count made its byte length negative, so the check + * failed, and the recovery path then replaced the clamped zero with every row + * that fitted between the table and EOF — reading trailing bytes as live + * block entries. + */ + @Test + public void aNegativeBlockCountDoesNotBecomeEverythingToTheEndOfTheFile() throws IOException { + final byte[] archive = MpqArchiveWriter.create(MpqWriteOptions.defaults().withPrefix(false)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + // Trailing bytes after the block table, so "everything that fits" and + // "nothing" are observably different answers. + final byte[] image = new byte[archive.length + 64]; + System.arraycopy(archive, 0, image, 0, archive.length); + for (int i = archive.length; i < image.length; i++) { + image[i] = (byte) 0xCD; + } + + final int headerAt = headerOffset(image); + ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN).putInt(headerAt + 0x1C, -1); + + try (MpqArchive open = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(open.header().blockTableEntries(), 0, + "a negative count describes no blocks at all"); + Assert.assertTrue(open.header().malformed()); + Assert.assertEquals(open.blockCount(), 0); + } + } + // ------------------------------------------------- P2-1 user data header /** From 678d99f1d0b51dcd20a267e50110a40913cc4a5c Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 11:19:46 +0200 Subject: [PATCH 08/13] Build candidate table offsets the way the parser builds them The plausibility screen read low-only table offsets while the parser combines them with the high words at 0x28/0x2A from version 1 onward. So a decoy with in-range low offsets and a non-zero high word passed the screen and then failed to parse, with a valid header still ahead of it in the file. This is the same mismatch the previous two rounds were about, and the screen's own doc comment claimed to mirror the parser exactly -- it did for the thresholds and not for the offsets. Asserting an invariant in a comment did not keep it true, so the offsets now come from one tablePosition helper that both sides call, which is the same treatment storedSize got. The version-3 size helpers take the resolved version instead of re-deriving it, removing the last two places that decided independently what version a candidate is. Covered both ways round: a high word on either table offset, verified to fail with the low-only reads restored. --- src/main/java/org/inwc3/jmpq/MpqHeader.java | 74 +++++++++++++------ .../jmpq3test/Version3IntegrityTests.java | 15 ++++ 2 files changed, 65 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqHeader.java b/src/main/java/org/inwc3/jmpq/MpqHeader.java index a6d6000..213f6ce 100644 --- a/src/main/java/org/inwc3/jmpq/MpqHeader.java +++ b/src/main/java/org/inwc3/jmpq/MpqHeader.java @@ -446,6 +446,35 @@ private static Located findHeader(MpqSource source, boolean forceV0) throws JMpq throw new JMpqException("No MPQ archive header in " + source.origin() + "."); } + /** + * A table position as the format lays it out. + *

+ * From version 1 the offset is 48 bits: a low word in the version 0 field + * and a high word further into the header. Both the plausibility screen and + * the parser go through here, because when they each built the offset + * themselves they drifted — the screen kept using low-only positions after + * the parser had learned about the high words, so a decoy with in-range low + * offsets and non-zero high words passed the screen and then failed to + * parse, with a valid header still ahead of it. + * + * @param source the archive bytes. + * @param headerAt the header offset. + * @param version the format version the header will be read as, so 0 + * under {@code forceV0}, where the high words are not + * part of the header at all. + * @param lowOffset offset of the 32-bit low field. + * @param highOffset offset of the 16-bit high field. + * @return the table offset relative to the header. + */ + private static long tablePosition(MpqSource source, long headerAt, int version, + int lowOffset, int highOffset) throws JMpqException { + long position = source.u32(headerAt + lowOffset); + if (version >= 1) { + position |= (long) source.u16(headerAt + highOffset) << 32; + } + return position; + } + /** * Cheap plausibility test for a candidate header, mirroring StormLib's * {@code ERROR_FAKE_MPQ_HEADER} checks. @@ -497,17 +526,18 @@ private static boolean isPlausible(MpqSource source, long position, boolean forc return false; } - final long hashTablePosition = source.u32(position + 0x10); - final long blockTablePosition = source.u32(position + 0x14); + // Built exactly as the parser builds them, high words included. + final long hashTablePosition = tablePosition(source, position, version, 0x10, 0x28); + final long blockTablePosition = tablePosition(source, position, version, 0x14, 0x2A); return hashTablePosition > 0 && blockTablePosition > 0 && source.contains(position + hashTablePosition, - candidateHashTableBytes(source, position, hashTableEntries, forceV0)) + candidateHashTableBytes(source, position, hashTableEntries, version)) // A block table that runs past the end is clamped rather than // refused, so only its position has to be in the file -- unless it // is compressed, where the stored length is all there is to go on. && source.contains(position + blockTablePosition, - candidateCompressedBlockTableBytes(source, position, forceV0)); + candidateCompressedBlockTableBytes(source, position, version)); } /** @@ -516,19 +546,18 @@ private static boolean isPlausible(MpqSource source, long position, boolean forc * * @param source the archive bytes. * @param position the candidate header offset. - * @param forceV0 whether the archive will be read as version 0 regardless. + * @param version the version the header will be read as. * @return bytes to require at the block table position, or 0 to require only * that the position itself is in the file. */ private static long candidateCompressedBlockTableBytes(MpqSource source, long position, - boolean forceV0) throws JMpqException { - if (forceV0 || source.u16(position + 0x0C) != 3 - || !source.contains(position, SIZE_BY_VERSION[3])) { + int version) throws JMpqException { + if (version != 3) { return 0; } final long plain = (long) source.i32(position + 0x1C) * BLOCK_ENTRY_SIZE; - final long stored = source.i64(position + 0x4C); - return stored > 0 && stored < plain ? stored : 0; + final long stored = storedSize(source.i64(position + 0x4C), plain); + return stored < plain ? stored : 0; } /** @@ -540,25 +569,23 @@ private static long candidateCompressedBlockTableBytes(MpqSource source, long po * the scan would then settle on the decoy. That is the reverse of what this * check is for, so the stored length is used where the header declares one. * - * Only format version 3 has the field, and only if this candidate is going - * to be read as version 3 -- under {@code forceV0} it will be read as - * version 0, where those bytes mean nothing. + * Only format version 3 has the field, which is why the resolved version is + * passed in rather than read again here: under {@code forceV0} it is 0, and + * those bytes mean nothing. * * @param source the archive bytes. * @param position the candidate header offset. * @param entries declared hash table entries. - * @param forceV0 whether the archive will be read as version 0 regardless. + * @param version the version the header will be read as. * @return bytes to require at the hash table position. */ private static long candidateHashTableBytes(MpqSource source, long position, int entries, - boolean forceV0) throws JMpqException { + int version) throws JMpqException { final long plain = (long) entries * HASH_ENTRY_SIZE; - if (forceV0 || source.u16(position + 0x0C) != 3 - || !source.contains(position, SIZE_BY_VERSION[3])) { + if (version != 3) { return plain; } - final long stored = source.i64(position + 0x44); - return stored > 0 && stored < plain ? stored : plain; + return storedSize(source.i64(position + 0x44), plain); } /** @@ -610,8 +637,8 @@ private static MpqHeader parseAt(MpqSource source, Located located, boolean forc + " would mean sectors of " + (512L << sectorSizeShift) + " bytes."); } - long hashTablePosition = source.u32(offset + 0x10); - long blockTablePosition = source.u32(offset + 0x14); + final long hashTablePosition = tablePosition(source, offset, formatVersion, 0x10, 0x28); + final long blockTablePosition = tablePosition(source, offset, formatVersion, 0x14, 0x2A); final int hashTableEntries = source.i32(offset + 0x18) & 0x0FFFFFFF; int blockTableEntries = source.i32(offset + 0x1C); @@ -622,10 +649,9 @@ private static MpqHeader parseAt(MpqSource source, Located located, boolean forc Extended extended = Extended.NONE; if (formatVersion >= 1) { + // The table offsets already carry their high words, from + // tablePosition above. hiBlockTablePosition = source.i64(offset + 0x20); - // The high words extend the table offsets beyond 4 GiB. - hashTablePosition |= (long) source.u16(offset + 0x28) << 32; - blockTablePosition |= (long) source.u16(offset + 0x2A) << 32; } if (formatVersion >= 2) { archiveSize = source.i64(offset + 0x2C); diff --git a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java index 573e0dc..554eb7e 100644 --- a/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Version3IntegrityTests.java @@ -253,6 +253,21 @@ public void everyDecoyTheParserWouldRefuseIsSkipped() throws IOException { // A zero hash table position, which no real archive has. assertScanSkipsDecoy(decoy -> decoy.putInt(0x10, 0), "no hash table"); + + // In-range low table offsets, but a high word putting the real position + // 4 GiB out. The parser builds 48-bit offsets from version 1 onward, so + // the screen has to as well or it accepts what the parser refuses. + assertScanSkipsDecoy(decoy -> { + decoy.putShort(0x0C, (short) 1); + decoy.putInt(0x04, 44); + decoy.putShort(0x28, (short) 1); + }, "high word on the hash table offset"); + + assertScanSkipsDecoy(decoy -> { + decoy.putShort(0x0C, (short) 1); + decoy.putInt(0x04, 44); + decoy.putShort(0x2A, (short) 1); + }, "high word on the block table offset"); } /** From 0155098a5d8bb616176474fb0f55c4a9bee3f6c8 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 11:32:18 +0200 Subject: [PATCH 09/13] Match internal file names case-insensitively MPQ names are case-insensitive, but the writer filtered its carry-over list by exact string. A source whose listfile spells its attributes file (ATTRIBUTES) therefore had the stale file carried into pending, where the collision check -- which does canonicalise -- then refused to build at all. So rebuilding such an archive with attributes enabled failed outright. Three sites decided internal-name identity three different ways: exact match here, equalsIgnoreCase for the generated list file, and exact match again when counting what a rebuild drops. All three now go through MpqNames.canonical, the same fold the hash table and the writer's own keys use, so they cannot disagree. This is the same mistake as comparing paths without folding their separators, which cost a file on rebuild in Phase 0. --- src/main/java/org/inwc3/jmpq/MpqArchive.java | 7 ++- .../java/org/inwc3/jmpq/MpqArchiveWriter.java | 36 +++++++++--- .../crigges/jmpq3test/Phase2FormatTests.java | 57 +++++++++++++++++++ 3 files changed, 89 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index b1fd916..6bb3377 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -22,6 +22,7 @@ import java.util.Map; import java.util.Optional; import java.util.SequencedMap; +import java.util.Set; /** * Read-only access to an MPQ archive. @@ -65,7 +66,8 @@ public final class MpqArchive implements AutoCloseable { * regenerated, so it is not lost; these two are simply dropped, which * {@link #filesLostOnRebuild()} has to account for. */ - private static final List LOST_ON_REBUILD = List.of("(attributes)", "(signature)"); + private static final Set LOST_ON_REBUILD = Set.of( + MpqNames.canonical("(attributes)"), MpqNames.canonical("(signature)")); private static int tableKey(String name) { final MPQHashGenerator hasher = MPQHashGenerator.getFileKeyGenerator(); @@ -734,7 +736,8 @@ public Enumeration enumerationState() { public int filesLostOnRebuild() { int lost = 0; for (MpqFileEntry entry : entries()) { - if (entry.name().isEmpty() || LOST_ON_REBUILD.contains(entry.name())) { + if (entry.name().isEmpty() + || LOST_ON_REBUILD.contains(MpqNames.canonical(entry.name()))) { lost++; } } diff --git a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java index 6d96ae1..ce44fcf 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java @@ -20,6 +20,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.SequencedMap; +import java.util.Set; /** * Builds an MPQ archive and writes it somewhere, when told to. @@ -63,15 +64,34 @@ public final class MpqArchiveWriter { * never generated, so a caller holding those bytes may write them as an * ordinary file. */ - private static final List GENERATED = List.of("(listfile)"); + private static final Set GENERATED = canonicalNames("(listfile)"); /** * Internal files carried over from a source archive is not attempted: * {@code (listfile)} is regenerated, and the other two cannot be * regenerated so a copy would be stale. */ - private static final List NOT_CARRIED_OVER = - List.of("(listfile)", "(attributes)", "(signature)"); + private static final Set NOT_CARRIED_OVER = + canonicalNames("(listfile)", "(attributes)", "(signature)"); + + /** + * Folds a set of internal names for matching. + *

+ * MPQ names are case-insensitive, so a set of them has to be compared the + * way the archive itself compares them — through {@link MpqNames#canonical}, + * the same fold the hash table and this writer's own keys use. Matching + * internal names by exact string let a source spelling one {@code + * (ATTRIBUTES)} slip past the carry-over filter and then collide with the + * generated file, and it is the same mistake as comparing paths without + * folding their separators. + */ + private static Set canonicalNames(String... names) { + final Set canonical = new java.util.HashSet<>(); + for (String name : names) { + canonical.add(MpqNames.canonical(name)); + } + return Set.copyOf(canonical); + } /** * Flags the writer gives the internal files it generates, matching what @@ -167,7 +187,7 @@ public static MpqArchiveWriter from(MpqArchive source, MpqWriteOptions options) writer.prefix = source.prefixBytes(); } for (String name : source.names()) { - if (NOT_CARRIED_OVER.contains(name)) { + if (NOT_CARRIED_OVER.contains(MpqNames.canonical(name))) { continue; } // Every locale variant, not just the one a lookup resolves. @@ -299,11 +319,9 @@ private void requireUsableName(String name) { if (name == null || name.isEmpty()) { throw new IllegalArgumentException("A file name is required."); } - for (String generated : GENERATED) { - if (generated.equalsIgnoreCase(name)) { - throw new IllegalArgumentException(name + " is generated by the writer and cannot" - + " be supplied. Use MpqWriteOptions.withListfile to control it."); - } + if (GENERATED.contains(MpqNames.canonical(name))) { + throw new IllegalArgumentException(name + " is generated by the writer and cannot" + + " be supplied. Use MpqWriteOptions.withListfile to control it."); } } diff --git a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java index ec57cec..179ca45 100644 --- a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java @@ -355,6 +355,63 @@ public void unparseableAttributesDoNotStopTheArchiveOpening() throws IOException } } + /** + * Internal names are matched case-insensitively, like every other MPQ name. + *

+ * A source spelling its attributes file {@code (ATTRIBUTES)} used to slip + * past the carry-over filter, which compared by exact string, and then + * collide with the generated one, which compares canonically — so rebuilding + * such an archive with attributes enabled failed outright. This is the same + * mistake as comparing paths without folding them, which cost a file on + * rebuild in Phase 0. + */ + @Test + public void internalNamesAreCarriedOverCaseInsensitively() throws IOException { + final byte[] stale = MpqAttributes.build(new int[]{9, 9}, new long[]{9, 9}); + final byte[] original = MpqArchiveWriter.create(MpqWriteOptions.defaults()) + .put("(ATTRIBUTES)", stale) + .put("a.txt", "kept".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + try (MpqArchive source = MpqArchive.open(original, MpqOpenOptions.defaults())) { + Assert.assertTrue(source.names().contains("(ATTRIBUTES)"), + "the fixture must spell it in capitals"); + // It is an attributes file whatever its case, so a rebuild drops it. + Assert.assertEquals(source.filesLostOnRebuild(), 1); + + final byte[] rebuilt = MpqArchiveWriter + .from(source, MpqWriteOptions.defaults() + .withAttributes(true) + .withAttributesTimestamp(0)) + .toByteArray(); + + try (MpqArchive archive = MpqArchive.open(rebuilt, MpqOpenOptions.defaults())) { + // Fresh attributes, generated -- not the stale ones carried over. + final MpqAttributes attributes = archive.attributes().orElseThrow(); + Assert.assertEquals(attributes.entries(), archive.header().blockTableEntries()); + final int block = archive.entry("a.txt").orElseThrow().blockIndex(); + Assert.assertEquals(attributes.crc32Of(block), + crc32("kept".getBytes(StandardCharsets.UTF_8))); + Assert.assertEquals(archive.read("a.txt"), + "kept".getBytes(StandardCharsets.UTF_8)); + // Exactly one entry under that name, whatever its spelling. + Assert.assertEquals(archive.names().stream() + .filter(name -> name.equalsIgnoreCase(MpqAttributes.NAME)).count(), 0, + "generated internals are not listed"); + } + } + } + + /** The generated list file cannot be supplied under any spelling. */ + @Test + public void theGeneratedListfileIsReservedUnderEverySpelling() { + for (String spelling : new String[]{"(listfile)", "(LISTFILE)", "(ListFile)"}) { + Assert.expectThrows(IllegalArgumentException.class, + () -> MpqArchiveWriter.create(MpqWriteOptions.defaults()) + .put(spelling, new byte[1])); + } + } + // ---------------------------------------------------- P2-2 hi-block table /** From c8325be11db5969afe62de5e3b0c47038be579bf Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 11:42:05 +0200 Subject: [PATCH 10/13] An unreadable checksum chunk fails the read rather than being skipped Bounds that cannot locate the checksum chunk -- negative, reversed, or past the stored bytes -- were treated the same as an empty chunk, so default verification returned the file without checking it. The data sectors are delimited by the same offset table, so if its last entries are nonsense the sector entries are only accidentally still in range; handing the bytes back is the one outcome verification must not produce. Same principle as the digest round: unable to check is not the same as checked. An empty chunk stays legitimate, since a file may carry the flag and record nothing. The sibling case is fixed too: a chunk too short to hold one checksum per sector is corrupt rather than absent, and was also being swallowed. Recovering a damaged archive is still possible through withSectorChecksumVerification(false), which is what makes failing the default read the right default. --- .../java/org/inwc3/jmpq/MpqFileReader.java | 28 +++++-- .../crigges/jmpq3test/Phase2FormatTests.java | 79 +++++++++++++++++++ 2 files changed, 102 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqFileReader.java b/src/main/java/org/inwc3/jmpq/MpqFileReader.java index 1e25031..fc46b5c 100644 --- a/src/main/java/org/inwc3/jmpq/MpqFileReader.java +++ b/src/main/java/org/inwc3/jmpq/MpqFileReader.java @@ -178,7 +178,10 @@ private void readSectors(MpqFileEntry entry, OutputStream target, long base, int * compressed when that makes it smaller. * * @return one checksum per data sector, or an empty array when the file - * carries none or verification is off. + * records none or verification is off. + * @throws JMpqException if the chunk is present but its bounds or length are + * structurally impossible. Absent is fine; unreadable is not, since + * the caller asked for these bytes to be checked. */ private int[] readSectorChecksums(MpqFileEntry entry, int[] offsets, long base) throws IOException { @@ -190,9 +193,20 @@ private int[] readSectorChecksums(MpqFileEntry entry, int[] offsets, long base) final int start = offsets[sectors]; final int end = offsets[sectors + 1]; final int plainSize = sectors * 4; - if (start < 0 || end < start || end > entry.compressedSize() || end == start) { - // A file can carry the flag and no checksums; StormLib treats that - // as "nothing to check" rather than as damage. + + if (start < 0 || end < start || end > entry.compressedSize()) { + // Not "no checksums" -- a checksum chunk the offset table cannot + // locate means the table is damaged, and the data sectors it also + // delimits are only accidentally still in range. Skipping quietly + // would hand back a file that was asked to be verified and was not, + // which is the one thing verification must never do. + throw new JMpqException("The checksum chunk of <" + entry.name() + "> spans [" + + start + ", " + end + "), outside its " + entry.compressedSize() + + " stored bytes; the sector offset table is damaged."); + } + if (end == start) { + // An empty chunk is the legitimate case: a file may carry the flag + // and record nothing, which StormLib treats as nothing to check. return new int[0]; } @@ -202,7 +216,11 @@ private int[] readSectorChecksums(MpqFileEntry entry, int[] offsets, long base) header.formatVersion()); } if (chunk.length < plainSize) { - return new int[0]; + // Same reasoning as above: a chunk too short to hold one checksum + // per sector is corrupt, not absent. + throw new JMpqException("The checksum chunk of <" + entry.name() + "> holds " + + chunk.length + " bytes but the file has " + sectors + " sectors, needing " + + plainSize + "."); } final ByteBuffer in = ByteBuffer.wrap(chunk).order(java.nio.ByteOrder.LITTLE_ENDIAN); diff --git a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java index 179ca45..d8ee7c7 100644 --- a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java @@ -189,6 +189,85 @@ public void checksumsAreAddedToCarriedOverFilesToo() throws IOException { } } + /** + * A checksum chunk the offset table cannot locate is damage, not absence. + *

+ * The distinction matters because the data sectors are delimited by the same + * table: if its last entries are nonsense, the sector entries are only + * accidentally still in range. Treating that as "no checksums recorded" + * hands back a file that was asked to be verified and was not — the one + * outcome verification must never produce. An empty chunk is different, and + * stays legitimate. + */ + @Test + public void anUnreadableChecksumChunkFailsTheReadRatherThanSkippingIt() throws IOException { + final byte[] content = incompressible(9_000, 11); + + // Last offset entry pushed past the stored bytes. + final byte[] beyond = checksummedArchiveWithLastOffset(content, 1 << 20); + try (MpqArchive archive = MpqArchive.open(beyond, MpqOpenOptions.defaults())) { + final JMpqException thrown = Assert.expectThrows(JMpqException.class, + () -> archive.read("data.bin")); + Assert.assertTrue(thrown.getMessage().contains("checksum chunk"), thrown.getMessage()); + } + + // Last offset entry before the chunk starts. + final byte[] backwards = checksummedArchiveWithLastOffset(content, 0); + try (MpqArchive archive = MpqArchive.open(backwards, MpqOpenOptions.defaults())) { + Assert.expectThrows(JMpqException.class, () -> archive.read("data.bin")); + } + + // Turning verification off still recovers the data, which is the escape + // hatch that makes failing the default read acceptable. + try (MpqArchive archive = MpqArchive.open(beyond, + MpqOpenOptions.defaults().withSectorChecksumVerification(false))) { + Assert.assertEquals(archive.read("data.bin"), content); + } + } + + /** An empty checksum chunk means nothing was recorded, and reads fine. */ + @Test + public void anEmptyChecksumChunkIsTreatedAsNoneRecorded() throws IOException { + final byte[] content = incompressible(9_000, 12); + final byte[] image = checksummedArchiveWithLastOffset(content, -1); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.read("data.bin"), content, + "no checksums to check is not a failure"); + } + } + + /** + * Builds a checksummed archive and rewrites the final sector offset entry, + * which delimits the checksum chunk. The offset table of an unencrypted file + * is stored plainly, so it can be edited directly. + * + * @param lastOffset the value to write, or -1 to make the chunk empty by + * copying the entry before it. + */ + private static byte[] checksummedArchiveWithLastOffset(byte[] content, int lastOffset) + throws IOException { + final byte[] image = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withSectorChecksums(true)) + .put("data.bin", content) + .toByteArray(); + + final int base; + final int sectors; + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + final MpqFileEntry entry = archive.entry("data.bin").orElseThrow(); + Assert.assertFalse(entry.isEncrypted(), "the offset table must be readable as is"); + base = (int) (archive.header().headerOffset() + entry.filePosition()); + sectors = (entry.normalSize() + archive.header().sectorSize() - 1) + / archive.header().sectorSize(); + } + + final ByteBuffer table = ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN); + final int lastAt = base + (sectors + 1) * 4; + table.putInt(lastAt, lastOffset >= 0 ? lastOffset : table.getInt(base + sectors * 4)); + return image; + } + /** Where a file's first sector payload begins, past its offset table. */ private static int payloadStart(byte[] image, String name) throws IOException { try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { From d6ea908a634eacc4046d0145e69215f06ad2ad7d Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 11:49:11 +0200 Subject: [PATCH 11/13] Fix a signed overflow in the sector checksum, via the JDK intrinsic The hand-written Adler-32 loop used int accumulators with zlib's NMAX fold interval. zlib picks that interval so s2 cannot overflow an *unsigned* 32-bit accumulator; Java has no such type and a signed int overflows at half that, so a sector of a few thousand high-valued bytes was checksummed wrongly. Reachable in practice: the default recompression setting never shrinks a sector, so raw bytes reach the checksum as they are. Now computed with java.util.zip.Adler32 and corrected for the seed, which differs by a closed form -- 1 in the low half, one per byte in the high half. That is also the faster path, since Adler32.update is a HotSpot intrinsic and a Java loop is not, and sectors can be 16 MiB. The bug survived a round trip, a green suite and a review for the same reason the seed itself did: reader and writer shared the wrong arithmetic and agreed with each other. It took an independent implementation plus a fixture with high-valued stored sectors, which is now exported for CI. The old loop survives as the oracle the fast path is fuzzed against, with long accumulators so it is actually correct. Also aligns the hi-block table with StormLib: an archive declaring one it cannot hold is reported rather than read with the low words alone, which silently relocated every file. FORCE_V0 still opens such an archive, since a version 0 header has no such field. --- docs/mpq-format-notes.md | 28 +++++- src/main/java/org/inwc3/jmpq/MpqArchive.java | 17 +++- .../java/org/inwc3/jmpq/MpqChecksums.java | 64 ++++++++++-- src/main/java/org/inwc3/jmpq/MpqHeader.java | 7 +- .../crigges/jmpq3test/MpqChecksumTests.java | 99 +++++++++++++++++++ .../crigges/jmpq3test/Phase2FormatTests.java | 60 +++++++++-- 6 files changed, 248 insertions(+), 27 deletions(-) create mode 100644 src/test/java/systems/crigges/jmpq3test/MpqChecksumTests.java diff --git a/docs/mpq-format-notes.md b/docs/mpq-format-notes.md index 7d4f268..3b14dae 100644 --- a/docs/mpq-format-notes.md +++ b/docs/mpq-format-notes.md @@ -243,9 +243,31 @@ with `0` starts them at `s1 = 0, s2 = 0`. The two results differ by 1 in the low half and by the byte count in the high half — for every input, without exception. -**Decision.** `MpqChecksums.adler32` implements the seeded-zero form. -`java.util.zip.Adler32` cannot be used: it offers no way to seed, so it always -computes the standard variant. +**Decision.** `MpqChecksums.adler32` produces the seeded-zero form *via* +`java.util.zip.Adler32`. The seeds differ by a closed form rather than anything +structural — running the recurrence from `s1 = 1` adds 1 to the low half and one +per byte to the high half — so the JDK's intrinsic does the work and the result +is corrected: + +``` +s1(seed 0) = s1(seed 1) - 1 (mod 65521) +s2(seed 0) = s2(seed 1) - n (mod 65521) +``` + +The first implementation was a hand-written loop instead, and it was **wrong**. +zlib chooses its `NMAX = 5552` fold interval so that `s2` cannot overflow an +*unsigned* 32-bit accumulator; Java has no such type, and a signed `int` +overflows at half that. A single sector of a few thousand high-valued bytes was +therefore checksummed incorrectly — reachable in practice, because the default +recompression setting never shrinks a sector, so raw bytes reach the checksum as +they are. + +That bug survived a round trip, an all-green suite and a code review, for the +same reason as the seed itself: the reader and the writer shared the wrong +arithmetic and agreed with each other. `tools/mpqref.py` caught it once a +fixture with high-valued stored sectors existed, which is now part of the +exported set. The hand-written loop survives as `adler32Reference`, used only as +the oracle the fast path is fuzzed against. This one is worth dwelling on, because no self-consistent test can catch it. A reader and a writer that both use the standard seed agree with each other on diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index 6bb3377..d6b0865 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -633,11 +633,18 @@ private int[] readHiBlockTable(int blockCount) throws IOException { } final long tableBytes = (long) blockCount * MpqHeader.HI_BLOCK_ENTRY_SIZE; if (!source.contains(header.hiBlockTableFileOffset(), tableBytes)) { - // The archive claims a hi-block table it does not hold. Reading the - // low words alone at least matches what a version 0 reader sees. - log.warn("{} declares a hi-block table at {} that does not fit; ignoring it.", - source.origin(), header.hiBlockTableFileOffset()); - return new int[0]; + // Reported, not worked around. An archive declaring a hi-block table + // is declaring that its file positions do not fit in 32 bits, so + // carrying on with the low words alone puts every file at the wrong + // offset -- reads that fail, or worse, succeed with wrong bytes. + // StormLib treats an unreadable hi-block table as fatal too + // (BuildFileTable_Classic sets dwErrCode and stops). A version 0 + // archive never reaches here, so MPQOpenOption.FORCE_V0 remains the + // way to read one whose header claims a table it does not have. + throw new JMpqException("Archive declares a hi-block table at " + + header.hiBlockTableFileOffset() + " spanning " + tableBytes + + " bytes, which is not inside " + source.origin() + + ". Its file positions cannot be resolved."); } final int[] highWords = new int[blockCount]; for (int i = 0; i < blockCount; i++) { diff --git a/src/main/java/org/inwc3/jmpq/MpqChecksums.java b/src/main/java/org/inwc3/jmpq/MpqChecksums.java index 5e8f98b..33367c8 100644 --- a/src/main/java/org/inwc3/jmpq/MpqChecksums.java +++ b/src/main/java/org/inwc3/jmpq/MpqChecksums.java @@ -1,22 +1,38 @@ package org.inwc3.jmpq; +import java.util.zip.Adler32; + /** * The Adler-32 variant MPQ sector checksums use. * - *

Why this is not {@link java.util.zip.Adler32}

+ *

Why this is not plain {@link Adler32}

* StormLib computes sector checksums as {@code adler32(0, buffer, length)} — * both when writing them ({@code SFileAddFile.cpp}) and when checking them * ({@code ReadMpqSectors} in {@code SFileReadFile.cpp}). Passing zlib a seed of * {@code 0} starts the accumulators at {@code s1 = 0, s2 = 0}, whereas a - * standard Adler-32 — and so {@code java.util.zip.Adler32}, which offers no way - * to seed it — starts at {@code s1 = 1}. The results differ by 1 in the low half - * and by the byte count in the high half, for every input. + * standard Adler-32 — and so {@link Adler32}, which offers no way to seed it — + * starts at {@code s1 = 1}. The results differ by 1 in the low half and by the + * byte count in the high half, for every input. *

* That is a difference no self-consistent test can see: a reader and a writer * that both use the standard seed agree with each other perfectly and disagree * with every archive StormLib ever wrote. It was caught by * {@code tools/mpqref.py}, which computes the value independently, and is the * reason that cross-check exists. + * + *

Getting it from the intrinsic anyway

+ * The two seeds differ by a closed form rather than by anything structural, so + * the JDK's implementation can still do the work. Running the recurrence from + * {@code s1 = 1} instead of {@code s1 = 0} adds exactly 1 to the low half, and + * adds 1 per byte to the high half: + * 
+ * s1(seed 1) = s1(seed 0) + 1
+ * s2(seed 1) = s2(seed 0) + n
+ *
+ * So the seeded-zero value is recovered by subtracting those, modulo 65521. + * {@link Adler32#update(byte[], int, int)} is a HotSpot intrinsic, which a + * hand-written loop in Java is not — worth having when a single sector can be + * 16 MiB and every sector of every file passes through here. */ final class MpqChecksums { @@ -24,8 +40,8 @@ final class MpqChecksums { private static final int BASE = 65521; /** - * Largest number of bytes that can be accumulated before {@code s2} could - * overflow a signed 32-bit int. zlib calls this {@code NMAX}. + * Largest number of bytes the reference implementation accumulates before + * reducing. zlib calls this {@code NMAX}. */ private static final int NMAX = 5552; @@ -48,8 +64,38 @@ static int adler32(byte[] data) { * @return the sector checksum MPQ records. */ static int adler32(byte[] data, int offset, int length) { - int s1 = 0; - int s2 = 0; + final Adler32 standard = new Adler32(); + standard.update(data, offset, length); + final int seededOne = (int) standard.getValue(); + + // Undo the seed: 1 from the low half, one per byte from the high half. + final int low = Math.floorMod((seededOne & 0xFFFF) - 1, BASE); + final int high = Math.floorMod(((seededOne >>> 16) & 0xFFFF) - length % BASE, BASE); + return (high << 16) | low; + } + + /** + * The definition, computed directly. + *

+ * Kept as the oracle {@code MpqChecksumTests} checks {@link #adler32} + * against, so the seed correction above cannot drift from what it claims to + * compute. Not used in production: it is the same arithmetic without the + * intrinsic. + * + * @param data bytes to checksum. + * @param offset first byte to include. + * @param length how many bytes to include. + * @return the sector checksum MPQ records. + */ + static int adler32Reference(byte[] data, int offset, int length) { + // long accumulators, deliberately. zlib picks NMAX so that s2 cannot + // overflow an *unsigned* 32-bit accumulator; Java has no such type, and + // a signed int overflows at half that. An earlier version of this method + // was the production path and used int, so it silently produced wrong + // checksums for a sector of a few thousand high-valued bytes -- reachable + // as soon as an archive uses a sector size above the 4 KiB default. + long s1 = 0; + long s2 = 0; int at = offset; int remaining = length; @@ -63,6 +109,6 @@ static int adler32(byte[] data, int offset, int length) { s2 %= BASE; remaining -= block; } - return (s2 << 16) | s1; + return (int) ((s2 << 16) | s1); } } diff --git a/src/main/java/org/inwc3/jmpq/MpqHeader.java b/src/main/java/org/inwc3/jmpq/MpqHeader.java index 213f6ce..71aeff7 100644 --- a/src/main/java/org/inwc3/jmpq/MpqHeader.java +++ b/src/main/java/org/inwc3/jmpq/MpqHeader.java @@ -692,9 +692,10 @@ private static MpqHeader parseAt(MpqSource source, Located located, boolean forc if (hiBlockTablePosition < 0 || (hiBlockTablePosition != 0 && !source.contains(offset + hiBlockTablePosition, 0))) { - // A position outside the file cannot be a table. Dropping it leaves - // the low words, which is what a version 0 reader would use. - hiBlockTablePosition = 0; + // Kept, not dropped. Dropping it left the low words in place, which + // silently relocates every file in an archive that needs the high + // ones; MpqArchive reports it instead, as StormLib does. Recording + // that the header is malformed is still worth doing. malformed = true; } diff --git a/src/test/java/systems/crigges/jmpq3test/MpqChecksumTests.java b/src/test/java/systems/crigges/jmpq3test/MpqChecksumTests.java new file mode 100644 index 0000000..23ca9c5 --- /dev/null +++ b/src/test/java/systems/crigges/jmpq3test/MpqChecksumTests.java @@ -0,0 +1,99 @@ +package systems.crigges.jmpq3test; + +import org.testng.Assert; +import org.testng.annotations.Test; + +import java.lang.reflect.Method; +import java.nio.charset.StandardCharsets; +import java.util.Random; + +/** + * The seeded-zero Adler-32 that MPQ sector checksums use. + *

+ * Two things need pinning. That the value matches what StormLib computes, which + * is what the literal constants below are for; and that the fast path -- the + * JDK intrinsic plus a seed correction -- agrees with the definition computed + * directly, which is what the fuzz comparison is for. Deriving one from the + * other is an algebraic shortcut, and a shortcut nobody checks is a bug waiting + * to happen. + */ +public class MpqChecksumTests { + + private static Method fast; + private static Method reference; + + private static void load() throws Exception { + if (fast == null) { + final Class type = Class.forName("org.inwc3.jmpq.MpqChecksums"); + fast = type.getDeclaredMethod("adler32", byte[].class, int.class, int.class); + reference = type.getDeclaredMethod("adler32Reference", byte[].class, int.class, int.class); + fast.setAccessible(true); + reference.setAccessible(true); + } + } + + private static int fast(byte[] data, int offset, int length) throws Exception { + load(); + return (int) fast.invoke(null, data, offset, length); + } + + private static int reference(byte[] data, int offset, int length) throws Exception { + load(); + return (int) reference.invoke(null, data, offset, length); + } + + /** Values taken from {@code zlib.adler32(data, 0)}, not from this code. */ + @Test + public void knownValuesMatchZlibSeededWithZero() throws Exception { + final byte[] abc = "abc".getBytes(StandardCharsets.UTF_8); + Assert.assertEquals(fast(abc, 0, abc.length), 0x024A0126, + "seeding with 1 would give 0x024D0127"); + Assert.assertEquals(fast(new byte[0], 0, 0), 0); + + final byte[] many = new byte[10_000]; + java.util.Arrays.fill(many, (byte) 'a'); + Assert.assertEquals(fast(many, 0, many.length), 0x78ABCDE2, + "long enough to cross the block boundary the accumulator folds at"); + } + + /** + * The intrinsic-plus-correction path against the definition, over lengths + * that straddle every boundary that matters: empty, one byte, and either + * side of zlib's 5552-byte fold. + */ + @Test + public void theFastPathAgreesWithTheDefinition() throws Exception { + final Random random = new Random(11); + final int[] lengths = {0, 1, 2, 15, 16, 255, 4096, 5551, 5552, 5553, 11_104, 40_000}; + + for (int length : lengths) { + final byte[] data = new byte[length]; + random.nextBytes(data); + Assert.assertEquals(fast(data, 0, length), reference(data, 0, length), + "length " + length); + + // All-zero and all-0xFF exercise the modulus at both extremes. + Assert.assertEquals(fast(new byte[length], 0, length), + reference(new byte[length], 0, length), "zeroes, length " + length); + final byte[] high = new byte[length]; + java.util.Arrays.fill(high, (byte) 0xFF); + Assert.assertEquals(fast(high, 0, length), reference(high, 0, length), + "0xFF, length " + length); + } + } + + /** Offsets and lengths inside a larger array must be honoured. */ + @Test + public void slicesAreHonoured() throws Exception { + final Random random = new Random(12); + final byte[] data = new byte[8192]; + random.nextBytes(data); + + for (int offset : new int[]{0, 1, 7, 4095}) { + for (int length : new int[]{0, 1, 100, 4000}) { + Assert.assertEquals(fast(data, offset, length), reference(data, offset, length), + "offset " + offset + " length " + length); + } + } + } +} diff --git a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java index d8ee7c7..0b56d7e 100644 --- a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java @@ -538,12 +538,21 @@ public void aNonZeroHiBlockEntryMovesTheFilePosition() throws IOException { } /** - * An archive claiming a hi-block table it does not hold is read with the low - * words alone, which is what a version 0 reader would do anyway. Refusing it - * would lose an archive that is entirely readable. + * An archive claiming a hi-block table it does not hold is reported, not + * quietly read with the low words alone. + *

+ * Ignoring the table looked like leniency and was not: declaring one means + * the file positions do not fit in 32 bits, so dropping the high words puts + * every file at the wrong offset — reads that fail, or worse, succeed with + * the wrong bytes. StormLib treats an unreadable hi-block table as fatal for + * the same reason. + *

+ * The escape hatch is real rather than notional: a version 0 archive never + * consults the field, so a Warcraft III map whose header was corrupted into + * claiming one still opens under {@code FORCE_V0}. */ @Test - public void aHiBlockTableOutsideTheFileIsIgnored() throws IOException { + public void aHiBlockTableOutsideTheFileIsReported() throws IOException { final byte[] plain = MpqArchiveWriter.create(MpqWriteOptions.defaults() .withFormatVersion(1)) .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) @@ -553,13 +562,36 @@ public void aHiBlockTableOutsideTheFileIsIgnored() throws IOException { final int headerAt = headerOffset(plain); edit.putLong(headerAt + 0x20, 0x7FFF_FFFFL); - try (MpqArchive archive = MpqArchive.open(plain, MpqOpenOptions.defaults())) { - Assert.assertFalse(archive.header().hasHiBlockTable(), "dropped as implausible"); - Assert.assertTrue(archive.header().malformed()); + final JMpqException thrown = Assert.expectThrows(JMpqException.class, + () -> MpqArchive.open(plain, MpqOpenOptions.defaults()).close()); + Assert.assertTrue(thrown.getMessage().contains("hi-block table"), thrown.getMessage()); + + // Read as version 0, the field is not part of the header at all. + try (MpqArchive archive = MpqArchive.open(plain, MpqOpenOptions.warcraft3())) { + Assert.assertFalse(archive.header().hasHiBlockTable()); Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8)); } } + /** A table whose position is in the file but which runs off the end, likewise. */ + @Test + public void aTruncatedHiBlockTableIsReported() throws IOException { + final byte[] plain = MpqArchiveWriter.create(MpqWriteOptions.defaults() + .withFormatVersion(1)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + // One byte before the end: inside the file, but far too small to hold + // one entry per block. + final ByteBuffer edit = ByteBuffer.wrap(plain).order(ByteOrder.LITTLE_ENDIAN); + final int headerAt = headerOffset(plain); + edit.putLong(headerAt + 0x20, plain.length - headerAt - 1); + + final JMpqException thrown = Assert.expectThrows(JMpqException.class, + () -> MpqArchive.open(plain, MpqOpenOptions.defaults()).close()); + Assert.assertTrue(thrown.getMessage().contains("hi-block table"), thrown.getMessage()); + } + /** * Appends a hi-block table to a version 1 archive and points the header at * it. The table is neither encrypted nor compressed, per StormLib. @@ -723,11 +755,25 @@ public void exportForReferenceVerification() throws IOException { final StringBuilder expected = new StringBuilder("# archive\tname\tsize\tmd5\n"); + // 8 KiB sectors of incompressible data are stored raw, so a whole + // sector of high-valued bytes reaches the checksum. That is what + // overflows a signed 32-bit Adler accumulator -- and it is invisible to a + // round trip, because both sides would share the same wrong arithmetic. + // Only the reference can see it, which is why this shape is exported. + files.put("wide.bin", incompressible(40_000, 21)); + // High-valued bytes, stored raw: the default recompression setting never + // shrinks a sector, so these reach the checksum as they are. A run of + // 0xFF is what pushes the Adler accumulator past a signed 32-bit range. + final byte[] high = new byte[40_000]; + java.util.Arrays.fill(high, (byte) 0xFF); + files.put("high.bin", high); + final MpqWriteOptions[] shapes = { MpqWriteOptions.defaults().withSectorChecksums(true), MpqWriteOptions.defaults().withAttributes(true).withAttributesTimestamp(0), MpqWriteOptions.defaults().withSectorChecksums(true).withAttributes(true) .withAttributesTimestamp(0).withFormatVersion(1), + MpqWriteOptions.defaults().withSectorChecksums(true).withSectorSizeShift(4), }; for (int shape = 0; shape < shapes.length; shape++) { From c200e32ed2c4a1c049fda9fac4dc85983d765c9b Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 11:54:07 +0200 Subject: [PATCH 12/13] Address review: unknown attributes tails, and bound the user data payload The parser claimed to read the known prefix of an attributes file and ignore the rest, and did not: it required the length to match the known arrays exactly, so any file actually carrying an unknown array was rejected and its perfectly good checksums went with it. An exact length is still preferred; a longer file is accepted only when the bytemask names an array this implementation does not know, so a tail nothing explains stays an error. The test that was supposed to cover this passed for the wrong reason: it set an unknown flag without appending the array it names, so the length still matched exactly and the case was never exercised. It now appends the array. A user data header declaring more payload than fits before the archive returned the archive itself as metadata, because the clamp was to the end of the file rather than to the redirect offset -- which is the actual end of the user data area. The narrowing to an array length is bounded now too, since both clamps are 64-bit. MpqUserData.payload needs an MpqSource, which no caller outside this package can obtain, so it was unreachable. MpqArchive.userDataPayload is the way in. --- src/main/java/org/inwc3/jmpq/MpqArchive.java | 15 ++++++ .../java/org/inwc3/jmpq/MpqAttributes.java | 29 +++++++++-- src/main/java/org/inwc3/jmpq/MpqUserData.java | 25 +++++++-- .../crigges/jmpq3test/MpqAttributesTests.java | 51 ++++++++++++++++--- .../crigges/jmpq3test/Phase2FormatTests.java | 35 +++++++++++++ 5 files changed, 141 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index d6b0865..b3a905a 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -229,6 +229,21 @@ public Optional userData() { return Optional.ofNullable(header.userData()); } + /** + * The payload of this archive's user data header, if it has one. + *

+ * {@link MpqUserData#payload} needs an {@link MpqSource}, which a caller + * holding an archive has no way to obtain, so it was unreachable from + * outside this package. This is the way in. + * + * @return the user data payload, or empty when the archive starts the file. + * @throws IOException if the bytes cannot be read. + */ + public Optional userDataPayload() throws IOException { + final MpqUserData userData = header.userData(); + return userData == null ? Optional.empty() : Optional.of(userData.payload(source)); + } + /** * The archive's {@code (attributes)} file, parsed. *

diff --git a/src/main/java/org/inwc3/jmpq/MpqAttributes.java b/src/main/java/org/inwc3/jmpq/MpqAttributes.java index 9024690..2ef860f 100644 --- a/src/main/java/org/inwc3/jmpq/MpqAttributes.java +++ b/src/main/java/org/inwc3/jmpq/MpqAttributes.java @@ -197,23 +197,46 @@ private static int inMemorySize(int flags, int entries, long patchBytes) { * writes one and reads the other. A length matching none is reported rather * than guessed at. * + * When the bytemask names an array this implementation does not understand, + * the file is longer than the known arrays account for, and the surplus is + * that unknown array. The known prefix is still resolvable, so it is. + * + * @param flags the bytemask as stored. * @param usable the bytemask, restricted to arrays we understand. * @param blockCount block table rows the archive has. * @param length the file length. * @return the entry count that length implies. * @throws JMpqException if no candidate matches. */ - private static int resolveEntryCount(int usable, int blockCount, int length) + private static int resolveEntryCount(int flags, int usable, int blockCount, int length) throws JMpqException { final int fewest = Math.max(0, blockCount - 1); + + // An exact length first, so a file with nothing unexpected in it is + // resolved the same way regardless of what follows. for (int entries = blockCount; entries >= fewest; entries--) { if (sizeFor(usable, entries, patchBitBytesStormLibWrites(entries)) == length || sizeFor(usable, entries, patchBitBytesNeeded(entries)) == length) { return entries; } } + + // Then, only when the bytemask names an array this implementation does + // not know, a known prefix followed by that array. Without this the + // promise of reading the known prefix and ignoring the rest was empty: + // any file actually carrying an unknown array was rejected outright, + // taking its perfectly good checksums with it. Gated on there being an + // unknown bit, so a stray tail on an otherwise-known file stays an error. + if (flags != usable) { + for (int entries = blockCount; entries >= fewest; entries--) { + if (sizeFor(usable, entries, patchBitBytesStormLibWrites(entries)) <= length) { + return entries; + } + } + } + throw new JMpqException("An attributes file with flags 0x" - + Integer.toHexString(usable) + " for " + blockCount + " blocks should be " + + Integer.toHexString(flags) + " for " + blockCount + " blocks should be " + sizeFor(usable, blockCount) + " bytes, but is " + length + "."); } @@ -249,7 +272,7 @@ public static MpqAttributes parse(byte[] data, int blockCount) throws JMpqExcept // ignoring the rest is what StormLib does. final int usable = flags & KNOWN_FLAGS; - final int entries = resolveEntryCount(usable, blockCount, data.length); + final int entries = resolveEntryCount(flags, usable, blockCount, data.length); final boolean truncated = entries != blockCount; final int[] crc32 = (usable & HAS_CRC32) != 0 ? new int[entries] : new int[0]; diff --git a/src/main/java/org/inwc3/jmpq/MpqUserData.java b/src/main/java/org/inwc3/jmpq/MpqUserData.java index b213520..d90273c 100644 --- a/src/main/java/org/inwc3/jmpq/MpqUserData.java +++ b/src/main/java/org/inwc3/jmpq/MpqUserData.java @@ -37,6 +37,12 @@ public record MpqUserData( /** Size of the fixed part of a user data header. */ public static final int SIZE = 16; + /** + * Largest payload {@link #payload} will return. Both bounds it clamps to are + * 64-bit, so without this the narrowing to an array length could wrap. + */ + private static final long MAX_PAYLOAD = Integer.MAX_VALUE - 8L; + /** * Reads a user data header. * @@ -76,10 +82,21 @@ public long archiveHeaderOffset() { public byte[] payload(MpqSource source) throws systems.crigges.jmpq3.JMpqException { final long start = offset + SIZE; final long declared = Integer.toUnsignedLong(userDataSize); + // The declared size is not trustworthy: it is a plain u32 written by - // another tool, so clamp it to the file rather than letting it drive - // the allocation. - final long available = Math.max(0, source.size() - start); - return source.bytes(start, (int) Math.min(declared, available)); + // another tool. Two bounds apply, and the tighter one is the archive + // header this block redirects to -- the user data area ends where the + // archive begins. Clamping only to the file returned the archive itself + // as though it were metadata. + final long limit = Math.min(source.size(), Math.max(start, archiveHeaderOffset())); + final long available = Math.max(0, limit - start); + final long length = Math.min(declared, available); + + if (length > MAX_PAYLOAD) { + throw new systems.crigges.jmpq3.JMpqException("A user data header at " + offset + + " describes " + length + " bytes of payload, more than can be returned" + + " in one array."); + } + return source.bytes(start, (int) length); } } diff --git a/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java b/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java index 8f1441c..1086c8d 100644 --- a/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java +++ b/src/test/java/systems/crigges/jmpq3test/MpqAttributesTests.java @@ -116,25 +116,54 @@ public void anImplausibleLengthIsRejected() { } /** - * An unknown bit means an array of unknown length, so nothing past the - * known arrays can be located. The known prefix is still read. + * An unknown bit means an array of unknown length, so nothing past the known + * arrays can be located. The known prefix is still read. + *

+ * The trailing bytes are the point. An earlier version of this test set an + * unknown flag without appending the array it names, so the file length + * still matched the known arrays exactly and the test passed without ever + * exercising the case it was named after — while a real file carrying an + * unknown array was rejected outright, losing its good checksums with it. */ @Test public void unknownFlagsDoNotStopTheKnownArraysBeingRead() throws JMpqException { - final byte[] file = MpqAttributes.build(new int[]{7, 8}, new long[]{9, 10}); - // Set a bit no version of the format defines. - file[4] |= 0x40; + final byte[] known = MpqAttributes.build(new int[]{7, 8}, new long[]{9, 10}); + known[4] |= 0x40; + + // The unknown array actually present, as a file in the wild would have. + final byte[] file = new byte[known.length + 32]; + System.arraycopy(known, 0, file, 0, known.length); + for (int i = known.length; i < file.length; i++) { + file[i] = (byte) 0xA5; + } final MpqAttributes attributes = MpqAttributes.parse(file, 2); Assert.assertEquals(attributes.crc32Of(0), 7); + Assert.assertEquals(attributes.crc32Of(1), 8); Assert.assertEquals(attributes.fileTimeOf(1), 10); Assert.assertTrue(attributes.has(0x40), "the flag is preserved as stored"); + Assert.assertEquals(attributes.entries(), 2); + // Re-emitting drops what could not be understood, and says so by // emitting only the known bits. Assert.assertEquals(MpqAttributes.parse(attributes.toByteArray(), 2).flags(), MpqAttributes.HAS_CRC32 | MpqAttributes.HAS_FILETIME); } + /** + * The leniency is confined to what an unknown bit can explain. A file whose + * bytemask this implementation fully understands, with bytes on the end that + * nothing accounts for, is still an error. + */ + @Test + public void aTailNothingExplainsIsStillAnError() { + final byte[] known = MpqAttributes.build(new int[]{7, 8}, new long[]{9, 10}); + final byte[] file = new byte[known.length + 16]; + System.arraycopy(known, 0, file, 0, known.length); + + Assert.expectThrows(JMpqException.class, () -> MpqAttributes.parse(file, 2)); + } + /** The default shape round-trips through build and parse unchanged. */ @Test public void theDefaultShapeRoundTrips() throws JMpqException { @@ -305,7 +334,15 @@ public void aBytemaskNamingNothingKnownDescribesNothing() throws JMpqException { out.putInt(0x40); Assert.assertEquals(new AttributesFile(out.array()).entries(), 0); - // 64 bytes is not 8, so the strict parser reports the mismatch instead. - Assert.expectThrows(JMpqException.class, () -> MpqAttributes.parse(out.array(), 3)); + + // The known prefix here is empty, and the rest is the array the unknown + // bit names, so parsing succeeds and reports nothing recorded. Refusing + // would deny an archive its attributes because of a field this + // implementation is explicitly willing to ignore. + final MpqAttributes attributes = MpqAttributes.parse(out.array(), 3); + Assert.assertEquals(attributes.crc32().length, 0); + Assert.assertEquals(attributes.fileTimes().length, 0); + Assert.assertEquals(attributes.crc32Of(0), 0, "nothing recorded"); + Assert.assertTrue(attributes.has(0x40)); } } diff --git a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java index 0b56d7e..931e495 100644 --- a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java @@ -689,6 +689,41 @@ public void aUserDataHeaderIsParsedAndItsPayloadKept() throws IOException { } } + /** + * A user data header declaring more payload than fits before the archive + * gets clamped to where the archive starts, not to the end of the file. + *

+ * The redirect offset is the real upper bound of the user data area. Clamping + * only to the file handed the caller the archive itself as though it were + * metadata — and for a large archive the narrowing to an array length could + * wrap on the way. + */ + @Test + public void aUserDataPayloadStopsWhereTheArchiveBegins() throws IOException { + final byte[] inner = MpqArchiveWriter.create(MpqWriteOptions.defaults().withPrefix(false)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + final byte[] image = withUserData(inner, "small".getBytes(StandardCharsets.UTF_8)); + + // Claim the whole address space as user data. + ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN).putInt(0x04, -1); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + final byte[] payload = archive.userDataPayload().orElseThrow(); + + Assert.assertEquals(payload.length, MpqHeader.ALIGNMENT - MpqUserData.SIZE, + "the payload ends where the archive header begins"); + Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8)); + + // And it really is metadata, not the archive: no header signature in it. + final ByteBuffer scan = ByteBuffer.wrap(payload).order(ByteOrder.LITTLE_ENDIAN); + for (int at = 0; at + 4 <= payload.length; at += 4) { + Assert.assertNotEquals(scan.getInt(at), MpqHeader.ARCHIVE_SIGNATURE, + "archive bytes leaked into the payload at " + at); + } + } + } + /** Wraps an archive behind a user data header at offset 0. */ private static byte[] withUserData(byte[] archive, byte[] payload) { final byte[] out = new byte[MpqHeader.ALIGNMENT + archive.length]; From 886f403844f9d211175f193662b511c94ac7fd5e Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 12:01:59 +0200 Subject: [PATCH 13/13] Bound the user data payload by the redirect, not by either size field Neither size field in a user data header is the payload length. StormLib documents cbUserDataSize as the *maximum* size of the area -- a capacity -- and its comment on cbUserDataHeader is openly unsure ("Appears to be size of user data header"). What SFileGetFileInfo actually returns for SFileMpqUserData is the span between the two headers: ha->UserDataPos + sizeof(TMPQUserData), ha->pUserData->dwHeaderOffs - sizeof(TMPQUserData) So the payload is now that span, clamped to the file because the redirect is an untrusted u32, and both size fields are carried for inspection without bounding anything. Using the capacity truncated an archive that reserved more area than it filled; using it as the only bound returned the archive itself as metadata when it was garbage. The ambiguous field is renamed userDataHeaderSize to stop it reading like the size of the 16-byte header, and format note 15 records the citation. --- docs/mpq-format-notes.md | 32 +++++++++++++ src/main/java/org/inwc3/jmpq/MpqUserData.java | 46 +++++++++++++------ .../crigges/jmpq3test/Phase2FormatTests.java | 32 ++++++++++++- 3 files changed, 95 insertions(+), 15 deletions(-) diff --git a/docs/mpq-format-notes.md b/docs/mpq-format-notes.md index 3b14dae..3c13a4b 100644 --- a/docs/mpq-format-notes.md +++ b/docs/mpq-format-notes.md @@ -431,3 +431,35 @@ Two properties keep this safe to have strengthened: header where a naive scan found one — never fewer. - The test only rejects on things that make an archive unreadable regardless, so a candidate it rejects would have failed at table-parse time in any case. + + +## 15. The user data payload is bounded by the redirect, not by either size field + +A user data header carries two size fields and neither is the payload length. +StormLib documents them as: + +```c +DWORD cbUserDataSize; // Maximum size of the user data +DWORD dwHeaderOffs; // Offset of the MPQ header, relative to the begin of this header +DWORD cbUserDataHeader; // Appears to be size of user data header (Starcraft II maps) +``` + +`cbUserDataSize` is a capacity, so an archive may reserve more area than it +filled. `cbUserDataHeader` has no agreed meaning — note that StormLib's own +comment says "appears to be". + +What StormLib hands a caller asking for the user data is neither: + +```c +// SFileGetFileInfo.cpp, case SFileMpqUserData +ha->UserDataPos + sizeof(TMPQUserData), +ha->pUserData->dwHeaderOffs - sizeof(TMPQUserData) +``` + +**Decision.** `MpqUserData.payload` returns the span between the end of the user +data header and the archive header, clamped to the file because the redirect is +an untrusted `u32`. Both size fields are exposed for inspection and neither +bounds anything. Reading `cbUserDataSize` as a length truncates an archive that +reserved generously; reading it as the only bound returned the archive itself as +metadata when it was garbage. + diff --git a/src/main/java/org/inwc3/jmpq/MpqUserData.java b/src/main/java/org/inwc3/jmpq/MpqUserData.java index d90273c..8fa7e65 100644 --- a/src/main/java/org/inwc3/jmpq/MpqUserData.java +++ b/src/main/java/org/inwc3/jmpq/MpqUserData.java @@ -23,16 +23,34 @@ * caller can read the user data area, and — more importantly — a rebuild can * preserve it instead of silently dropping a map's metadata. * + *

Which field is the payload length

+ * Neither of the size fields, as it turns out. StormLib documents + * {@code cbUserDataSize} as the maximum size of the user data — a + * capacity, not a length — and its comment on {@code cbUserDataHeader} is openly + * unsure: "Appears to be size of user data header (Starcraft II maps)". What + * StormLib actually hands a caller asking for the user data is the span between + * the two headers: + * 
+ * // SFileGetFileInfo.cpp, case SFileMpqUserData
+ * ha->UserDataPos + sizeof(TMPQUserData),
+ * ha->pUserData->dwHeaderOffs - sizeof(TMPQUserData)
+ *
+ * So the redirect offset defines the payload, and both size fields are carried + * here for inspection without being trusted to bound anything. + * * @param offset where this header sits in the file. - * @param userDataSize declared size of the user data area that follows. - * @param headerOffset archive header offset, relative to {@link #offset}. - * @param headerSize declared size of this user data header. + * @param userDataSize {@code cbUserDataSize}: the capacity of the user data + * area, which is advisory and may exceed what is there. + * @param headerOffset {@code dwHeaderOffs}: archive header offset, relative to + * {@link #offset}. This is what bounds the payload. + * @param userDataHeaderSize {@code cbUserDataHeader}, whose meaning StormLib + * itself hedges on. Recorded, not relied upon. */ public record MpqUserData( long offset, int userDataSize, int headerOffset, - int headerSize) { + int userDataHeaderSize) { /** Size of the fixed part of a user data header. */ public static final int SIZE = 16; @@ -76,25 +94,25 @@ public long archiveHeaderOffset() { * The user data payload, which is whatever the producing tool put there. * * @param source the archive bytes. - * @return the payload, truncated to what the file actually holds. + * @return everything between this header and the archive header, truncated + * to what the file actually holds. * @throws systems.crigges.jmpq3.JMpqException if the bytes cannot be read. */ public byte[] payload(MpqSource source) throws systems.crigges.jmpq3.JMpqException { final long start = offset + SIZE; - final long declared = Integer.toUnsignedLong(userDataSize); - // The declared size is not trustworthy: it is a plain u32 written by - // another tool. Two bounds apply, and the tighter one is the archive - // header this block redirects to -- the user data area ends where the - // archive begins. Clamping only to the file returned the archive itself - // as though it were metadata. + // The span between the two headers, which is what StormLib returns, and + // then clamped to the file because the redirect offset is a plain u32 + // written by another tool. Neither size field takes part: one is a + // capacity and the other has no agreed meaning, so an archive reserving + // more area than it filled, or declaring less than it holds, is read the + // same way StormLib reads it. final long limit = Math.min(source.size(), Math.max(start, archiveHeaderOffset())); - final long available = Math.max(0, limit - start); - final long length = Math.min(declared, available); + final long length = Math.max(0, limit - start); if (length > MAX_PAYLOAD) { throw new systems.crigges.jmpq3.JMpqException("A user data header at " + offset - + " describes " + length + " bytes of payload, more than can be returned" + + " spans " + length + " bytes before the archive, more than can be returned" + " in one array."); } return source.bytes(start, (int) length); diff --git a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java index 931e495..77056a9 100644 --- a/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java @@ -674,7 +674,7 @@ public void aUserDataHeaderIsParsedAndItsPayloadKept() throws IOException { try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { final MpqUserData userData = archive.userData().orElseThrow(); Assert.assertEquals(userData.offset(), 0); - Assert.assertEquals(userData.headerSize(), MpqUserData.SIZE); + Assert.assertEquals(userData.userDataHeaderSize(), MpqUserData.SIZE); Assert.assertEquals(userData.archiveHeaderOffset(), MpqHeader.ALIGNMENT); Assert.assertEquals(archive.header().headerOffset(), MpqHeader.ALIGNMENT); Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8)); @@ -724,6 +724,36 @@ public void aUserDataPayloadStopsWhereTheArchiveBegins() throws IOException { } } + /** + * A declared user data size smaller than the gap does not shorten the + * payload either. + *

+ * {@code cbUserDataSize} is documented as the maximum size of the + * area — a capacity, not a length — and StormLib ignores it when handing back + * the user data, using the span to the archive header instead. An archive + * that reserved a 512-byte area and filled five bytes of it still has 496 + * bytes of user data area, and that is what a caller gets. + */ + @Test + public void theDeclaredUserDataSizeDoesNotBoundThePayload() throws IOException { + final byte[] inner = MpqArchiveWriter.create(MpqWriteOptions.defaults().withPrefix(false)) + .put("a.txt", "content".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + final byte[] image = withUserData(inner, "five!".getBytes(StandardCharsets.UTF_8)); + + // Declare far less than the area actually spans. + ByteBuffer.wrap(image).order(ByteOrder.LITTLE_ENDIAN).putInt(0x04, 5); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.userData().orElseThrow().userDataSize(), 5); + Assert.assertEquals(archive.userDataPayload().orElseThrow().length, + MpqHeader.ALIGNMENT - MpqUserData.SIZE, + "the span to the archive header is what defines the payload"); + Assert.assertEquals(archive.read("a.txt"), + "content".getBytes(StandardCharsets.UTF_8)); + } + } + /** Wraps an archive behind a user data header at offset 0. */ private static byte[] withUserData(byte[] archive, byte[] payload) { final byte[] out = new byte[MpqHeader.ALIGNMENT + archive.length];