From 36b1c5066c728a4fe09cb4aa033ff3cb050d8624 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 08:57:55 +0200 Subject: [PATCH 1/6] Phase 1: reduce JMpqEditor to a facade over the new core Completes P1-1. JMpqEditor drops from 1566 lines to 764 and contains no format-level operations at all: no encryption, no compression, no sector arithmetic, no buffer sizing. Everything delegates to MpqArchive and MpqArchiveWriter. The motivation is not tidiness. Two implementations of one format drift, and this branch proved it twice: the stored-encrypted sector defect had to be found and fixed separately in each copy, once on the read path and once on the copy path. There is now one implementation. Behaviour deliberately preserved, because callers depend on it: - rebuild on close, rather than the core's explicit save - read-only downgrade for an archive with no (listfile), recoverable through setExternalListfile - insertFile stores a path and reads it at close; insertByteArray copies - the block-index fallback in extractAllFiles for archives that cannot name their own contents Two behaviours I had dropped and the old suite caught: - the downgrade tested whether the list file named anything rather than whether it existed, so a freshly created archive was read-only and could never receive its first file - extractAllFiles produced nothing for a list-file-less archive, having lost the fallback that dumps blocks by index Supporting the deprecated accessors that expose the on-disk index needed three additive read-only accessors on MpqArchive (hashTable, rawBytes, rawBlocks) and BlockTable.of for building one in memory. They are documented as existing for this facade; new code has entries() and entry(). One test now asserts the opposite of what it did. Phase 0 rejected a garbage header size and told the caller to retry with FORCE_V0; P2-5a asks for StormLib's leniency, because Storm.dll ignores the field and the game loads these maps. The new core repairs the size, flags the archive malformed and opens it, so listfileTooLong.w3x is now readable without a special option. That is P2-5a arriving early rather than a regression. Full suite green: 120 tests. --- src/main/java/org/inwc3/jmpq/MpqArchive.java | 41 + .../systems/crigges/jmpq3/BlockTable.java | 28 + .../systems/crigges/jmpq3/JMpqEditor.java | 1514 ++++------------- .../jmpq3test/Phase0RegressionTests.java | 23 +- 4 files changed, 444 insertions(+), 1162 deletions(-) diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index 8482044..3acc986 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -440,6 +440,47 @@ private void readNames() { } } + /** + * The hash table backing this archive. + *

+ * Exposed for the deprecated {@code JMpqEditor} adapter, whose public API + * hands this object to callers. New code should use {@link #entry(String)} + * and {@link #localesOf(String)} instead, which do not require knowing how + * the format stores its index. + * + * @return the live hash table. + */ + public HashTable hashTable() { + return hashTable; + } + + /** + * A file's bytes exactly as stored, without decryption. + *

+ * Exposed for the deprecated adapter, which constructs legacy + * {@code MpqFile} objects that do their own decoding. New code should use + * {@link #read(MpqFileEntry)}. + * + * @param entry the file. + * @return exactly {@link MpqFileEntry#compressedSize()} bytes. + * @throws IOException if the range lies outside the archive. + */ + public byte[] rawBytes(MpqFileEntry entry) throws IOException { + return source.bytes(header.headerOffset() + entry.filePosition(), entry.compressedSize()); + } + + /** + * Every block table row, live or not, in table order. + *

+ * Exposed for the deprecated adapter. {@link #entries()} is the supported + * form: it skips dead rows and attaches names and locales. + * + * @return the raw rows. + */ + public List rawBlocks() { + return List.of(blocks); + } + /** * The bytes preceding the archive header. *

diff --git a/src/main/java/systems/crigges/jmpq3/BlockTable.java b/src/main/java/systems/crigges/jmpq3/BlockTable.java index 7dc1316..2cfb9d8 100644 --- a/src/main/java/systems/crigges/jmpq3/BlockTable.java +++ b/src/main/java/systems/crigges/jmpq3/BlockTable.java @@ -45,6 +45,34 @@ public BlockTable(ByteBuffer buf) throws IOException { this.blockMap.order(ByteOrder.LITTLE_ENDIAN); } + /** + * Wraps an already-decoded set of rows, for the deprecated + * {@code JMpqEditor} adapter, which obtains its rows from the new core + * rather than by decrypting the table itself. + * + * @param rows block table rows in table order. + * @return a block table over those rows. + */ + public static BlockTable of(List rows) { + final ByteBuffer plain = ByteBuffer.allocate(rows.size() * ENTRY_SIZE) + .order(ByteOrder.LITTLE_ENDIAN); + for (Block row : rows) { + row.writeToBuffer(plain); + } + plain.clear(); + // The constructor decrypts, so hand it an encrypted image built from + // these rows rather than duplicating the decoding logic here. + final ByteBuffer encrypted = ByteBuffer.allocate(rows.size() * ENTRY_SIZE) + .order(ByteOrder.LITTLE_ENDIAN); + new MPQEncryption(KEY_BLOCK_TABLE, false).processFinal(plain, encrypted); + encrypted.rewind(); + try { + return new BlockTable(encrypted); + } catch (IOException e) { + throw new IllegalStateException("Cannot rebuild an in-memory block table.", e); + } + } + /** * @return number of entries, live or not. */ diff --git a/src/main/java/systems/crigges/jmpq3/JMpqEditor.java b/src/main/java/systems/crigges/jmpq3/JMpqEditor.java index 22bfcd8..987368d 100644 --- a/src/main/java/systems/crigges/jmpq3/JMpqEditor.java +++ b/src/main/java/systems/crigges/jmpq3/JMpqEditor.java @@ -1,314 +1,156 @@ package systems.crigges.jmpq3; -import org.apache.commons.compress.utils.SeekableInMemoryByteChannel; +import org.inwc3.jmpq.MpqArchive; +import org.inwc3.jmpq.MpqArchiveWriter; +import org.inwc3.jmpq.MpqFileEntry; +import org.inwc3.jmpq.MpqHeader; +import org.inwc3.jmpq.MpqOpenOptions; +import org.inwc3.jmpq.MpqWriteOptions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import systems.crigges.jmpq3.BlockTable.Block; import systems.crigges.jmpq3.compression.RecompressOptions; -import systems.crigges.jmpq3.security.MPQEncryption; -import systems.crigges.jmpq3.security.MPQHashGenerator; -import java.io.EOFException; import java.io.File; import java.io.IOException; import java.io.OutputStream; import java.nio.ByteBuffer; import java.nio.ByteOrder; -import java.nio.channels.FileChannel; import java.nio.channels.NonWritableChannelException; -import java.nio.channels.ReadableByteChannel; -import java.nio.channels.SeekableByteChannel; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; -import java.nio.file.OpenOption; import java.nio.file.Path; -import java.nio.file.StandardOpenOption; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; -import java.util.Map; -import java.util.SequencedMap; - -import static systems.crigges.jmpq3.MpqFile.ADJUSTED_ENCRYPTED; -import static systems.crigges.jmpq3.MpqFile.COMPRESSED; -import static systems.crigges.jmpq3.MpqFile.ENCRYPTED; -import static systems.crigges.jmpq3.MpqFile.EXISTS; +import java.util.Optional; /** - * Provides an interface for using MPQ archive files. MPQ archive files contain - * a virtual file system used by some old games to hold data, primarily those - * from Blizzard Entertainment. - *

- * MPQ archives are not intended as a general purpose file system. File access - * and reading is highly efficient. File manipulation and writing is not - * efficient and may require rebuilding a large portion of the archive file. - * Empty directories are not supported. The full contents of the archive might - * not be discoverable, but such files can still be accessed if their full path - * is known. File attributes are optional. - *

- * For platform independence the implementation is pure Java. + * Compatibility facade over the {@code org.inwc3.jmpq} core. * - *

Thread safety

- * A single editor instance is not thread safe and must be confined to one - * thread. Separate instances, however, are fully independent: as of 2.0 nothing - * in the library holds global mutable state. Before 2.0, opening any archive - * wiped a shared {@code %TMP%/jmpq} directory and the compression codecs were - * static singletons, so concurrent use corrupted data. + * @deprecated use {@link MpqArchive} to read and {@link MpqArchiveWriter} to + * write. This class exists so code written against JMPQ3 1.x keeps + * compiling and behaving as it did; it will be removed in a future + * major release. + * + *

What this class is now

+ * Every method here delegates. The 1400 lines of archive logic that used to + * live in this file are gone, because two implementations of the same format + * inevitably drift: during the 2.0 work the same sector-decryption defect had + * to be found and fixed twice, once in each copy. There is now one + * implementation and this facade. * - *

Rebuild model

- * A writable archive is rebuilt when {@link #close()} runs. The rebuild happens - * entirely in memory and the finished image replaces the file in one write; no - * temporary files are involved. + *

Behaviour deliberately preserved

+ * + * + *

Thread safety

+ * Not thread safe; confine one editor to one thread. Separate editors are + * independent. */ +@Deprecated public class JMpqEditor implements AutoCloseable { private static final Logger log = LoggerFactory.getLogger(JMpqEditor.class); - public static final int ARCHIVE_HEADER_MAGIC = - ByteBuffer.wrap(new byte[]{'M', 'P', 'Q', 0x1A}).order(ByteOrder.LITTLE_ENDIAN).getInt(); - public static final int USER_DATA_HEADER_MAGIC = - ByteBuffer.wrap(new byte[]{'M', 'P', 'Q', 0x1B}).order(ByteOrder.LITTLE_ENDIAN).getInt(); - - /** Header size for each format version, indexed by version. */ - private static final int[] HEADER_SIZES = {32, 44, 68, 208}; - - /** Largest archive size a 32-bit header field can express. */ - private static final long V0_MAX_ARCHIVE_SIZE = 0xFFFFFFFFL; - - /** StormLib's {@code HASH_TABLE_SIZE_MAX}. */ - private static final int HASH_TABLE_SIZE_MAX = 0x00080000; - - /** - * Largest sector size shift that keeps {@code 512 << shift} inside a - * positive int. - */ - private static final int MAX_SECTOR_SIZE_SHIFT = 21; + /** {@code 'MPQ'}, the archive header signature. */ + public static final int ARCHIVE_HEADER_MAGIC = MpqHeader.ARCHIVE_SIGNATURE; - /** Alignment of candidate archive header positions. */ - private static final int HEADER_ALIGNMENT = 0x200; + /** {@code 'MPQ'}, the user data header signature. */ + public static final int USER_DATA_HEADER_MAGIC = MpqHeader.USER_DATA_SIGNATURE; - /** - * Encryption key for hash table data. - */ - private static final int KEY_HASH_TABLE; + /** The archive being read. Replaced after a rebuild. */ + private MpqArchive archive; - /** - * Encryption key for block table data. - */ - private static final int KEY_BLOCK_TABLE; - - static { - final MPQHashGenerator hasher = MPQHashGenerator.getFileKeyGenerator(); - hasher.process("(hash table)"); - KEY_HASH_TABLE = hasher.getHash(); - hasher.reset(); - hasher.process("(block table)"); - KEY_BLOCK_TABLE = hasher.getHash(); + /** A file the caller inserted, resolved at close time. */ + private record Insert(String name, byte[] bytes, Path file) { } - private AttributesFile attributes; + /** Insertions, keyed on canonical name, in insertion order. */ + private final java.util.SequencedMap inserts = new java.util.LinkedHashMap<>(); /** - * MPQ format version 0 forced compatibility is being used. - */ - private final boolean legacyCompatibility; - - /** The archive's backing channel. */ - private final SeekableByteChannel fc; - - /** Whether this editor owns {@link #fc} and must close it. */ - private final boolean ownsChannel; - - /** Offset of the archive header within the file. */ - private long headerOffset; - - /** Size of the archive header in bytes. */ - private int headerSize; - - /** Archive size as recorded in the header, possibly clamped. */ - private long archiveSize; - - /** Raw {@code wFormatVersion}. */ - private int formatVersion; - - private int sectorSizeShift; - - /** Sector size in bytes. */ - private int discBlockSize; - - /** Hash table position relative to {@link #headerOffset}. */ - private long hashPos; - - /** Block table position relative to {@link #headerOffset}. */ - private long blockPos; - - /** Number of hash table buckets. */ - private int hashSize; - - /** Number of block table entries. */ - private int blockSize; - - private HashTable hashTable; - private BlockTable blockTable; - private Listfile listFile = new Listfile(); - - /** - * A file waiting to be written on the next rebuild. + * Names supplied through {@link #setExternalListfile(File)}. *

- * Exactly one of {@code path} and {@code data} is set. - * - * @param displayName the name as the caller spelled it, preserved for the - * rebuilt list file. - * @param path source file to read at rebuild time. - * @param data content, already copied out of the caller's array. + * An archive with no list file cannot enumerate itself, so a rebuild has to + * be told which of its files to carry over. */ - private record PendingFile(String displayName, Path path, byte[] data) { - static PendingFile of(String name, Path path) { - return new PendingFile(name, path, null); - } + private final java.util.SequencedSet externalNames = new java.util.LinkedHashSet<>(); - static PendingFile of(String name, byte[] data) { - // Copy on insert: the caller is free to mutate its array - // afterwards, and the old implementation would then write the - // mutated content. - return new PendingFile(name, null, data.clone()); - } - - byte[] read() throws IOException { - return data != null ? data : Files.readAllBytes(path); - } - } - - /** - * Files to add or replace on the next rebuild, keyed on canonical name and - * kept in insertion order. - *

- * This was an identity-keyed map before 2.0, so {@code deleteFile(name)} - * only worked if the caller passed the very same {@code String} instance - * used at insert time; otherwise the "deleted" file quietly reappeared. - */ - private final SequencedMap pendingFiles = new LinkedHashMap<>(); - - /** Whether to preserve the bytes before the archive header on rebuild. */ - private boolean keepHeaderOffset = true; + /** The file this editor was opened from, or null for an in-memory archive. */ + private final Path path; - private int newHeaderSize; - private long newArchiveSize; - private int newFormatVersion; - private int newSectorSizeShift; - private int newDiscBlockSize; - private long newHashPos; - private long newBlockPos; - private int newHashSize; - private int newBlockSize; + /** The bytes an in-memory archive was opened from. */ + private byte[] memoryImage; - /** - * Whether the caller asked for a writable archive. - *

- * Distinct from {@link #canWrite}, which is the effective mode and - * gets downgraded when the archive has no usable list file. Without the - * distinction, {@link #setExternalListfile(File)} could never help: it - * refuses to run on a read-only editor, so the very archives it exists for - * were the ones it turned away. - */ + private final boolean forceV0; private final boolean writeRequested; - - /** - * If write operations are supported on the archive. - */ private boolean canWrite; + private boolean keepHeaderOffset = true; + private boolean closed; + + /** Names the caller deleted, so a rebuild does not carry them over. */ + private final List deleted = new ArrayList<>(); - /** The rebuilt archive image, available after a successful rebuild. */ + /** The rebuilt image from the most recent close. */ private byte[] outputByteArray; /** * Opens the MPQ archive at the specified path. - *

- * The file must already exist. Unlike before 2.0, opening a writable - * archive never creates the file: probing a path that did not exist used to - * leave an empty file behind (issue #38). Use - * {@link #createEmptyArchive(File)} to make a new archive explicitly. - *

- * Changes made through this editor only reach the file system when - * {@link #close()} is called. * - * @param mpqArchive path to an MPQ archive file. + * @param mpqArchive path to an MPQ archive file; must exist. * @param openOptions options to use when opening the archive. * @throws JMpqException if the archive is missing, damaged or unsupported. */ public JMpqEditor(Path mpqArchive, MPQOpenOption... openOptions) throws JMpqException { - writeRequested = !Arrays.asList(openOptions).contains(MPQOpenOption.READ_ONLY); - canWrite = writeRequested; - legacyCompatibility = Arrays.asList(openOptions).contains(MPQOpenOption.FORCE_V0); - log.debug("Opening {}", mpqArchive); - - if (!Files.isRegularFile(mpqArchive)) { - throw new JMpqException("Not an MPQ archive file: " + mpqArchive.toAbsolutePath()); - } - - SeekableByteChannel channel = null; + this.path = mpqArchive; + this.forceV0 = has(openOptions, MPQOpenOption.FORCE_V0); + this.writeRequested = !has(openOptions, MPQOpenOption.READ_ONLY); + this.canWrite = writeRequested; try { - final OpenOption[] fcOptions = canWrite - ? new OpenOption[]{StandardOpenOption.READ, StandardOpenOption.WRITE} - : new OpenOption[]{StandardOpenOption.READ}; - channel = FileChannel.open(mpqArchive, fcOptions); - fc = channel; - ownsChannel = true; - - readMpq(); + this.archive = MpqArchive.open(mpqArchive, openOptions()); } catch (JMpqException e) { - closeQuietly(channel); - // Keep the diagnostic in the top-level message: a caller who only - // prints getMessage() must still learn what was wrong with the - // archive, not just which file it was. - throw new JMpqException(mpqArchive.toAbsolutePath() + ": " + e.getMessage(), e); + throw e; } catch (IOException e) { - closeQuietly(channel); throw new JMpqException("Cannot open MPQ archive " + mpqArchive.toAbsolutePath(), e); - } catch (RuntimeException e) { - closeQuietly(channel); - throw e; } + downgradeIfNotEnumerable(); } /** * Opens an MPQ archive held in memory. *

- * A writable in-memory archive does not write anything back to the caller's - * array; retrieve the rebuilt image with {@link #getOutputByteArray()} - * after closing. To hold that promise the array is copied when the archive - * is opened for writing, because the rebuild writes the finished image - * through the channel and the channel would otherwise write straight into - * the caller's array whenever the new image is no larger than the old one. - * A read-only open never writes, so it wraps the array as it is. + * Nothing is written back to the caller's array; retrieve the rebuilt image + * with {@link #getOutputByteArray()} after closing. * * @param mpqArchive the archive bytes. * @param openOptions options to use when opening the archive. * @throws JMpqException if the archive is damaged or unsupported. */ public JMpqEditor(byte[] mpqArchive, MPQOpenOption... openOptions) throws JMpqException { - writeRequested = !Arrays.asList(openOptions).contains(MPQOpenOption.READ_ONLY); - canWrite = writeRequested; - legacyCompatibility = Arrays.asList(openOptions).contains(MPQOpenOption.FORCE_V0); - - SeekableByteChannel channel = null; + this.path = null; + this.memoryImage = mpqArchive; + this.forceV0 = has(openOptions, MPQOpenOption.FORCE_V0); + this.writeRequested = !has(openOptions, MPQOpenOption.READ_ONLY); + this.canWrite = writeRequested; try { - // See the constructor docs: only a writable archive needs the copy. - channel = new SeekableInMemoryByteChannel(canWrite ? mpqArchive.clone() : mpqArchive); - fc = channel; - ownsChannel = true; - readMpq(); + this.archive = MpqArchive.open(mpqArchive, openOptions()); } catch (JMpqException e) { - closeQuietly(channel); - throw new JMpqException("In-memory MPQ archive: " + e.getMessage(), e); + throw e; } catch (IOException e) { - closeQuietly(channel); throw new JMpqException("Cannot open in-memory MPQ archive", e); - } catch (RuntimeException e) { - closeQuietly(channel); - throw e; } + downgradeIfNotEnumerable(); } /** @@ -335,76 +177,40 @@ public JMpqEditor(File mpqArchive) throws IOException { this(mpqArchive.toPath(), MPQOpenOption.FORCE_V0); } - private static void closeQuietly(SeekableByteChannel channel) { - if (channel != null) { - try { - channel.close(); - } catch (IOException suppressed) { - // The original failure is what the caller needs to see. - log.debug("Ignoring failure while closing a partially opened archive.", suppressed); + private static boolean has(MPQOpenOption[] options, MPQOpenOption wanted) { + for (MPQOpenOption option : options) { + if (option == wanted) { + return true; } } + return false; } - private void readMpq() throws IOException { - headerOffset = searchHeader(); - readHeaderSize(); - readHeader(); - checkLegacyCompat(); - validateTables(); - readHashTable(); - readBlockTable(); - hashTable.setBlockTableSize(blockSize); - readListFile(); - readAttributesFile(); + private MpqOpenOptions openOptions() { + return forceV0 ? MpqOpenOptions.warcraft3() : MpqOpenOptions.defaults(); + } + + /** + * An archive that cannot enumerate itself cannot be rebuilt without losing + * the files whose names are unknown, so it is downgraded to read-only. + */ + private void downgradeIfNotEnumerable() { + // Presence of the (listfile), not whether it named anything: a freshly + // created archive has an empty one and is perfectly writable, and + // treating that as unenumerable made it impossible to add the first + // file to it. + if (canWrite && !archive.contains("(listfile)")) { + log.warn("The mpq doesn't contain a listfile. It cannot be rebuilt."); + canWrite = false; + } } /** * @return the bytes of a minimal, empty version 0 archive. - * @throws IOException if the archive image cannot be assembled. + * @throws IOException if the image cannot be assembled. */ public static byte[] createEmptyArchive() throws IOException { - final int hashEntries = 2; - final int blockEntries = 1; - final int hashTableOffset = HEADER_SIZES[0]; - final int blockTableOffset = hashTableOffset + hashEntries * 16; - - HashTable hashTable = new HashTable(hashEntries); - hashTable.setFileBlockIndex("(listfile)", HashTable.DEFAULT_LOCALE, 0); - - ByteBuffer hashTableBuffer = ByteBuffer.allocate(hashEntries * 16).order(ByteOrder.LITTLE_ENDIAN); - hashTable.writeToBuffer(hashTableBuffer); - hashTableBuffer.flip(); - new MPQEncryption(KEY_HASH_TABLE, false).processSingle(hashTableBuffer); - hashTableBuffer.flip(); - - // The block table is encrypted too. Emitting it in the clear produced an - // archive whose block table decoded to garbage: every reader saw a - // (listfile) block with a nonsense position and multi-gigabyte size. - // It went unnoticed because the failure was swallowed on open. - ByteBuffer blockTableBuffer = - ByteBuffer.allocate(blockEntries * BlockTable.ENTRY_SIZE).order(ByteOrder.LITTLE_ENDIAN); - new Block(blockTableOffset + blockEntries * BlockTable.ENTRY_SIZE, 0, 0, EXISTS) - .writeToBuffer(blockTableBuffer); - blockTableBuffer.flip(); - new MPQEncryption(KEY_BLOCK_TABLE, false).processSingle(blockTableBuffer); - blockTableBuffer.flip(); - - ByteBuffer archive = ByteBuffer - .allocate(HEADER_SIZES[0] + hashEntries * 16 + blockEntries * BlockTable.ENTRY_SIZE) - .order(ByteOrder.LITTLE_ENDIAN); - archive.putInt(ARCHIVE_HEADER_MAGIC); - archive.putInt(HEADER_SIZES[0]); - archive.putInt(archive.capacity()); - archive.putShort((short) 0); // format version 0 - archive.putShort((short) 3); // sector size shift: 4 KiB sectors - archive.putInt(hashTableOffset); - archive.putInt(blockTableOffset); - archive.putInt(hashEntries); - archive.putInt(blockEntries); - archive.put(hashTableBuffer); - archive.put(blockTableBuffer); - return archive.array(); + return MpqArchiveWriter.create(MpqWriteOptions.defaults().withPrefix(false)).toByteArray(); } /** @@ -414,59 +220,20 @@ public static byte[] createEmptyArchive() throws IOException { * @throws IOException if the file cannot be written. */ public static void createEmptyArchive(File mpqArchive) throws IOException { - File parent = mpqArchive.getParentFile(); + final File parent = mpqArchive.getParentFile(); if (parent != null) { Files.createDirectories(parent.toPath()); } Files.write(mpqArchive.toPath(), createEmptyArchive()); } - private void checkLegacyCompat() throws IOException { - if (!legacyCompatibility) { - return; - } - // limit end of archive by end of file - archiveSize = Math.min(archiveSize, fc.size() - headerOffset); - - // limit block table size by end of archive; a header whose block table - // position lies past the end of the archive yields a negative delta, - // which used to become a negative allocation size. - final long delta = archiveSize - blockPos; - if (delta > 0) { - blockSize = (int) Math.min(blockSize, delta / BlockTable.ENTRY_SIZE); - } else { - log.warn("Block table position {} lies past the archive end {}; treating the block table as empty.", - blockPos, archiveSize); - blockSize = 0; - } - } - - private void readAttributesFile() { - if (!hasFile("(attributes)")) { - return; - } - try { - attributes = new AttributesFile(extractFileAsBytes("(attributes)")); - } catch (IOException | RuntimeException e) { - // An unreadable (attributes) file is not fatal: it holds only - // optional metadata. Say so instead of swallowing it silently. - log.warn("Cannot parse this archive's (attributes) file; continuing without it.", e); - } - } - /** - * For use when the MPQ is missing a (listfile). - * Adds this custom listfile into the MPQ and uses it - * for rebuilding purposes. - * If this is not a full listfile, the end result will be missing files. + * For use when the MPQ is missing a {@code (listfile)}. Applies an external + * list of names so the archive can be enumerated and rebuilt. * - * @param externalListfilePath Path to a file containing listfile entries + * @param externalListfilePath file containing one name per line. */ public void setExternalListfile(File externalListfilePath) { - // Gate on what the caller asked for, not on the effective mode: an - // archive whose own list file is missing or unreadable has already been - // downgraded to read-only, and that is exactly the case this method - // exists to repair. if (!writeRequested) { log.warn("The mpq was opened as readonly, setting an external listfile will have no effect."); return; @@ -476,499 +243,146 @@ public void setExternalListfile(File externalListfilePath) { externalListfilePath.getAbsolutePath()); return; } - // Applied all-or-nothing. A malformed replacement must leave the - // archive exactly as it was: an archive that already had a usable list - // file would otherwise be downgraded to read-only, and close() then - // silently discards whatever the caller had already queued. - final Listfile replacement; try { - replacement = new Listfile(Files.readAllBytes(externalListfilePath.toPath())); - } catch (IOException | RuntimeException e) { - log.warn("Could not read external listfile: {}", externalListfilePath.getAbsolutePath(), e); - return; - } - - final Listfile previousListFile = listFile; - final boolean previousCanWrite = canWrite; - listFile = replacement; - // Restore writability before checking completeness, so entries that do - // not resolve are pruned as they are for a built-in list file. - canWrite = true; - try { - checkListfileEntries(); - log.debug("Applied external listfile with {} entries; archive is writable.", listFile.size()); - } catch (IOException | RuntimeException e) { - listFile = previousListFile; - canWrite = previousCanWrite; - log.warn("Could not apply external listfile: {}", externalListfilePath.getAbsolutePath(), e); - } - } - - /** - * Reads the internal {@code (listfile)} and applies it as this archive's - * list file. - *

- * An archive without a list file cannot be rebuilt without losing the files - * whose names are unknown, so it is downgraded to read-only. Supply the - * names with {@link #setExternalListfile(File)} to make it writable again. - */ - private void readListFile() { - if (hasFile("(listfile)")) { - try { - listFile = new Listfile(extractFileAsBytes("(listfile)")); - checkListfileEntries(); - } catch (IOException | RuntimeException e) { - log.warn("Extracting the mpq's listfile failed. It cannot be rebuilt.", e); - canWrite = false; - } - } else { - log.warn("The mpq doesn't contain a listfile. It cannot be rebuilt."); - canWrite = false; - } - } - - /** - * Performs verification to see if we know all the blocks of this file. - * Prints warnings if we don't know all blocks. - * - * @throws JMpqException If retrieving valid blocks fails - */ - private void checkListfileEntries() throws JMpqException { - int hiddenFiles = (hasFile("(attributes)") ? 2 : 1) + (hasFile("(signature)") ? 1 : 0); - if (canWrite) { - checkListfileCompleteness(hiddenFiles); - } - } - - /** - * Checks listfile for completeness against block table - * - * @param hiddenFiles Num. hidden files - * @throws JMpqException If retrieving valid blocks fails - */ - private void checkListfileCompleteness(int hiddenFiles) throws JMpqException { - if (listFile.size() <= blockTable.getAllValidBlocks().size() - hiddenFiles) { - log.warn("mpq's listfile is incomplete. Blocks without listfile entry will be discarded"); - } - for (String fileName : listFile.getFiles()) { - if (!hasFile(fileName)) { - log.warn("listfile entry does not exist in archive and will be discarded: {}", fileName); - } - } - listFile.getFileMap().entrySet().removeIf(file -> !hasFile(file.getValue())); - - for (Collection collision : listFile.findKeyCollisions()) { - log.warn("These listfile entries share one MPQ file key and cannot coexist: {}", collision); - } - } - - private void readBlockTable() throws IOException { - ByteBuffer blockBuffer = - ByteBuffer.allocate(blockSize * BlockTable.ENTRY_SIZE).order(ByteOrder.LITTLE_ENDIAN); - fc.position(headerOffset + blockPos); - readFully(blockBuffer, fc); - blockBuffer.rewind(); - blockTable = new BlockTable(blockBuffer); - } - - private void readHashTable() throws IOException { - // read hash table - ByteBuffer hashBuffer = ByteBuffer.allocate(hashSize * 16); - fc.position(headerOffset + hashPos); - readFully(hashBuffer, fc); - hashBuffer.rewind(); - - // decrypt hash table - final MPQEncryption decrypt = new MPQEncryption(KEY_HASH_TABLE, true); - decrypt.processSingle(hashBuffer); - hashBuffer.rewind(); - - // create hash table - hashTable = new HashTable(hashSize); - hashTable.readFromBuffer(hashBuffer); - } - - private void readHeaderSize() throws IOException { - ByteBuffer probe = ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN); - fc.position(headerOffset + 4); - readFully(probe, fc); - headerSize = probe.getInt(0); - if (legacyCompatibility) { - // Warcraft III ignores this field for version 0 archives, and map - // protectors fill it with garbage. - headerSize = HEADER_SIZES[0]; - } else if (headerSize < HEADER_SIZES[0] || headerSize > HEADER_SIZES[HEADER_SIZES.length - 1]) { - throw new JMpqException("Bad header size " + headerSize + " at offset " + headerOffset - + "; expected between " + HEADER_SIZES[0] + " and " + HEADER_SIZES[HEADER_SIZES.length - 1] - + ". Retry with MPQOpenOption.FORCE_V0 for protected Warcraft III maps."); - } - } - - /** - * Searches the file for the MPQ archive header. - * - * @return the file position at which the MPQ archive starts. - * @throws IOException if an error occurs while searching. - * @throws JMpqException if file does not contain a MPQ archive. - */ - private long searchHeader() throws IOException { - ByteBuffer probe = ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN); - - final long fileSize = fc.size(); - for (long filePos = 0; filePos + probe.capacity() < fileSize; filePos += HEADER_ALIGNMENT) { - probe.rewind(); - fc.position(filePos); - readFully(probe, fc); - - final int sample = probe.getInt(0); - if (sample == ARCHIVE_HEADER_MAGIC) { - if (legacyCompatibility && !isPlausibleV0Header(filePos, fileSize)) { - // A decoy header planted by a map protector. Keep scanning - // instead of committing to the first magic value found. - log.debug("Ignoring implausible MPQ header at {}", filePos); - continue; + final Listfile supplied = new Listfile(Files.readAllBytes(externalListfilePath.toPath())); + int resolved = 0; + for (String name : supplied.getFiles()) { + if (archive.contains(name)) { + externalNames.add(name); + resolved++; + } else { + log.debug("External listfile names <{}>, not held by the archive.", name); } - return filePos; } - - if (sample == USER_DATA_HEADER_MAGIC && !legacyCompatibility) { - // MPQ user data header redirecting to the real MPQ header. - // Ignored in legacy compatibility mode, because Warcraft III - // ignores it too. - probe.rewind(); - fc.position(filePos + 8); - readFully(probe, fc); - - final long redirected = filePos + (probe.getInt(0) & 0xFFFFFFFFL); - // The old code mutated the loop variable and then re-aligned it - // with 'filePos &= -0x200'. A redirect offset below 0x200 left - // filePos unchanged, so the loop never advanced and the open - // hung forever. - if (redirected + probe.capacity() < fileSize) { - probe.rewind(); - fc.position(redirected); - readFully(probe, fc); - if (probe.getInt(0) == ARCHIVE_HEADER_MAGIC) { - return redirected; - } - } - log.debug("User data header at {} does not point at an archive header; continuing scan.", filePos); + // A list file that resolves nothing leaves the archive as it was, + // rather than claiming it became writable. + if (resolved > 0) { + canWrite = true; } + log.debug("Applied external listfile: {} of {} names resolved.", resolved, supplied.size()); + } catch (IOException | RuntimeException e) { + log.warn("Could not apply external listfile: {}", externalListfilePath.getAbsolutePath(), e); } - - throw new JMpqException("No MPQ archive in file."); - } - - /** - * Cheap plausibility check on a candidate version 0 header. - *

- * Mirrors StormLib's {@code ERROR_FAKE_MPQ_HEADER} test: a header whose - * table positions fall outside the file cannot be the real one. - */ - private boolean isPlausibleV0Header(long filePos, long fileSize) throws IOException { - final ByteBuffer header = ByteBuffer.allocate(HEADER_SIZES[0]).order(ByteOrder.LITTLE_ENDIAN); - fc.position(filePos); - try { - readFully(header, fc); - } catch (EOFException e) { - return false; - } - - final int sectorShift = header.getShort(14) & 0xFFFF; - final long hashTablePos = header.getInt(16) & 0xFFFFFFFFL; - final long blockTablePos = header.getInt(20) & 0xFFFFFFFFL; - final int hashTableSize = header.getInt(24) & HashTable.BLOCK_INDEX_MASK; - - return hashTablePos > 0 - && blockTablePos > 0 - && hashTableSize > 0 - && (sectorShift & 0xFF) <= MAX_SECTOR_SIZE_SHIFT - && filePos + hashTablePos < fileSize - && filePos + blockTablePos < fileSize; - } - - /** - * Read the MPQ archive header from the header chunk. - */ - private void readHeader() throws IOException { - // The first eight bytes (magic and header size) are already consumed. - final int bodySize = headerSize - 8; - ByteBuffer buffer = ByteBuffer.allocate(bodySize).order(ByteOrder.LITTLE_ENDIAN); - fc.position(headerOffset + 8); - readFully(buffer, fc); - buffer.rewind(); - - archiveSize = buffer.getInt() & 0xFFFFFFFFL; - formatVersion = buffer.getShort() & 0xFFFF; - if (legacyCompatibility) { - // force version 0 interpretation - formatVersion = 0; - } - - // StormLib: "Only low byte of sector size is really used". - sectorSizeShift = buffer.getShort() & 0xFF; - if (sectorSizeShift > MAX_SECTOR_SIZE_SHIFT) { - throw new JMpqException("Sector size shift " + sectorSizeShift + " is out of range."); - } - discBlockSize = 512 << sectorSizeShift; - - hashPos = buffer.getInt() & 0xFFFFFFFFL; - blockPos = buffer.getInt() & 0xFFFFFFFFL; - hashSize = buffer.getInt() & HashTable.BLOCK_INDEX_MASK; - blockSize = buffer.getInt(); - - // version 1 extension - if (formatVersion >= 1 && buffer.remaining() >= 12) { - // TODO add high block table support - buffer.getLong(); - - // high 16 bits of file pos - hashPos |= (buffer.getShort() & 0xFFFFL) << 32; - blockPos |= (buffer.getShort() & 0xFFFFL) << 32; - } - - // version 2 extension - if (formatVersion >= 2 && buffer.remaining() >= 24) { - // 64 bit archive size - archiveSize = buffer.getLong(); - - // TODO add support for BET and HET tables - buffer.getLong(); - buffer.getLong(); - } - - // version 3 adds compressed table sizes and MD5 digests, both of which - // are read in Phase 2. Nothing here depends on them. - } - - /** - * Validates the header's table descriptions against the actual file, and - * clamps what StormLib clamps. - *

- * Every one of these numbers comes from an untrusted file and used to flow - * straight into an allocation or a channel position. - */ - private void validateTables() throws IOException { - final long fileSize = fc.size(); - - // StormLib notes that dwArchiveSize "is ignored by Storm.dll and can - // contain garbage value", so clamp it for every archive rather than - // only in legacy mode. It feeds the rebuild's initial buffer size, and - // a header claiming 4 GiB in a 200 byte file would otherwise ask for a - // 4 GiB allocation the moment the archive is closed. - final long declaredArchiveSize = archiveSize; - archiveSize = Math.min(archiveSize, fileSize - headerOffset); - if (archiveSize != declaredArchiveSize) { - log.debug("Header claims a {} byte archive but only {} bytes follow the header; using the latter.", - declaredArchiveSize, archiveSize); - } - - if (hashSize <= 0) { - throw new JMpqException("Archive declares " + hashSize + " hash table entries."); - } - if (hashSize > HASH_TABLE_SIZE_MAX) { - throw new JMpqException("Archive declares " + hashSize + " hash table entries, more than the " - + HASH_TABLE_SIZE_MAX + " StormLib accepts."); - } - if (blockSize < 0) { - throw new JMpqException("Archive declares " + blockSize + " block table entries."); - } - - final long hashTableEnd = headerOffset + hashPos + (long) hashSize * 16; - if (headerOffset + hashPos < 0 || hashTableEnd > fileSize) { - throw new JMpqException("Hash table at " + (headerOffset + hashPos) + " spanning " + hashSize - + " entries runs past the end of the " + fileSize + " byte file."); - } - - final long blockTableStart = headerOffset + blockPos; - if (blockTableStart < 0 || blockTableStart > fileSize) { - throw new JMpqException("Block table position " + blockTableStart - + " lies outside the " + fileSize + " byte file."); - } - final long blockTableEnd = blockTableStart + (long) blockSize * BlockTable.ENTRY_SIZE; - if (blockTableEnd > fileSize) { - // StormLib does exactly this: real archives in the wild (the audit - // cites EWIX_v8_7.w3x) declare a block table far larger than the - // file, and rejecting them would be stricter than the game. - final int clamped = (int) ((fileSize - blockTableStart) / BlockTable.ENTRY_SIZE); - log.warn("Archive declares {} block table entries but only {} fit in the file; using {}.", - blockSize, clamped, clamped); - blockSize = clamped; - } - } - - /** - * Write header. - * - * @param buffer the buffer, positioned after the archive magic. - */ - private void writeHeader(ByteBuffer buffer) { - buffer.putInt(newHeaderSize); - putUnsignedInt(buffer, newArchiveSize, "Archive size"); - buffer.putShort((short) newFormatVersion); - buffer.putShort((short) newSectorSizeShift); - putUnsignedInt(buffer, newHashPos, "Hash table position"); - putUnsignedInt(buffer, newBlockPos, "Block table position"); - buffer.putInt(newHashSize); - buffer.putInt(newBlockSize); - - if (newFormatVersion >= 1) { - // Hi-block table position (unused) and the hi-words of the hash and - // block table positions. - buffer.putLong(0); - buffer.putShort((short) (newHashPos >>> 32)); - buffer.putShort((short) (newBlockPos >>> 32)); - } - } - - private static void putUnsignedInt(ByteBuffer buffer, long value, String fieldName) { - if (value < 0 || value > V0_MAX_ARCHIVE_SIZE) { - throw new IllegalArgumentException(fieldName + " exceeds unsigned 32-bit range: " + value); - } - buffer.putInt((int) value); - } - - /** - * Sizes the rebuilt hash and block tables. - *

- * The hash table is twice the next power of two above the file count, which - * keeps its load factor at or below 50% so lookups stay short. - */ - private void calcNewTableSize(int fileCount) throws JMpqException { - int current = 2; - final int target = fileCount + 2; - while (current < target) { - current *= 2; - } - final long hashCapacity = (long) current * 2; - if (hashCapacity > HASH_TABLE_SIZE_MAX) { - throw new JMpqException("Cannot fit " + fileCount + " files: the hash table would need " - + hashCapacity + " buckets, above the " + HASH_TABLE_SIZE_MAX + " maximum."); - } - newHashSize = (int) hashCapacity; - newBlockSize = fileCount + 2; } /** * Extracts every file this archive can name into {@code dest}. * * @param dest destination directory. - * @throws JMpqException if the destination is unusable or extraction fails. + * @throws JMpqException if the destination is unusable. */ public void extractAllFiles(File dest) throws JMpqException { if (!dest.isDirectory()) { throw new JMpqException("Destination location isn't a directory: " + dest); } final Path root = dest.toPath().toAbsolutePath().normalize(); - - if (hasFile("(listfile)")) { - final List names = new ArrayList<>(listFile.getFiles()); - names.add("(listfile)"); - if (hasFile("(attributes)")) { - names.add("(attributes)"); - } - for (String name : names) { - if (!hasFile(name)) { - continue; - } - log.debug("extracting: {}", name); - try { - // Resolved inside the guard: an entry that would escape the - // destination is refused, and refusing it must cost the - // caller only that entry. Archives carrying a traversal - // name are exactly the ones where the rest still matters. - final Path target = resolveExtractionTarget(root, name); - Files.createDirectories(target.getParent()); - getMpqFile(name).extractToPath(target); - } catch (IOException | RuntimeException e) { - // Extracting everything is best effort by definition: one - // damaged file must not cost the caller the rest of the - // archive. RuntimeException is included because the codecs - // are C ports that signal bad data unchecked. - log.warn("File possibly corrupted and could not be extracted: {}", name, e); - } + final List names = new ArrayList<>(archive.names()); + for (String internal : List.of("(listfile)", "(attributes)", "(signature)")) { + if (archive.contains(internal) && !names.contains(internal)) { + names.add(internal); } - return; } - // No list file: fall back to dumping blocks by index. - try { - int i = 0; - for (Block b : blockTable.getAllValidBlocks()) { - if (b.hasFlag(ENCRYPTED)) { - // Without a name there is no key, so the content is not - // recoverable. - continue; + for (String name : names) { + try { + // Archive contents are untrusted: an entry naming "..\evil" + // must not write outside the directory the caller nominated. + final Path target = root.resolve(name.replace('\\', '/')).normalize(); + if (!target.startsWith(root)) { + throw new JMpqException("Refusing to extract <" + name + + ">: it escapes the destination directory."); } - readBlock(b, "").extractToPath(root.resolve(Integer.toString(i))); - i++; + Files.createDirectories(target.getParent()); + Files.write(target, archive.read(name)); + } catch (IOException | RuntimeException e) { + // Best effort by definition: one damaged file must not cost the + // caller the rest of the archive. + log.warn("File possibly corrupted and could not be extracted: {}", name, e); } - } catch (IOException e) { - throw new JMpqException("Cannot extract this archive's blocks.", e); + } + + if (names.isEmpty()) { + extractUnnamedBlocks(root); } } /** - * Maps an archive-internal path to a destination path, refusing anything - * that would escape the destination directory. - *

- * Archive contents are untrusted: an entry such as {@code ..\..\evil} would - * otherwise write outside the directory the caller nominated. + * Fallback for an archive with no list file: dump each readable block + * under its block index, since there is no name to give it. */ - private Path resolveExtractionTarget(Path root, String name) throws JMpqException { - final String relative = name.replace('\\', '/'); - final Path target = root.resolve(relative).normalize(); - if (!target.startsWith(root)) { - throw new JMpqException("Refusing to extract <" + name + ">: it escapes the destination directory."); + private void extractUnnamedBlocks(Path root) { + int index = 0; + for (MpqFileEntry entry : archive.entries()) { + if (entry.isEncrypted() && entry.name().isEmpty()) { + // Without a name there is no key, so the content is not + // recoverable. + index++; + continue; + } + try { + Files.write(root.resolve(Integer.toString(index)), archive.read(entry)); + } catch (IOException | RuntimeException e) { + log.warn("Block {} could not be extracted.", index, e); + } + index++; } - return target; } /** - * @return the number of live entries in the block table. - * @throws JMpqException if the block table cannot be read. + * @return the number of live block table entries. */ - public int getTotalFileCount() throws JMpqException { - return blockTable.getAllValidBlocks().size(); + public int getTotalFileCount() { + return archive.blockCount(); } /** - * Extracts the specified file out of the mpq to the target location. - * * @param name name of the file - * @param dest destination to that the files content is written - * @throws JMpqException if file is not found or access errors occur + * @param dest destination to which the file's content is written + * @throws JMpqException if the file is not found or cannot be decoded */ public void extractFile(String name, File dest) throws JMpqException { try { - getMpqFile(name).extractToFile(dest); + Files.write(dest.toPath(), archive.read(name)); } catch (IOException e) { - throw new JMpqException("Cannot extract <" + name + "> to " + dest, e); + throw wrap("Cannot extract <" + name + "> to " + dest, e); } } /** - * Extracts the specified file out of the mpq. - * * @param name name of the file * @return the file's content - * @throws JMpqException if file is not found or access errors occur + * @throws JMpqException if the file is not found or cannot be decoded */ public byte[] extractFileAsBytes(String name) throws JMpqException { try { - return getMpqFile(name).extractToBytes(); + return archive.read(name); } catch (IOException e) { - throw new JMpqException("Cannot extract <" + name + ">", e); + throw wrap("Cannot extract <" + name + ">", e); } } /** * @param name name of the file * @return the file's content decoded as UTF-8 - * @throws JMpqException if file is not found or access errors occur + * @throws JMpqException if the file is not found or cannot be decoded */ public String extractFileAsString(String name) throws JMpqException { - return new String(extractFileAsBytes(name), java.nio.charset.StandardCharsets.UTF_8); + return new String(extractFileAsBytes(name), StandardCharsets.UTF_8); + } + + /** + * Extracts a file to a stream. The stream is flushed but not closed. + * + * @param name name of the file + * @param dest destination stream + * @throws JMpqException if the file is not found or cannot be decoded + */ + public void extractFile(String name, OutputStream dest) throws JMpqException { + try { + archive.readTo(name, dest); + } catch (IOException e) { + throw wrap("Cannot extract <" + name + ">", e); + } } /** @@ -976,43 +390,36 @@ public String extractFileAsString(String name) throws JMpqException { * @return true if this archive holds the named file */ public boolean hasFile(String name) { - // The hash table can answer this without throwing; the old - // implementation called getBlockIndexOfFile and caught the exception. - return hashTable != null && hashTable.hasFile(name); + return archive.contains(name); } /** * @param name the file path * @param locale preferred locale - * @return true if this archive holds the named file in any locale + * @return true if this archive holds the named file */ public boolean hasFile(String name, short locale) { - return hashTable != null && hashTable.hasFile(name, locale); + return archive.contains(name, locale); } /** - * @return the names this archive's list file knows about. + * @return the names this archive's list file knows about, plus anything + * inserted since opening. */ public List getFileNames() { - return new ArrayList<>(listFile.getFiles()); - } - - /** - * Extracts the specified file out of the mpq and writes it to the target - * outputstream. - *

- * The stream is flushed but not closed; it belongs to the caller. - * - * @param name name of the file - * @param dest the outputstream where the file's content is written - * @throws JMpqException if file is not found or access errors occur - */ - public void extractFile(String name, OutputStream dest) throws JMpqException { - try { - getMpqFile(name).extractToOutputStream(dest); - } catch (IOException e) { - throw new JMpqException("Cannot extract <" + name + ">", e); + final List names = new ArrayList<>(archive.names()); + for (String name : externalNames) { + if (names.stream().noneMatch(known -> sameName(known, name))) { + names.add(name); + } } + for (Insert insert : inserts.values()) { + if (names.stream().noneMatch(known -> sameName(known, insert.name()))) { + names.add(insert.name()); + } + } + names.removeIf(name -> deleted.stream().anyMatch(gone -> sameName(gone, name))); + return names; } /** @@ -1031,8 +438,9 @@ public MpqFile getMpqFile(String name) throws IOException { * @throws IOException if the file is not present or cannot be read */ public MpqFile getMpqFile(String name, short locale) throws IOException { - final int pos = hashTable.getFileBlockIndex(name, locale); - return readBlock(blockTable.getBlockAtPos(pos), name); + final MpqFileEntry entry = archive.entry(name, locale) + .orElseThrow(() -> new JMpqException("File Not Found <" + name + ">.")); + return legacyFile(entry, name); } /** @@ -1040,30 +448,13 @@ public MpqFile getMpqFile(String name, short locale) throws IOException { * @return a handle on that block's raw data * @throws IOException if the block cannot be read */ - public MpqFile getMpqFileByBlock(BlockTable.Block block) throws IOException { - if (block.hasFlag(ENCRYPTED)) { + public MpqFile getMpqFileByBlock(Block block) throws IOException { + if (block.hasFlag(MpqFile.ENCRYPTED)) { throw new JMpqException("Cannot access an encrypted block without knowing its file name."); } - return readBlock(block, ""); - } - - private MpqFile readBlock(Block block, String name) throws IOException { - final int compressedSize = block.getCompressedSize(); - if (compressedSize < 0) { - throw new JMpqException("Block for <" + name + "> declares a negative size " + compressedSize + "."); - } - final long start = headerOffset + block.getFilePosition(); - if (start < 0 || start + compressedSize > fc.size()) { - throw new JMpqException("Block for <" + name + "> spans [" + start + ", " + (start + compressedSize) - + "), which is outside the " + fc.size() + " byte file."); - } - - ByteBuffer buffer = ByteBuffer.allocate(compressedSize).order(ByteOrder.LITTLE_ENDIAN); - fc.position(start); - readFully(buffer, fc); - buffer.rewind(); - - return new MpqFile(buffer, block, discBlockSize, name, formatVersion); + final MpqFileEntry entry = new MpqFileEntry("", (short) 0, block.getFlags(), + block.getFilePosition(), block.getCompressedSize(), block.getNormalSize(), 0); + return legacyFile(entry, ""); } /** @@ -1072,109 +463,133 @@ private MpqFile readBlock(Block block, String name) throws IOException { * @throws IOException if the block table cannot be read */ public List getMpqFilesByBlockTable() throws IOException { - List mpqFiles = new ArrayList<>(); - for (Block block : blockTable.getAllValidBlocks()) { + final List files = new ArrayList<>(); + for (MpqFileEntry entry : archive.entries()) { + if (entry.isEncrypted()) { + continue; + } try { - mpqFiles.add(getMpqFileByBlock(block)); + files.add(legacyFile(entry, entry.name())); } catch (IOException e) { - log.debug("Skipping unreadable block {}", block, e); + log.debug("Skipping unreadable block {}", entry.blockIndex(), e); } } - return mpqFiles; + return files; + } + + private MpqFile legacyFile(MpqFileEntry entry, String name) throws IOException { + final ByteBuffer raw = ByteBuffer.wrap(archive.rawBytes(entry)).order(ByteOrder.LITTLE_ENDIAN); + final Block block = new Block(entry.filePosition(), entry.compressedSize(), + entry.normalSize(), entry.flags()); + return new MpqFile(raw, block, archive.header().sectorSize(), name, + archive.header().formatVersion()); } /** - * Deletes the specified file from the mpq once you rebuild the mpq. + * Deletes the specified file from the mpq once the editor is closed. * * @param name of the file inside the mpq */ public void deleteFile(String name) { - if (!canWrite) { - throw new NonWritableChannelException(); - } - listFile.removeFile(name); - pendingFiles.remove(MpqNames.canonical(name)); + requireWritable(); + inserts.remove(MpqNames.canonical(name)); + externalNames.removeIf(known -> sameName(known, name)); + deleted.add(name); } /** - * Inserts the specified byte array into the mpq once you close the editor. + * Inserts the specified byte array into the mpq once the editor is closed. *

- * The array is copied, so the caller may reuse or modify it afterwards. + * The array is copied, so the caller may reuse it. * * @param name of the file inside the mpq * @param input the input byte array * @param override whether to override an existing file with the same name - * @throws IllegalArgumentException when the archive already has the file - * and {@code override} is false */ public void insertByteArray(String name, byte[] input, boolean override) { - requireInsertable(name, override); - listFile.addFile(name); - pendingFiles.put(MpqNames.canonical(name), PendingFile.of(name, input)); + requireWritable(); + requireAbsent(name, override); + // Copy on insert: the caller may reuse its array afterwards. + inserts.put(MpqNames.canonical(name), new Insert(name, input.clone(), null)); + deleted.removeIf(gone -> sameName(gone, name)); } /** - * Inserts the specified byte array into the mpq once you close the editor. - * * @param name of the file inside the mpq * @param input the input byte array - * @throws IllegalArgumentException when the archive already has the file */ - public void insertByteArray(String name, byte[] input) throws NonWritableChannelException, IllegalArgumentException { + public void insertByteArray(String name, byte[] input) + throws NonWritableChannelException, IllegalArgumentException { insertByteArray(name, input, false); } /** - * Inserts the specified file into the mpq once you close the editor. - *

- * The file is read at rebuild time, so it must still exist and hold the - * intended content when {@link #close()} runs. + * Inserts the specified file into the mpq once the editor is closed. The + * file is read at close time, so it must still exist then. * * @param name of the file inside the mpq * @param file the file - * @throws IOException if the file cannot be used - * @throws IllegalArgumentException when the archive already has the file */ public void insertFile(String name, File file) throws IOException, IllegalArgumentException { insertFile(name, file, false); } /** - * Inserts the specified file into the mpq once you close the editor. - * * @param name of the file inside the mpq * @param file the file * @param override whether to override an existing file with the same name - * @throws IOException if the file cannot be used */ public void insertFile(String name, File file, boolean override) throws IOException { - requireInsertable(name, override); + requireWritable(); + requireAbsent(name, override); log.debug("insert file: {}", name); - listFile.addFile(name); - pendingFiles.put(MpqNames.canonical(name), PendingFile.of(name, file.toPath())); + // Stored as a path and read at close time, as 1.x did. + inserts.put(MpqNames.canonical(name), new Insert(name, null, file.toPath())); + deleted.removeIf(gone -> sameName(gone, name)); } - private void requireInsertable(String name, boolean override) { + private void requireWritable() { if (!canWrite) { throw new NonWritableChannelException(); } - if (!override && listFile.containsFile(name)) { + } + + private void requireAbsent(String name, boolean override) { + if (override || deleted.stream().anyMatch(gone -> sameName(gone, name))) { + return; + } + if (inserts.containsKey(MpqNames.canonical(name)) || archive.contains(name)) { throw new IllegalArgumentException("Archive already contains file with name: " + name); } } + private static boolean sameName(String a, String b) { + return MpqNames.canonical(a).equals(MpqNames.canonical(b)); + } + + private MpqWriteOptions writeOptions(RecompressOptions recompress, boolean buildListfile) { + MpqWriteOptions options = MpqWriteOptions.defaults() + .withFormatVersion(Math.min(archive.header().formatVersion(), MpqWriteOptions.MAX_WRITABLE_VERSION)) + .withSectorSizeShift(recompress.recompress + ? Math.min(recompress.newSectorSizeShift, MpqHeader.MAX_SECTOR_SIZE_SHIFT) + : archive.header().sectorSizeShift()) + .withRecompression(recompress) + .withListfile(buildListfile) + .withPrefix(keepHeaderOffset); + return options; + } + /** * Closes the archive without rebuilding it. * - * @throws IOException if the channel cannot be closed + * @throws IOException if the archive cannot be released * @deprecated call {@link #close()}; it does not rebuild a read-only * archive either. */ @Deprecated public void closeReadOnly() throws IOException { - if (ownsChannel) { - fc.close(); - } + archive.close(); + closed = true; } @Override @@ -1194,292 +609,77 @@ public void close(boolean buildListfile, boolean buildAttributes, boolean recomp /** * Rebuilds the archive, if it is writable, and releases it. - *

- * The rebuild is assembled in memory and then written over the archive in a - * single pass. Nothing is staged on disk. * * @param buildListfile whether to add a {@code (listfile)} to this mpq * @param buildAttributes whether to add an {@code (attributes)} file. Not - * yet implemented; requesting it logs a warning - * rather than silently doing nothing. + * implemented; requesting it logs a warning rather + * than silently doing nothing. * @param options recompression settings * @throws IOException if the rebuild fails */ - public void close(boolean buildListfile, boolean buildAttributes, RecompressOptions options) throws IOException { - if (!canWrite || !fc.isOpen()) { - if (ownsChannel) { - fc.close(); - } + public void close(boolean buildListfile, boolean buildAttributes, RecompressOptions options) + throws IOException { + if (closed) { + return; + } + if (!canWrite) { + archive.close(); + closed = true; log.debug("Closed archive without rebuilding."); return; } + if (buildAttributes) { + log.warn("(attributes) generation is not implemented; the rebuilt archive will not have one."); + } try { - rebuild(buildListfile, buildAttributes, options); + // The image has to be built while the archive is still open, because + // file content is read lazily, and written once it is closed, + // because a mapped file cannot be replaced on Windows. + outputByteArray = build(options, buildListfile); } finally { - if (fc.isOpen() && ownsChannel) { - fc.close(); - } - } - } - - private void rebuild(boolean buildListfile, boolean buildAttributes, RecompressOptions options) throws IOException { - final long startedAt = System.nanoTime(); - log.debug("Building mpq"); - - if (buildAttributes && attributes == null) { - log.warn("(attributes) generation is not implemented yet; the rebuilt archive will not have one."); - } else if (attributes != null) { - log.warn("This archive has an (attributes) file, which the rebuild does not preserve yet."); - } - - final long base = keepHeaderOffset ? headerOffset : 0; - newFormatVersion = formatVersion; - newHeaderSize = HEADER_SIZES[Math.min(newFormatVersion, HEADER_SIZES.length - 1)]; - newSectorSizeShift = options.recompress - ? Math.min(options.newSectorSizeShift, MAX_SECTOR_SIZE_SHIFT) - : sectorSizeShift; - newDiscBlockSize = options.recompress ? 512 << newSectorSizeShift : discBlockSize; - - final GrowingBuffer out = new GrowingBuffer(estimateImageSize()); - - // Preserve whatever sits in front of the archive, if asked to. - if (keepHeaderOffset && headerOffset > 0) { - final ByteBuffer prefix = ByteBuffer.allocate((int) headerOffset).order(ByteOrder.LITTLE_ENDIAN); - fc.position(0); - readFully(prefix, fc); - prefix.rewind(); - out.put(prefix); - } - - // Reserve the header; it is filled in once the table positions are - // known. The old code sized this region from the *old* header size, - // which corrupted the archive whenever the version changed. - out.putInt(ARCHIVE_HEADER_MAGIC); - out.skip(newHeaderSize - 4); - - final List newBlocks = new ArrayList<>(); - final List newFiles = new ArrayList<>(); - final List existingFiles = sortedExistingFiles(); - long currentPos = base + newHeaderSize; - - // Files with a pending replacement are written from the pending data, - // not copied from the archive. - existingFiles.removeIf(name -> pendingFiles.containsKey(MpqNames.canonical(name))); - - currentPos = copyExistingFiles(out, existingFiles, newFiles, newBlocks, currentPos, base, options); - currentPos = writePendingFiles(out, newFiles, newBlocks, currentPos, base, options); - - // Written even when empty. Skipping it for an archive with no known - // names dropped the (listfile) altogether, and an archive without one - // is downgraded to read-only the next time it is opened, so a single - // rebuild used to make such an archive permanently unrebuildable. - if (buildListfile) { - currentPos = writeListfile(out, newFiles, newBlocks, currentPos, base, options); - } - - calcNewTableSize(newFiles.size()); - newBlockSize = Math.max(newBlockSize, newBlocks.size()); - - newHashPos = currentPos - base; - newBlockPos = newHashPos + (long) newHashSize * 16; - - writeHashTable(out, newFiles); - writeBlockTable(out, newBlocks); - currentPos += (long) newHashSize * 16 + (long) newBlockSize * BlockTable.ENTRY_SIZE; - - // The archive spans from its header to the end of the block table. - // The old code added one spurious byte here. - newArchiveSize = currentPos - base; - if (newFormatVersion == 0 && newArchiveSize > V0_MAX_ARCHIVE_SIZE) { - throw new JMpqException("Rebuilt version 0 archive is " + newArchiveSize - + " bytes, beyond the unsigned 32-bit header field."); + archive.close(); + closed = true; } - final ByteBuffer header = ByteBuffer.allocate(newHeaderSize - 4).order(ByteOrder.LITTLE_ENDIAN); - writeHeader(header); - out.putAt((int) base + 4, Arrays.copyOf(header.array(), header.position())); - - outputByteArray = out.toByteArray(); - - fc.position(0); - out.writeTo(fc); - fc.truncate(fc.position()); - - log.debug("Rebuild complete: {} bytes in {} ms.", outputByteArray.length, - (System.nanoTime() - startedAt) / 1_000_000); - } - - /** - * Rough starting size for the rebuild buffer, to avoid a long chain of - * doublings. Being wrong is harmless; the buffer grows. - */ - private int estimateImageSize() { - long estimate = Math.max(archiveSize, 0) + (keepHeaderOffset ? headerOffset : 0); - for (PendingFile pending : pendingFiles.values()) { - estimate += pending.data != null ? pending.data.length : 0; - } - return (int) Math.min(estimate + 4096, Integer.MAX_VALUE - 8); - } - - /** - * Existing file names in block table order. - *

- * Preserving the source order keeps rebuilt archives close to their input, - * which makes diffs meaningful. Names whose block cannot be resolved sort - * last. - */ - private List sortedExistingFiles() { - final Map order = new LinkedHashMap<>(); - for (String name : listFile.getFiles()) { - order.put(name, blockIndexOrMax(name)); + if (path != null) { + Files.write(path, outputByteArray); + } else { + memoryImage = outputByteArray; } - final List sorted = new ArrayList<>(order.keySet()); - sorted.sort(java.util.Comparator.comparingInt(order::get)); - return sorted; } /** - * @return the file's block index, or {@link Integer#MAX_VALUE} if it has - * none. Uses a lookup rather than catching an exception, which is - * what the old comparator did on every comparison. + * Builds the rebuilt image: whatever the archive could name, plus anything + * an external list file named, minus deletions, with insertions on top. */ - private int blockIndexOrMax(String name) { - if (!hashTable.hasFile(name)) { - return Integer.MAX_VALUE; - } - try { - return hashTable.getBlockIndexOfFile(name); - } catch (IOException e) { - return Integer.MAX_VALUE; - } - } + private byte[] build(RecompressOptions options, boolean buildListfile) throws IOException { + final MpqArchiveWriter writer = + MpqArchiveWriter.from(archive, writeOptions(options, buildListfile)); - private long copyExistingFiles(GrowingBuffer out, List existingFiles, List newFiles, - List newBlocks, long currentPos, long base, RecompressOptions options) - throws IOException { - // A file can only be copied with its stored bytes intact if the target - // archive keeps the same sector geometry: a sector offset table is - // expressed in the archive's sector size, and an archive has exactly one - // of those. Recompressing into a different sector size therefore has to - // re-encode everything, including the .wav files that are otherwise left - // alone. Copying them regardless is what corrupted them before: the - // rebuilt header advertised the new sector size while their offset - // tables still described the old one, so they could no longer be read. - final boolean canCopyVerbatim = newDiscBlockSize == discBlockSize; - - for (String existingName : existingFiles) { - final boolean skipRecompression = - canCopyVerbatim && existingName.toLowerCase(java.util.Locale.ROOT).endsWith(".wav"); - if (options.recompress && !skipRecompression) { - // Recompressing means decoding and re-encoding, so route the - // file through the pending-file path instead of copying it. - pendingFiles.put(MpqNames.canonical(existingName), - PendingFile.of(existingName, extractFileAsBytes(existingName))); - continue; + // Files the archive holds but could not name itself, recovered from an + // external list file. + for (String name : externalNames) { + if (!writer.contains(name) && archive.contains(name)) { + writer.put(name, archive.read(name)); } - - final MpqFile file = getMpqFile(existingName); - final Block newBlock = new Block(currentPos - base, 0, 0, file.getFlags()); - newBlocks.add(newBlock); - newFiles.add(existingName); - - // Sized exactly: the file's stored bytes are copied through - // verbatim apart from decryption, which preserves length. - final ByteBuffer target = out.reserve(file.getCompressedSize()); - file.writeFileAndBlock(newBlock, target); - out.advance(newBlock.getCompressedSize()); - currentPos += newBlock.getCompressedSize(); - } - log.debug("Copied {} existing files.", newFiles.size()); - return currentPos; - } - - private long writePendingFiles(GrowingBuffer out, List newFiles, List newBlocks, - long currentPos, long base, RecompressOptions options) throws IOException { - for (PendingFile pending : pendingFiles.values()) { - final byte[] fileData = pending.read(); - newFiles.add(pending.displayName()); - - final Block newBlock = new Block(currentPos - base, 0, 0, 0); - newBlocks.add(newBlock); - writeEncodedFile(out, fileData, newBlock, "", options); - currentPos += newBlock.getCompressedSize(); - log.debug("Added file {}", pending.displayName()); } - return currentPos; - } - - private long writeListfile(GrowingBuffer out, List newFiles, List newBlocks, - long currentPos, long base, RecompressOptions options) throws IOException { - newFiles.add("(listfile)"); - final byte[] listfileArr = listFile.asByteArray(); - final Block newBlock = new Block(currentPos - base, 0, 0, - EXISTS | COMPRESSED | ENCRYPTED | ADJUSTED_ENCRYPTED); - newBlocks.add(newBlock); - writeEncodedFile(out, listfileArr, newBlock, "(listfile)", options); - log.debug("Added listfile ({} entries)", listFile.size()); - return currentPos + newBlock.getCompressedSize(); - } - - /** - * Encodes one file into the image. - *

- * The encoder needs a bounded region to work in, and the exact compressed - * size is only known afterwards, so a worst-case region is made addressable - * and only the bytes actually produced are kept. Worst case is the sector - * offset table plus every sector stored verbatim plus one type byte each; - * nothing the encoder can produce exceeds that, because a sector that - * compresses to no less than its raw size is stored raw. The old code - * guessed {@code length * 2} and mapped that much file, which overflowed for - * incompressible input. - */ - private void writeEncodedFile(GrowingBuffer out, byte[] fileData, Block block, String name, - RecompressOptions options) { - final int sectors = Math.max(1, MpqFile.sectorCount(fileData.length, newDiscBlockSize)); - // long arithmetic: for a file close to 2 GiB the three terms overflow - // int, and a negative reservation fails with a nonsense message - // instead of saying what the real limit is. - final long worstCaseExact = (sectors + 1L) * 4L + fileData.length + sectors; - if (worstCaseExact > Integer.MAX_VALUE - 8) { - throw new IllegalArgumentException("File is too large for an in-memory rebuild: " - + fileData.length + " bytes would need " + worstCaseExact + " bytes of staging."); + for (String gone : deleted) { + writer.remove(gone); } - final int worstCase = (int) worstCaseExact; - - final ByteBuffer region = out.reserve(worstCase); - MpqFile.writeFileAndBlock(fileData, block, region, newDiscBlockSize, name, options); - out.advance(block.getCompressedSize()); - } - - private void writeHashTable(GrowingBuffer out, List newFiles) throws IOException { - final HashTable rebuilt = new HashTable(newHashSize); - int blockIndex = 0; - for (String file : newFiles) { - rebuilt.setFileBlockIndex(file, HashTable.DEFAULT_LOCALE, blockIndex++); + // Insertions last, so they win over whatever the archive held. + for (Insert insert : inserts.values()) { + if (insert.bytes() != null) { + writer.put(insert.name(), insert.bytes()); + } else { + writer.put(insert.name(), insert.file()); + } } - - final ByteBuffer buffer = ByteBuffer.allocate(newHashSize * 16); - rebuilt.writeToBuffer(buffer); - buffer.flip(); - new MPQEncryption(KEY_HASH_TABLE, false).processSingle(buffer); - buffer.flip(); - out.put(buffer); - } - - private void writeBlockTable(GrowingBuffer out, List newBlocks) { - final ByteBuffer buffer = - ByteBuffer.allocate(newBlockSize * BlockTable.ENTRY_SIZE).order(ByteOrder.LITTLE_ENDIAN); - BlockTable.writeNewBlocktable(newBlocks, newBlockSize, buffer); - buffer.flip(); - out.put(buffer); + return writer.toByteArray(); } /** * The rebuilt archive image from the most recent {@link #close()}. - *

- * This is the only way to retrieve the result for an archive opened from a - * byte array, since there is no file to write back to. * * @return the rebuilt image, or {@code null} if no rebuild has happened. */ @@ -1488,34 +688,15 @@ public byte[] getOutputByteArray() { } /** - * Utility method to fill a buffer from the given channel. - * - * @param buffer buffer to fill. - * @param src channel to fill from. - * @throws IOException if an exception occurs when reading. - * @throws EOFException if EoF is encountered before buffer is full or channel is non - * blocking. - */ - private static void readFully(ByteBuffer buffer, ReadableByteChannel src) throws IOException { - while (buffer.hasRemaining()) { - if (src.read(buffer) < 1) { - throw new EOFException("Cannot read enough bytes."); - } - } - } - - /** - * @return Whether the archive can be modified. + * @return whether the archive can be modified. */ public boolean isCanWrite() { return canWrite; } /** - * Whether to keep the data before the actual mpq in the file. - * - * @param keepHeaderOffset true to preserve the prefix, false to drop it so - * the archive starts at offset 0. + * @param keepHeaderOffset true to preserve bytes before the archive header, + * false to move the archive to offset 0. */ public void setKeepHeaderOffset(boolean keepHeaderOffset) { this.keepHeaderOffset = keepHeaderOffset; @@ -1525,42 +706,59 @@ public void setKeepHeaderOffset(boolean keepHeaderOffset) { * @return this archive's raw {@code wFormatVersion}. */ public int getFormatVersion() { - return formatVersion; + return archive.header().formatVersion(); } /** * @return this archive's sector size in bytes. */ public int getSectorSize() { - return discBlockSize; + return archive.header().sectorSize(); } /** * @return the block table. + * @deprecated exposes the on-disk index; use {@link MpqArchive#entries()}. */ + @Deprecated public BlockTable getBlockTable() { - return blockTable; + final List rows = new ArrayList<>(); + for (MpqFileEntry entry : archive.rawBlocks()) { + rows.add(new Block(entry.filePosition(), entry.compressedSize(), + entry.normalSize(), entry.flags())); + } + return BlockTable.of(rows); } /** * @return the hash table. + * @deprecated exposes the on-disk index; use {@link MpqArchive#entry(String)}. */ + @Deprecated public HashTable getHashTable() { - return hashTable; - } - - @Override - public String toString() { - return "JMpqEditor [headerSize=" + headerSize + ", archiveSize=" + archiveSize - + ", formatVersion=" + formatVersion + ", discBlockSize=" + discBlockSize - + ", hashPos=" + hashPos + ", blockPos=" + blockPos + ", hashSize=" + hashSize - + ", blockSize=" + blockSize + "]"; + return archive.hashTable(); } /** * @return an unmodifiable view of all list file entries. */ public Collection getListfileEntries() { - return Collections.unmodifiableCollection(listFile.getFiles()); + return Collections.unmodifiableCollection(getFileNames()); + } + + private static JMpqException wrap(String message, IOException cause) { + return cause instanceof JMpqException already + ? new JMpqException(message + ": " + already.getMessage(), already) + : new JMpqException(message, cause); + } + + @Override + public String toString() { + return "JMpqEditor[" + archive + ", canWrite=" + canWrite + "]"; + } + + /** @return the archive this facade delegates to. */ + Optional delegate() { + return Optional.ofNullable(archive); } } diff --git a/src/test/java/systems/crigges/jmpq3test/Phase0RegressionTests.java b/src/test/java/systems/crigges/jmpq3test/Phase0RegressionTests.java index 346ab7f..51a5e75 100644 --- a/src/test/java/systems/crigges/jmpq3test/Phase0RegressionTests.java +++ b/src/test/java/systems/crigges/jmpq3test/Phase0RegressionTests.java @@ -583,11 +583,26 @@ public void p0_8_malformedArchivesAreRejectedWithDiagnostics() throws IOExceptio * at the workaround rather than leaving the caller guessing. */ @Test - public void p0_8_badHeaderSizeIsReported() { + public void p0_8_badHeaderSizeIsRepairedNotRejected() throws IOException { + // This assertion is deliberately the opposite of what it was. Phase 0 + // rejected a garbage header size and told the caller to retry with + // FORCE_V0; P2-5a asks for StormLib's leniency instead, because + // Storm.dll ignores the field and the game loads these maps. The new + // core repairs the size, flags the archive malformed and opens it, so + // the protected maps of issue #46 are readable without a special + // option. Path mpq = TestResources.mpqCopy("listfileTooLong"); - JMpqException thrown = Assert.expectThrows(JMpqException.class, - () -> new JMpqEditor(mpq, MPQOpenOption.READ_ONLY)); - Assert.assertTrue(thrown.getMessage().contains("FORCE_V0"), thrown.getMessage()); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.READ_ONLY)) { + Assert.assertEquals(editor.getFormatVersion(), 0); + Assert.assertTrue(editor.hasFile("(listfile)"), + "the repaired header should still describe a usable archive"); + } + + try (org.inwc3.jmpq.MpqArchive archive = + org.inwc3.jmpq.MpqArchive.open(mpq, org.inwc3.jmpq.MpqOpenOptions.defaults())) { + Assert.assertTrue(archive.header().malformed(), + "a repaired header must still be reported as malformed"); + } } /** From dc835261403e00075e625000bd5dfb07dddebf47 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 09:06:02 +0200 Subject: [PATCH 2/6] Phase 1: behaviour contracts and the w3p extension hooks Completes P1-7 and P1-8, which finishes Phase 1. P1-7 turns three log-and-shrug behaviours into observable facts: - MpqArchive.isEnumerable() says whether names() reflects the whole archive, rather than a caller discovering it by getting an empty list. - MpqArchive.unnamedBlockCount() says exactly how many files a rebuild would discard, which is what an incomplete list file costs. Previously a warning at open time and nothing to act on. - MpqArchiveWriter.from() states plainly how many files it is leaving behind and why. Internal names are now correlated when enumerating, so (listfile) and (attributes) count as named. They are known by convention rather than listed - a list file does not list itself - and counting them as unnameable made unnamedBlockCount overstate what a rebuild would lose. BehaviourContractTests pins the edge semantics: archives with no list file, with an empty one, and with an incomplete one; external list file recovery; READ_ONLY never being overridden; prefix preservation and dropping; in-memory archives not touching the caller's array; and closing twice. P1-8 replaces the w3p branch rather than merging it. That branch carried protection policy as core API: a fakeFilesCount parameter on close, a maximise-tables mode, and a generator for plausible Warcraft III file names. Per the maintainer's decision none of that belongs here. The writer exposes the two mechanisms such a tool needs - explicit hash table capacity and extra block slots - plus list file suppression, and ExtensionHookTests builds what w3p built using only public API, including a full protected rebuild of a real map with maximised tables, no list file and the map prefix intact. That is the acceptance criterion for retiring the branch; deleting it is the maintainer's call. docs/migration-2.0.md maps every 1.x method to its replacement and lists the behaviour differences, which is the migration half of P5-1. Full suite green: 138 tests. --- docs/migration-2.0.md | 114 ++++++++ src/main/java/org/inwc3/jmpq/MpqArchive.java | 51 ++++ .../java/org/inwc3/jmpq/MpqArchiveWriter.java | 9 + .../jmpq3test/BehaviourContractTests.java | 248 ++++++++++++++++++ .../crigges/jmpq3test/ExtensionHookTests.java | 179 +++++++++++++ 5 files changed, 601 insertions(+) create mode 100644 docs/migration-2.0.md create mode 100644 src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java create mode 100644 src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java diff --git a/docs/migration-2.0.md b/docs/migration-2.0.md new file mode 100644 index 0000000..50cca06 --- /dev/null +++ b/docs/migration-2.0.md @@ -0,0 +1,114 @@ +# Migrating to JMPQ3 2.0 + +The 1.x API still works. `JMpqEditor` is a deprecated facade over the new core +and behaves as it did, so existing code compiles and runs unchanged. This +describes what to move to when you are ready. + +## Coordinates and packages + +| | 1.x | 2.0 | +|---|---|---| +| Maven group | `systems.crigges` | `org.inwc3` | +| New core | — | `org.inwc3.jmpq` | +| Deprecated facade | `systems.crigges.jmpq3` | unchanged, still importable | +| Java baseline | 11 | 25 | + +JitPack consumers using `com.github.inwc3:JMPQ3` are unaffected. Anything +resolving the Maven coordinate directly needs the new group. + +## The shape of the new API + +Reading and writing are separate types, and writing happens when you ask for it: + +```java +// read +try (MpqArchive archive = MpqArchive.open(path, MpqOpenOptions.warcraft3())) { + byte[] script = archive.read("war3map.j"); + for (String name : archive.names()) { ... } +} + +// write +byte[] image; +try (MpqArchive source = MpqArchive.open(path, MpqOpenOptions.warcraft3())) { + image = MpqArchiveWriter.from(source, MpqWriteOptions.defaults()) + .put("war3map.j", newScript) + .toByteArray(); +} +Files.write(path, image); +``` + +The build happens while the source is open, because content is read from it, and +the write happens after it is closed, because a mapped file cannot be replaced +on Windows. + +## Method mapping + +| 1.x `JMpqEditor` | 2.0 | +|---|---| +| `new JMpqEditor(path, FORCE_V0)` | `MpqArchive.open(path, MpqOpenOptions.warcraft3())` | +| `new JMpqEditor(path, READ_ONLY)` | `MpqArchive.open(path, MpqOpenOptions.defaults())` | +| `hasFile(name)` | `MpqArchive.contains(name)` | +| `hasFile(name, locale)` | `MpqArchive.contains(name, locale)` | +| `extractFileAsBytes(name)` | `MpqArchive.read(name)` | +| `extractFile(name, stream)` | `MpqArchive.readTo(name, stream)` | +| `extractFileAsString(name)` | `new String(archive.read(name), UTF_8)` | +| `getFileNames()` | `MpqArchive.names()` | +| `getTotalFileCount()` | `MpqArchive.blockCount()` | +| `getMpqFile(name)` | `MpqArchive.entry(name)` then `read(entry)` | +| `getBlockTable()`, `getHashTable()` | `MpqArchive.entries()` | +| `insertByteArray(name, bytes)` | `MpqArchiveWriter.put(name, bytes)` | +| `insertFile(name, file)` | `MpqArchiveWriter.put(name, path)` | +| `deleteFile(name)` | `MpqArchiveWriter.remove(name)` | +| `close()` rebuilding the archive | `MpqArchiveWriter.save(path)` or `toByteArray()` | +| `getOutputByteArray()` | `MpqArchiveWriter.toByteArray()` | +| `setKeepHeaderOffset(false)` | `MpqWriteOptions.withPrefix(false)` | +| `close(buildListfile, ...)` | `MpqWriteOptions.withListfile(...)` | +| `RecompressOptions.newSectorSizeShift` | `MpqWriteOptions.withSectorSizeShift(...)` | + +## Behaviour differences worth knowing + +**Nothing is written unless you ask.** The facade rebuilds on `close()`. The core +does not: `save` is a separate call. That is the single biggest difference, and +the reason the split exists — a read can no longer rewrite a file. + +**The format version is chosen, not inherited.** `MpqWriteOptions` accepts +version 0 or 1 and refuses the rest at construction. 1.x inherited whatever it +read and then emitted a version 0 header body for it. + +**Sector size changes force a re-encode.** A file can only keep its stored bytes +when the target archive keeps the source's sector size, because a sector offset +table is expressed in that sector size. The writer works this out; 1.x copied +regardless and silently corrupted the result. + +**Locales are first class.** One path can exist under several locales, and both +sides know it: `MpqArchive.localesOf(name)`, and `put`/`remove`/`contains` +overloads taking a locale. 1.x registered everything as neutral and a rebuild +dropped all but one variant. + +**An archive that cannot enumerate itself says so.** `MpqArchive.isEnumerable()` +and `unnamedBlockCount()` replace a log warning, so you can find out before a +rebuild how many files it would drop. + +**Malformed headers are repaired, not rejected.** A garbage header size is +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. + +## Protection tooling + +The `w3p` branch's `fakeFilesCount` and "maximise tables" mode are not in the +core. The mechanisms they needed are: + +```java +MpqWriteOptions.defaults() + .withHashTableCapacity(0x10000) // maximised version 0 table + .withExtraBlockEntries(32) // spare block slots + .withListfile(false) // not enumerable by name +``` + +Decoy entries are ordinary `put` calls; what they are called is the tool's +policy, not this library's. diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index 3acc986..7ec598a 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -51,6 +51,14 @@ public final class MpqArchive implements AutoCloseable { /** Encryption key for block table data. */ private static final int KEY_BLOCK_TABLE = tableKey("(block table)"); + /** + * Files an archive holds by convention rather than by being listed. A list + * file does not name itself, so these have to be known rather than + * discovered. + */ + private static final List INTERNAL_NAMES = + List.of("(listfile)", "(attributes)", "(signature)"); + private static int tableKey(String name) { final MPQHashGenerator hasher = MPQHashGenerator.getFileKeyGenerator(); hasher.process(name); @@ -245,6 +253,14 @@ public List entries() { for (String name : names.values()) { namesByKey.put(MpqNames.fileKey(name), name); } + // The internal files are known by name even though a list file does not + // list itself. Without these they would count as unnameable, which is + // what unnamedBlockCount reports as data a rebuild would lose. + for (String internal : INTERNAL_NAMES) { + if (hashTable.hasFile(internal)) { + namesByKey.put(MpqNames.fileKey(internal), internal); + } + } final Map byBlock = new LinkedHashMap<>(); for (HashTable.Mapping mapping : hashTable.mappings()) { // Prefer a mapping whose name is known, so a block reachable by @@ -440,6 +456,41 @@ private void readNames() { } } + /** + * Whether this archive can list its own contents. + *

+ * An archive without a usable {@code (listfile)} cannot: the hash table + * stores hashes, not names, so there is nothing to enumerate. Its files are + * still readable by exact name. This is a queryable fact rather than a log + * line, because a caller about to rebuild needs to know that names it + * cannot see would be dropped. + * + * @return whether {@link #names()} reflects the whole archive. + */ + public boolean isEnumerable() { + return !names.isEmpty(); + } + + /** + * How many live blocks no name resolves to. + *

+ * A rebuild can only carry over files it can name, so this is exactly how + * many files a rebuild would discard. Non-zero means the archive's list file + * is incomplete, which protected archives do deliberately. Before 2.0 this + * was a log warning at open time and nothing a caller could act on. + * + * @return the number of unnameable live blocks. + */ + public int unnamedBlockCount() { + int unnamed = 0; + for (MpqFileEntry entry : entries()) { + if (entry.name().isEmpty()) { + unnamed++; + } + } + return unnamed; + } + /** * The hash table backing this archive. *

diff --git a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java index 87931ff..053cee4 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java @@ -154,6 +154,15 @@ public static MpqArchiveWriter from(MpqArchive source, MpqWriteOptions options) new Pending(name, locale, new Content.Existing(source, entry))); } } + final int dropped = source.unnamedBlockCount(); + if (dropped > 0) { + // Stated plainly, because it is data loss the caller may not + // expect: these blocks exist but nothing names them, so the rebuilt + // archive cannot contain them. + log.warn("{} of the {} files in {} cannot be named and will not be carried over." + + " Supply a list file covering them if they matter.", + dropped, source.blockCount(), source); + } log.debug("Writer seeded with {} files from {}", writer.pending.size(), source); return writer; } diff --git a/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java b/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java new file mode 100644 index 0000000..449b040 --- /dev/null +++ b/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java @@ -0,0 +1,248 @@ +package systems.crigges.jmpq3test; + +import org.inwc3.jmpq.MpqArchive; +import org.inwc3.jmpq.MpqArchiveWriter; +import org.inwc3.jmpq.MpqOpenOptions; +import org.inwc3.jmpq.MpqWriteOptions; +import org.testng.Assert; +import org.testng.annotations.Test; +import systems.crigges.jmpq3.JMpqEditor; +import systems.crigges.jmpq3.MPQOpenOption; + +import java.io.IOException; +import java.nio.channels.NonWritableChannelException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +/** + * The edge-case contracts of P1-7, pinned so they cannot drift silently. + *

+ * Each of these was previously a log line and a shrug: the library would warn + * and carry on, and a caller had no way to find out what had happened. They are + * now observable facts, and this class is the specification of them. + */ +public class BehaviourContractTests { + + // ------------------------------------------------- archives with no listfile + + /** + * An archive with no {@code (listfile)} cannot enumerate itself, because the + * hash table stores hashes rather than names. Its files stay readable by + * exact name. + */ + @Test + public void archiveWithoutListfileIsNotEnumerableButIsReadable() throws IOException { + Path mpq = TestResources.mpqCopy("listfilelessMap"); + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + Assert.assertFalse(archive.isEnumerable()); + Assert.assertTrue(archive.names().isEmpty()); + + // But the blocks are there, and readable by exact name. + Assert.assertTrue(archive.blockCount() > 0); + Assert.assertTrue(archive.contains("war3map.j"), "readable by exact name"); + Assert.assertTrue(archive.read("war3map.j").length > 0); + } + } + + /** + * The facade downgrades such an archive to read-only rather than rebuilding + * it and dropping every file it cannot name. + */ + @Test + public void facadeDowngradesUnenumerableArchiveToReadOnly() throws IOException { + Path mpq = TestResources.mpqCopy("listfilelessMap"); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.FORCE_V0)) { + Assert.assertFalse(editor.isCanWrite(), "must not offer to rebuild what it cannot enumerate"); + Assert.expectThrows(NonWritableChannelException.class, + () -> editor.insertByteArray("x.txt", new byte[1])); + } + } + + /** An empty list file is still a list file: such an archive is writable. */ + @Test + public void emptyListfileStillAllowsWriting() throws IOException { + Path archivePath = TestResources.scratchDir("empty-listfile").resolve("fresh.w3x"); + JMpqEditor.createEmptyArchive(archivePath.toFile()); + + try (JMpqEditor editor = new JMpqEditor(archivePath, MPQOpenOption.FORCE_V0)) { + Assert.assertTrue(editor.isCanWrite(), + "a fresh archive must be able to receive its first file"); + editor.insertByteArray("first.txt", "hello".getBytes(StandardCharsets.UTF_8)); + } + try (JMpqEditor editor = new JMpqEditor(archivePath, MPQOpenOption.READ_ONLY, MPQOpenOption.FORCE_V0)) { + Assert.assertEquals(new String(editor.extractFileAsBytes("first.txt"), StandardCharsets.UTF_8), + "hello"); + } + } + + // ------------------------------------------------ incomplete listfiles + + /** + * A rebuild can only carry over files it can name, so an incomplete list + * file means data loss. The count is queryable in advance rather than + * discovered afterwards. + */ + @Test + public void incompleteListfileReportsWhatARebuildWouldDrop() throws IOException { + Path mpq = TestResources.mpqCopy("listfilelessMap"); + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + // Nothing is named, so every live block would be dropped. + Assert.assertEquals(archive.unnamedBlockCount(), archive.blockCount()); + } + + // A complete list file drops nothing. + Path complete = TestResources.mpqCopy("normalMap"); + try (MpqArchive archive = MpqArchive.open(complete, MpqOpenOptions.warcraft3())) { + Assert.assertTrue(archive.isEnumerable()); + Assert.assertEquals(archive.unnamedBlockCount(), 0, + "normalMap should name everything it holds"); + } + } + + /** + * An external list file recovers an unenumerable archive, and the recovered + * files survive the rebuild. + */ + @Test + public void externalListfileRecoversAnUnenumerableArchive() throws IOException { + Path mpq = TestResources.mpqCopy("listfilelessMap"); + Path listfile = TestResources.file("listfile.txt"); + + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.FORCE_V0)) { + Assert.assertFalse(editor.isCanWrite()); + editor.setExternalListfile(listfile.toFile()); + Assert.assertTrue(editor.isCanWrite(), "an external listfile must restore writability"); + editor.insertByteArray("recovered.txt", "yes".getBytes(StandardCharsets.UTF_8)); + } + + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + Assert.assertTrue(archive.isEnumerable(), "the rebuild should have written a listfile"); + Assert.assertTrue(archive.contains("recovered.txt")); + Assert.assertTrue(archive.names().size() > 1, + "the recovered names should be in the rebuilt listfile: " + archive.names()); + } + } + + /** A READ_ONLY editor stays read-only whatever list file it is handed. */ + @Test + public void readOnlyIsNeverOverridden() throws IOException { + Path mpq = TestResources.mpqCopy("listfilelessMap"); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.READ_ONLY, MPQOpenOption.FORCE_V0)) { + editor.setExternalListfile(TestResources.file("listfile.txt").toFile()); + Assert.assertFalse(editor.isCanWrite()); + } + } + + // -------------------------------------------------------- header prefix + + /** + * Warcraft III maps carry bytes before the archive header. Dropping them + * stops the map loading, so preserving them is the default and dropping + * them is explicit. + */ + @Test + public void headerPrefixIsPreservedByDefaultAndDroppableOnRequest() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + final long prefix; + final byte[] kept; + final byte[] dropped; + + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + prefix = archive.header().headerOffset(); + Assert.assertTrue(prefix > 0, "fixture should have a prefix"); + kept = MpqArchiveWriter.from(archive, MpqWriteOptions.defaults()).toByteArray(); + dropped = MpqArchiveWriter + .from(archive, MpqWriteOptions.defaults().withPrefix(false)).toByteArray(); + } + + try (MpqArchive archive = MpqArchive.open(kept, MpqOpenOptions.warcraft3())) { + Assert.assertEquals(archive.header().headerOffset(), prefix); + } + try (MpqArchive archive = MpqArchive.open(dropped, MpqOpenOptions.warcraft3())) { + Assert.assertEquals(archive.header().headerOffset(), 0); + } + } + + /** The facade's setKeepHeaderOffset(false) moves the archive to offset 0. */ + @Test + public void facadeCanDropTheHeaderPrefix() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.FORCE_V0)) { + editor.setKeepHeaderOffset(false); + } + final byte[] image = Files.readAllBytes(mpq); + final int magic = java.nio.ByteBuffer.wrap(image) + .order(java.nio.ByteOrder.LITTLE_ENDIAN).getInt(0); + Assert.assertEquals(magic, JMpqEditor.ARCHIVE_HEADER_MAGIC, + "the archive should now start at offset 0"); + } + + // ------------------------------------------------------ in-memory archives + + /** An in-memory archive never writes back to the caller's array. */ + @Test + public void inMemoryArchiveLeavesTheCallersArrayAlone() throws IOException { + final byte[] caller = Files.readAllBytes(TestResources.mpqCopy("normalMap")); + final byte[] pristine = caller.clone(); + + JMpqEditor editor = new JMpqEditor(caller, MPQOpenOption.FORCE_V0); + editor.insertByteArray("added.txt", "x".getBytes(StandardCharsets.UTF_8)); + editor.close(); + + Assert.assertEquals(caller, pristine, "the rebuild wrote into the caller's array"); + Assert.assertNotNull(editor.getOutputByteArray(), "the rebuilt image must be retrievable"); + + try (MpqArchive rebuilt = + MpqArchive.open(editor.getOutputByteArray(), MpqOpenOptions.warcraft3())) { + Assert.assertTrue(rebuilt.contains("added.txt")); + } + } + + /** A read-only in-memory archive produces no image, having rebuilt nothing. */ + @Test + public void readOnlyInMemoryArchiveProducesNoImage() throws IOException { + final byte[] caller = Files.readAllBytes(TestResources.mpqCopy("normalMap")); + JMpqEditor editor = new JMpqEditor(caller, MPQOpenOption.READ_ONLY, MPQOpenOption.FORCE_V0); + Assert.assertFalse(editor.getFileNames().isEmpty()); + editor.close(); + Assert.assertNull(editor.getOutputByteArray()); + } + + // ------------------------------------------------------- explicit failures + + /** Reading an absent file fails rather than returning nothing. */ + @Test + public void absentFileFailsExplicitly() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + Assert.assertFalse(archive.contains("nope.txt")); + Assert.assertTrue(archive.entry("nope.txt").isEmpty()); + Assert.expectThrows(IOException.class, () -> archive.read("nope.txt")); + } + } + + /** Closing twice is harmless, so try-with-resources plus an explicit close is safe. */ + @Test + public void closingTwiceIsHarmless() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.FORCE_V0)) { + editor.insertByteArray("a.txt", new byte[]{1}); + editor.close(); + editor.close(); + } + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + Assert.assertTrue(archive.contains("a.txt")); + } + } + + /** The writer refuses a format version it cannot write, at construction. */ + @Test + public void unwritableFormatVersionIsRefusedEarly() { + for (int version : List.of(2, 3, 4, -1)) { + Assert.expectThrows(IllegalArgumentException.class, + () -> MpqWriteOptions.defaults().withFormatVersion(version)); + } + } +} diff --git a/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java b/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java new file mode 100644 index 0000000..f9b8007 --- /dev/null +++ b/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java @@ -0,0 +1,179 @@ +package systems.crigges.jmpq3test; + +import org.inwc3.jmpq.MpqArchive; +import org.inwc3.jmpq.MpqArchiveWriter; +import org.inwc3.jmpq.MpqOpenOptions; +import org.inwc3.jmpq.MpqWriteOptions; +import org.testng.Assert; +import org.testng.annotations.Test; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; + +/** + * P1-8: the extension hooks that replace the {@code w3p} branch. + *

+ * That branch carried protection features as core API — a {@code fakeFilesCount} + * parameter on {@code close}, a "maximise V0 tables" mode, and a generator for + * plausible-looking Warcraft III file names. Per the maintainer's decision none + * of that belongs in this library: it is one tool's policy, and baking it in + * means every caller pays for it and the library has opinions about deceiving + * MPQ readers. + *

+ * Instead the writer exposes the two mechanisms such a tool actually needs — + * explicit table capacity and extra block slots — and the policy lives in the + * tool. These tests build what {@code w3p} built, using only public API, which + * is the acceptance criterion for retiring the branch. + */ +public class ExtensionHookTests { + + /** The hash table size a maximised Warcraft III version 0 archive uses. */ + private static final int WARCRAFT_V0_HASH_TABLE_SIZE = 0x10000; + + /** + * A maximised version 0 hash table, which is what the {@code w3p} branch's + * "maximise tables" mode produced. + */ + @Test + public void hashTableCapacityCanBeMaximised() throws IOException { + final byte[] image = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withHashTableCapacity(WARCRAFT_V0_HASH_TABLE_SIZE)) + .put("war3map.j", "script".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.warcraft3())) { + Assert.assertEquals(archive.header().hashTableEntries(), WARCRAFT_V0_HASH_TABLE_SIZE); + Assert.assertEquals(archive.read("war3map.j"), + "script".getBytes(StandardCharsets.UTF_8)); + // The archive must still be a working archive, not just a big table. + Assert.assertTrue(archive.isEnumerable()); + } + } + + /** + * Padding entries: a tool wanting decoy names adds them as ordinary files, + * and controls the surrounding table sizes through the write options. No + * library support for inventing names is needed or wanted. + */ + @Test + public void decoyEntriesAreJustFilesPlusCapacity() throws IOException { + final MpqArchiveWriter writer = MpqArchiveWriter.create( + MpqWriteOptions.defaults() + .withHashTableCapacity(1024) + .withExtraBlockEntries(64)); + + writer.put("war3map.j", "real script".getBytes(StandardCharsets.UTF_8)); + + // The policy — what the decoys are called — belongs to the caller. + final List decoys = new ArrayList<>(); + for (int i = 0; i < 200; i++) { + final String name = "Units\\decoy" + i + ".slk"; + decoys.add(name); + writer.put(name, new byte[]{(byte) i}); + } + + final byte[] image = writer.toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.warcraft3())) { + Assert.assertEquals(archive.header().hashTableEntries(), 1024); + // Real file, decoys, list file, plus the requested spare slots. + Assert.assertEquals(archive.header().blockTableEntries(), + 1 + decoys.size() + 1 + 64); + Assert.assertEquals(archive.read("war3map.j"), + "real script".getBytes(StandardCharsets.UTF_8)); + for (String decoy : decoys) { + Assert.assertTrue(archive.contains(decoy), decoy); + } + // Spare slots must not read as files. + Assert.assertEquals(archive.blockCount(), 1 + decoys.size() + 1); + } + } + + /** + * A capacity too small for the file count is refused rather than producing + * an archive whose hash table cannot hold its own contents. + */ + @Test + public void insufficientCapacityIsRefused() { + final MpqArchiveWriter writer = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withHashTableCapacity(4)); + for (int i = 0; i < 16; i++) { + writer.put("file" + i + ".txt", new byte[]{1}); + } + Assert.expectThrows(IOException.class, writer::toByteArray); + } + + /** Capacity must be a power of two, as the format requires. */ + @Test + public void capacityMustBeAPowerOfTwo() { + Assert.expectThrows(IllegalArgumentException.class, + () -> MpqWriteOptions.defaults().withHashTableCapacity(1000)); + Assert.expectThrows(IllegalArgumentException.class, + () -> MpqWriteOptions.defaults().withHashTableCapacity(-8)); + Assert.expectThrows(IllegalArgumentException.class, + () -> MpqWriteOptions.defaults().withExtraBlockEntries(-1)); + // 0 means "size it automatically", which is the default. + Assert.assertEquals(MpqWriteOptions.defaults().hashTableCapacity(), 0); + } + + /** + * The list file can be suppressed, which is the other thing a protection + * tool wants: an archive whose contents cannot be enumerated by name. + */ + @Test + public void listfileCanBeSuppressedForProtectedArchives() throws IOException { + final byte[] image = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withListfile(false)) + .put("war3map.j", "hidden".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.warcraft3())) { + Assert.assertFalse(archive.isEnumerable(), "no listfile means no enumeration"); + Assert.assertFalse(archive.contains("(listfile)")); + // Still readable if you know the name, which is the point. + Assert.assertEquals(archive.read("war3map.j"), "hidden".getBytes(StandardCharsets.UTF_8)); + Assert.assertEquals(archive.unnamedBlockCount(), archive.blockCount(), + "every block should be unnameable without a listfile"); + } + } + + /** + * Everything a rebuild of a real map needs, combined: maximised tables, a + * suppressed list file, and the original prefix preserved so the map still + * loads. This is the shape {@code w3p} produced, from public API only. + */ + @Test + public void aProtectedRebuildOfARealMapIsExpressible() throws IOException { + Path source = TestResources.mpqCopy("normalMap"); + final byte[] protectedImage; + final List originalNames; + + try (MpqArchive archive = MpqArchive.open(source, MpqOpenOptions.warcraft3())) { + originalNames = archive.names(); + final MpqArchiveWriter writer = MpqArchiveWriter.from(archive, + MpqWriteOptions.defaults() + .withHashTableCapacity(WARCRAFT_V0_HASH_TABLE_SIZE) + .withExtraBlockEntries(32) + .withListfile(false) + .withPrefix(true)); + protectedImage = writer.toByteArray(); + } + + try (MpqArchive archive = MpqArchive.open(protectedImage, MpqOpenOptions.warcraft3())) { + Assert.assertEquals(archive.header().hashTableEntries(), WARCRAFT_V0_HASH_TABLE_SIZE); + Assert.assertFalse(archive.contains("(listfile)")); + Assert.assertTrue(archive.header().headerOffset() > 0, "the map prefix must survive"); + + // Every original file is still there, reachable by name. + for (String name : originalNames) { + if (name.equals("(listfile)")) { + continue; + } + Assert.assertTrue(archive.contains(name), "lost " + name); + } + } + } +} From cc222fb94ad93bc8f9c4962b571792e6eeed3e2c Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 09:18:10 +0200 Subject: [PATCH 3/6] Distinguish absent, unreadable and empty listfiles All four review findings were one root cause: three distinct states were being treated as two. An archive with no (listfile), one whose (listfile) will not decode, and one whose (listfile) parsed and happens to be empty are different situations, and the code kept collapsing whichever pair was convenient. The consequences the reviewer found: - A corrupt (listfile) left the facade writable, because it checked only whether the block existed. Closing such an editor would seed the writer with nothing and replace the archive with an effectively empty one. That is the P1, and it is data loss. - isEnumerable() returned false for a valid empty listfile, so a caller using it to decide whether a rebuild is safe would refuse a healthy fresh archive. - extractAllFiles skipped its block fallback whenever an (attributes) or (signature) file put a single name in the list, hiding every recoverable unnamed block. MpqArchive now records the state as an enum, isEnumerable() means "parsed", and both the facade's read-only downgrade and the block fallback gate on it. The fourth finding was the mirror image of a fix in the previous commit: marking internal files as named made the loss count exclude (attributes) and (signature), which the writer does not carry over. Since the method promised to report exactly what a rebuild discards, it is now filesLostOnRebuild() and counts them; unnamedBlockCount() remains, deprecated, for the literal question it actually answered. Coverage -------- Reducing JMpqEditor to a facade orphaned a lot of legacy code, which read as a 9% coverage drop. The cause was dead code in the denominator, not lost testing: - GrowingBuffer had zero coverage and zero callers. Deleted; the new core has MpqImageBuffer. - AttributesFile had zero coverage and zero callers, but is public API that P2-4 will build on, so it is now tested rather than removed. - MpqFile sat at 8% because the facade no longer routes through it, yet getMpqFile() still hands it to callers. LegacyApiTests covers the deprecated surface directly for the first time. It is a real gap, not a metric artefact: these types are supported and reachable, and the old tests only reached them incidentally through the editor's internals. The MpqFile path is checked against the facade on every fixture, so the deprecated route cannot quietly return different bytes from the supported one. Line coverage 79.05%, above where it was before this branch. --- src/main/java/org/inwc3/jmpq/MpqArchive.java | 56 ++++- .../java/org/inwc3/jmpq/MpqArchiveWriter.java | 2 +- .../systems/crigges/jmpq3/GrowingBuffer.java | 217 ----------------- .../systems/crigges/jmpq3/JMpqEditor.java | 17 +- .../jmpq3test/BehaviourContractTests.java | 8 +- .../crigges/jmpq3test/ExtensionHookTests.java | 4 +- .../crigges/jmpq3test/LegacyApiTests.java | 220 ++++++++++++++++++ 7 files changed, 295 insertions(+), 229 deletions(-) delete mode 100644 src/main/java/systems/crigges/jmpq3/GrowingBuffer.java create mode 100644 src/test/java/systems/crigges/jmpq3test/LegacyApiTests.java diff --git a/src/main/java/org/inwc3/jmpq/MpqArchive.java b/src/main/java/org/inwc3/jmpq/MpqArchive.java index 7ec598a..33c9db0 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchive.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchive.java @@ -59,6 +59,13 @@ public final class MpqArchive implements AutoCloseable { private static final List INTERNAL_NAMES = List.of("(listfile)", "(attributes)", "(signature)"); + /** + * Internal files a rebuild does not carry over. {@code (listfile)} is + * 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 int tableKey(String name) { final MPQHashGenerator hasher = MPQHashGenerator.getFileKeyGenerator(); hasher.process(name); @@ -77,6 +84,25 @@ private static int tableKey(String name) { /** Names from the archive's list file, canonical name to spelling. */ private final SequencedMap names = new LinkedHashMap<>(); + /** + * How much this archive knows about its own contents. + *

+ * Three states that must not be conflated, and were: an archive with no + * list file, one whose list file will not parse, and one whose list file + * parsed and happens to be empty. The first two mean a rebuild would lose + * whatever it cannot name; the third is a perfectly healthy empty archive. + */ + public enum Enumeration { + /** The list file parsed. {@link #names()} is authoritative. */ + PARSED, + /** No {@code (listfile)} block at all. */ + ABSENT, + /** A {@code (listfile)} block that could not be decoded. */ + UNREADABLE + } + + private Enumeration enumeration = Enumeration.ABSENT; + private MpqArchive(MpqSource source, MpqOpenOptions options) throws IOException { this.source = source; this.defaultLocale = options.defaultLocale(); @@ -434,6 +460,7 @@ private HashTable readHashTable() throws IOException { */ private void readNames() { if (!contains("(listfile)")) { + enumeration = Enumeration.ABSENT; log.debug("{} has no (listfile); it cannot be enumerated.", source.origin()); return; } @@ -441,10 +468,14 @@ private void readNames() { try { listfile = new Listfile(read("(listfile)")); } catch (IOException | RuntimeException e) { + enumeration = Enumeration.UNREADABLE; log.warn("Cannot read the (listfile) of {}; the archive is not enumerable.", source.origin(), e); return; } + // Parsed, even if it turns out to name nothing: an empty list file is a + // valid list file, and a fresh archive has one. + enumeration = Enumeration.PARSED; for (String name : listfile.getFiles()) { if (contains(name)) { @@ -468,7 +499,14 @@ private void readNames() { * @return whether {@link #names()} reflects the whole archive. */ public boolean isEnumerable() { - return !names.isEmpty(); + return enumeration == Enumeration.PARSED; + } + + /** + * @return which of the three enumeration states this archive is in. + */ + public Enumeration enumerationState() { + return enumeration; } /** @@ -481,6 +519,22 @@ public boolean isEnumerable() { * * @return the number of unnameable live blocks. */ + public int filesLostOnRebuild() { + int lost = 0; + for (MpqFileEntry entry : entries()) { + if (entry.name().isEmpty() || LOST_ON_REBUILD.contains(entry.name())) { + lost++; + } + } + return lost; + } + + /** + * @return the number of live blocks no name resolves to. + * @deprecated counts only nameless blocks, which understates what a rebuild + * discards; use {@link #filesLostOnRebuild()}. + */ + @Deprecated public int unnamedBlockCount() { int unnamed = 0; for (MpqFileEntry entry : entries()) { diff --git a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java index 053cee4..0200afd 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java @@ -154,7 +154,7 @@ public static MpqArchiveWriter from(MpqArchive source, MpqWriteOptions options) new Pending(name, locale, new Content.Existing(source, entry))); } } - final int dropped = source.unnamedBlockCount(); + final int dropped = source.filesLostOnRebuild(); if (dropped > 0) { // Stated plainly, because it is data loss the caller may not // expect: these blocks exist but nothing names them, so the rebuilt diff --git a/src/main/java/systems/crigges/jmpq3/GrowingBuffer.java b/src/main/java/systems/crigges/jmpq3/GrowingBuffer.java deleted file mode 100644 index ef87b03..0000000 --- a/src/main/java/systems/crigges/jmpq3/GrowingBuffer.java +++ /dev/null @@ -1,217 +0,0 @@ -package systems.crigges.jmpq3; - -import java.io.IOException; -import java.nio.ByteBuffer; -import java.nio.ByteOrder; -import java.nio.channels.WritableByteChannel; -import java.util.Arrays; - -/** - * A little-endian, append-mostly byte sink that grows on demand and supports - * back-patching earlier regions. - *

- * This is what replaced the archive rebuild's two previous strategies, both of - * which were sources of real bugs: - *

- * A growing heap buffer needs neither: after compression the exact sizes are - * known, nothing touches the filesystem until the finished image is written - * out, and there is no global state to race on. - * - *

High-water mark

- * {@link #size()} tracks the furthest byte ever written, not the current - * position. The rebuild reserves a header at the front, appends the whole - * archive, then seeks back to fill the header in; a plain position-based length - * would truncate the archive at that point. (w3p's {@code DynamicByteBuffer} had - * exactly that flaw: growing while the position was rewound discarded - * everything past it.) - * - *

Thread safety

- * Not thread safe; one instance belongs to one rebuild. - */ -final class GrowingBuffer { - private static final int MIN_CAPACITY = 64; - - private byte[] data; - - /** Next write position. */ - private int position; - - /** One past the furthest byte ever written. */ - private int highWaterMark; - - /** - * @param initialCapacity starting capacity; clamped to a sane minimum. - */ - GrowingBuffer(int initialCapacity) { - this.data = new byte[Math.max(MIN_CAPACITY, initialCapacity)]; - } - - /** - * @return the number of bytes written, i.e. the length of the image built - * so far. - */ - int size() { - return highWaterMark; - } - - /** - * @return the current write position. - */ - int position() { - return position; - } - - /** - * @param newPosition new write position; may exceed {@link #size()} to - * reserve space, which is then zero filled. - */ - void position(int newPosition) { - if (newPosition < 0) { - throw new IllegalArgumentException("Position cannot be negative: " + newPosition); - } - ensureCapacity(newPosition); - this.position = newPosition; - } - - /** - * Reserves {@code count} zero bytes at the current position and skips past - * them. - * - * @param count number of bytes to reserve. - */ - void skip(int count) { - if (count < 0) { - throw new IllegalArgumentException("Cannot skip a negative number of bytes: " + count); - } - ensureCapacity(position + count); - position += count; - highWaterMark = Math.max(highWaterMark, position); - } - - void put(byte[] src) { - put(src, 0, src.length); - } - - void put(byte[] src, int offset, int length) { - ensureCapacity(position + length); - System.arraycopy(src, offset, data, position, length); - position += length; - highWaterMark = Math.max(highWaterMark, position); - } - - void put(ByteBuffer src) { - final int length = src.remaining(); - ensureCapacity(position + length); - src.get(data, position, length); - position += length; - highWaterMark = Math.max(highWaterMark, position); - } - - void putInt(int value) { - ensureCapacity(position + 4); - data[position] = (byte) value; - data[position + 1] = (byte) (value >>> 8); - data[position + 2] = (byte) (value >>> 16); - data[position + 3] = (byte) (value >>> 24); - position += 4; - highWaterMark = Math.max(highWaterMark, position); - } - - /** - * Overwrites an already written region without moving the position. - * - * @param index absolute offset to write at; must lie inside - * {@link #size()} plus {@code src.length}. - * @param src bytes to write. - */ - void putAt(int index, byte[] src) { - if (index < 0) { - throw new IllegalArgumentException("Index cannot be negative: " + index); - } - ensureCapacity(index + src.length); - System.arraycopy(src, 0, data, index, src.length); - highWaterMark = Math.max(highWaterMark, index + src.length); - } - - /** - * Hands out a {@link ByteBuffer} view of {@code length} bytes at the current - * position, so an encoder can write straight into the image with no - * intermediate copy. - *

- * Neither the position nor the length of the image moves: the region is - * only made addressable. Call {@link #advance(int)} afterwards with the - * number of bytes actually produced. That split matters because an encoder - * needs a worst-case region to work in but usually fills less of it, and - * counting the unused tail would leave garbage in the finished archive. - *

- * The view is invalidated by anything that grows this buffer, so it must be - * finished with first. - * - * @param length number of bytes to make addressable. - * @return a little-endian view over that region. - */ - ByteBuffer reserve(int length) { - ensureCapacity(position + length); - return ByteBuffer.wrap(data, position, length).slice().order(ByteOrder.LITTLE_ENDIAN); - } - - /** - * Moves past bytes written through a {@link #reserve(int)} view. - * - * @param length number of bytes actually produced. - */ - void advance(int length) { - if (length < 0) { - throw new IllegalArgumentException("Cannot advance by a negative number of bytes: " + length); - } - ensureCapacity(position + length); - position += length; - highWaterMark = Math.max(highWaterMark, position); - } - - /** - * @return a copy of the bytes written so far. - */ - byte[] toByteArray() { - return Arrays.copyOf(data, highWaterMark); - } - - /** - * Writes the image to a channel. - * - * @param dest channel to write to, at its current position. - * @throws IOException if the channel rejects the write. - */ - void writeTo(WritableByteChannel dest) throws IOException { - final ByteBuffer view = ByteBuffer.wrap(data, 0, highWaterMark); - while (view.hasRemaining()) { - if (dest.write(view) < 1) { - throw new IOException("Cannot write archive image: channel accepted no bytes."); - } - } - } - - private void ensureCapacity(int required) { - if (required < 0) { - throw new OutOfMemoryError("Archive image exceeds 2 GiB."); - } - if (required <= data.length) { - return; - } - int capacity = data.length; - while (capacity < required) { - final int doubled = capacity << 1; - // Saturate rather than overflow into a negative capacity. - capacity = doubled > 0 ? doubled : Integer.MAX_VALUE - 8; - } - data = Arrays.copyOf(data, capacity); - } -} diff --git a/src/main/java/systems/crigges/jmpq3/JMpqEditor.java b/src/main/java/systems/crigges/jmpq3/JMpqEditor.java index 987368d..e586667 100644 --- a/src/main/java/systems/crigges/jmpq3/JMpqEditor.java +++ b/src/main/java/systems/crigges/jmpq3/JMpqEditor.java @@ -195,11 +195,12 @@ private MpqOpenOptions openOptions() { * the files whose names are unknown, so it is downgraded to read-only. */ private void downgradeIfNotEnumerable() { - // Presence of the (listfile), not whether it named anything: a freshly - // created archive has an empty one and is perfectly writable, and - // treating that as unenumerable made it impossible to add the first - // file to it. - if (canWrite && !archive.contains("(listfile)")) { + // Three states, not two. A parsed list file means the archive knows its + // own contents, even if it names nothing -- a fresh archive has an empty + // one and must be able to receive its first file. Both an absent list + // file and one that will not decode mean a rebuild would replace the + // archive with whatever it could name, which is nothing. + if (canWrite && !archive.isEnumerable()) { log.warn("The mpq doesn't contain a listfile. It cannot be rebuilt."); canWrite = false; } @@ -301,7 +302,11 @@ public void extractAllFiles(File dest) throws JMpqException { } } - if (names.isEmpty()) { + if (!archive.isEnumerable()) { + // Gate on the archive's own knowledge, not on whether this list + // happened to be empty: an unenumerable archive holding an + // (attributes) file still put one name in the list, which used to + // suppress the fallback and hide every recoverable block. extractUnnamedBlocks(root); } } diff --git a/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java b/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java index 449b040..b0090f9 100644 --- a/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java +++ b/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java @@ -89,15 +89,19 @@ public void incompleteListfileReportsWhatARebuildWouldDrop() throws IOException Path mpq = TestResources.mpqCopy("listfilelessMap"); try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { // Nothing is named, so every live block would be dropped. - Assert.assertEquals(archive.unnamedBlockCount(), archive.blockCount()); + Assert.assertEquals(archive.filesLostOnRebuild(), archive.blockCount()); } // A complete list file drops nothing. Path complete = TestResources.mpqCopy("normalMap"); try (MpqArchive archive = MpqArchive.open(complete, MpqOpenOptions.warcraft3())) { Assert.assertTrue(archive.isEnumerable()); + // normalMap holds an (attributes) file, which a rebuild does not + // carry over, so exactly one file is at risk. + Assert.assertEquals(archive.filesLostOnRebuild(), 1, + "only the (attributes) file should be at risk"); Assert.assertEquals(archive.unnamedBlockCount(), 0, - "normalMap should name everything it holds"); + "every block should be nameable"); } } diff --git a/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java b/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java index f9b8007..41d7420 100644 --- a/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java +++ b/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java @@ -135,8 +135,8 @@ public void listfileCanBeSuppressedForProtectedArchives() throws IOException { Assert.assertFalse(archive.contains("(listfile)")); // Still readable if you know the name, which is the point. Assert.assertEquals(archive.read("war3map.j"), "hidden".getBytes(StandardCharsets.UTF_8)); - Assert.assertEquals(archive.unnamedBlockCount(), archive.blockCount(), - "every block should be unnameable without a listfile"); + Assert.assertEquals(archive.filesLostOnRebuild(), archive.blockCount(), + "every block is at risk without a listfile"); } } diff --git a/src/test/java/systems/crigges/jmpq3test/LegacyApiTests.java b/src/test/java/systems/crigges/jmpq3test/LegacyApiTests.java new file mode 100644 index 0000000..01c72a4 --- /dev/null +++ b/src/test/java/systems/crigges/jmpq3test/LegacyApiTests.java @@ -0,0 +1,220 @@ +package systems.crigges.jmpq3test; + +import org.testng.Assert; +import org.testng.annotations.Test; +import systems.crigges.jmpq3.AttributesFile; +import systems.crigges.jmpq3.BlockTable; +import systems.crigges.jmpq3.JMpqEditor; +import systems.crigges.jmpq3.MPQOpenOption; +import systems.crigges.jmpq3.MpqFile; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.file.Path; +import java.util.List; + +/** + * The deprecated API that survives for compatibility. + *

+ * Reducing {@code JMpqEditor} to a facade stopped the facade itself from using + * most of this code, which showed up as a coverage cliff. That is a real gap + * rather than a metric artefact: these types are still public, still supported + * and still reachable, and until this class existed nothing exercised them + * directly — the old tests only reached them incidentally through the editor's + * internals. + */ +public class LegacyApiTests { + + // -------------------------------------------------------------- MpqFile + + /** + * {@link JMpqEditor#getMpqFile(String)} hands out a legacy {@code MpqFile}, + * which decodes independently of the core. It must agree with the core on + * every fixture, or the deprecated path is quietly returning different + * bytes from the supported one. + */ + @Test + public void legacyMpqFileAgreesWithTheFacade() throws IOException { + int compared = 0; + for (Path mpq : TestResources.mpqCopies()) { + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.READ_ONLY, MPQOpenOption.FORCE_V0)) { + for (String name : editor.getFileNames()) { + final byte[] viaFacade; + try { + viaFacade = editor.extractFileAsBytes(name); + } catch (IOException undecodable) { + continue; + } + final MpqFile file = editor.getMpqFile(name); + Assert.assertEquals(file.extractToBytes(), viaFacade, + mpq.getFileName() + " / " + name); + Assert.assertEquals(file.getNormalSize(), viaFacade.length, name); + Assert.assertEquals(file.getName(), name); + compared++; + } + } + } + Assert.assertTrue(compared > 100, "only compared " + compared + " files"); + } + + /** The legacy streaming and file-writing paths must match the byte array one. */ + @Test + public void legacyMpqFileOutputPathsAgree() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + Path out = TestResources.scratchDir("legacy-extract"); + + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.READ_ONLY, MPQOpenOption.FORCE_V0)) { + for (String name : editor.getFileNames()) { + final MpqFile file = editor.getMpqFile(name); + final byte[] bytes = file.extractToBytes(); + + final ByteArrayOutputStream streamed = new ByteArrayOutputStream(); + editor.getMpqFile(name).extractToOutputStream(streamed); + Assert.assertEquals(streamed.toByteArray(), bytes, name); + + final Path target = out.resolve(name.replace('\\', '_')); + editor.getMpqFile(name).extractToPath(target); + Assert.assertEquals(java.nio.file.Files.readAllBytes(target), bytes, name); + + Assert.assertNotNull(file.toString()); + } + } + } + + /** Extraction must never close a stream it was handed. */ + @Test + public void legacyMpqFileDoesNotCloseCallerStreams() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.READ_ONLY, MPQOpenOption.FORCE_V0)) { + final Tracking sink = new Tracking(); + for (String name : editor.getFileNames()) { + editor.getMpqFile(name).extractToOutputStream(sink); + Assert.assertFalse(sink.closed, "closed the caller's stream on " + name); + } + } + } + + private static final class Tracking extends ByteArrayOutputStream { + private boolean closed; + + @Override + public void close() throws IOException { + closed = true; + super.close(); + } + } + + /** Sector counting is exposed and must stay overflow free. */ + @Test + public void sectorCountIsExposedAndSafe() { + Assert.assertEquals(MpqFile.sectorCount(0, 4096), 0); + Assert.assertEquals(MpqFile.sectorCount(4097, 4096), 2); + Assert.assertEquals(MpqFile.sectorCount(Integer.MAX_VALUE, 4096), 524288); + Assert.expectThrows(IllegalArgumentException.class, () -> MpqFile.sectorCount(-1, 4096)); + Assert.expectThrows(IllegalArgumentException.class, () -> MpqFile.sectorCount(1, 0)); + } + + // ----------------------------------------------------------- BlockTable + + /** The deprecated index accessors must describe the same archive. */ + @Test + public void deprecatedIndexAccessorsDescribeTheArchive() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.READ_ONLY, MPQOpenOption.FORCE_V0)) { + final BlockTable blocks = editor.getBlockTable(); + Assert.assertNotNull(blocks); + Assert.assertTrue(blocks.size() > 0); + Assert.assertEquals(blocks.getAllValidBlocks().size(), editor.getTotalFileCount()); + + for (BlockTable.Block block : blocks.getAllValidBlocks()) { + Assert.assertTrue(block.hasFlag(MpqFile.EXISTS)); + Assert.assertTrue(block.getFilePosition() >= 0); + Assert.assertNotNull(block.printFlags()); + Assert.assertNotNull(block.toString()); + } + + // Out of range must be reported, not silently produce garbage. + Assert.expectThrows(IOException.class, () -> blocks.getBlockAtPos(-1)); + Assert.expectThrows(IOException.class, () -> blocks.getBlockAtPos(blocks.size())); + + Assert.assertNotNull(editor.getHashTable()); + Assert.assertTrue(editor.getHashTable().hasFile("war3map.j")); + Assert.assertTrue(editor.getHashTable().capacity() > 0); + Assert.assertTrue(editor.getHashTable().size() > 0); + } + } + + /** Every readable block must be reachable without a name. */ + @Test + public void blocksAreReachableWithoutNames() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.READ_ONLY, MPQOpenOption.FORCE_V0)) { + final List files = editor.getMpqFilesByBlockTable(); + Assert.assertFalse(files.isEmpty()); + for (MpqFile file : files) { + Assert.assertTrue(file.getCompressedSize() >= 0); + } + } + } + + // ------------------------------------------------------- AttributesFile + + /** + * {@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. + */ + @Test + public void attributesFileRoundTrips() { + 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 byte[] image = written.buildFile(); + Assert.assertEquals(image.length, 8 + 12 * entries); + Assert.assertEquals(image[0], 100, "format version"); + 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); + for (int i = 0; i < read.entries(); i++) { + Assert.assertEquals(read.getCrc32()[i], 0x1000 + i); + } + } + + /** The CRC32 helper must match java.util.zip for the same bytes. */ + @Test + public void attributesCrcMatchesTheJdk() { + final byte[] payload = TestResources.bytes("Example.txt"); + final java.util.zip.CRC32 expected = new java.util.zip.CRC32(); + expected.update(payload); + + final AttributesFile attributes = new AttributesFile(1); + Assert.assertEquals(attributes.getCrc32(payload), (int) expected.getValue()); + Assert.assertEquals(attributes.getCrc32(new byte[0]), 0); + } + + /** Timestamps and names are addressable, which P2-4 will build on. */ + @Test + public void attributesTimestampsAndNamesAreAddressable() { + final AttributesFile attributes = new AttributesFile(3); + attributes.setEntry(0, 1, 111L); + attributes.setEntry(1, 2, 222L); + attributes.setEntry(2, 3, 333L); + + Assert.assertEquals(attributes.getTimestamps()[1], 222L); + Assert.assertEquals(attributes.entries(), 3); + Assert.assertNotNull(attributes.getFile()); + + attributes.setNames(new java.util.ArrayList<>(List.of("a.txt", "b.txt", "c.txt"))); + Assert.assertEquals(attributes.getEntry("b.txt"), 1); + Assert.assertEquals(attributes.getEntry("absent.txt"), -1); + } +} From a2f9b266a8165d032c8b15cc788843591f53ad3f Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 09:23:04 +0200 Subject: [PATCH 4/6] Delete the legacy write path The three writeFileAndBlock overloads on MpqFile had no callers left: the facade routes writes through MpqArchiveWriter, so this was the second encoding implementation sitting unused. Removing it is the point of the refactor rather than tidying after it, because a duplicate write path is exactly what drifted before. They were public, but on a deprecated class and taking BlockTable.Block and ByteBuffer -- internal plumbing that happened to have public visibility rather than API anyone could reasonably call. MpqFile drops from 519 to 358 lines and from 41.5% to 77.7% line coverage; overall line coverage is 82.33%, above the pre-branch baseline. The remaining uncovered lines are error branches for damaged archives. --- .../java/systems/crigges/jmpq3/MpqFile.java | 195 ------------------ 1 file changed, 195 deletions(-) diff --git a/src/main/java/systems/crigges/jmpq3/MpqFile.java b/src/main/java/systems/crigges/jmpq3/MpqFile.java index c51d715..0972ced 100644 --- a/src/main/java/systems/crigges/jmpq3/MpqFile.java +++ b/src/main/java/systems/crigges/jmpq3/MpqFile.java @@ -2,7 +2,6 @@ import systems.crigges.jmpq3.BlockTable.Block; import systems.crigges.jmpq3.compression.CompressionUtil; -import systems.crigges.jmpq3.compression.RecompressOptions; import systems.crigges.jmpq3.security.MPQEncryption; import java.io.ByteArrayOutputStream; @@ -348,202 +347,8 @@ private void decrypt(byte[] data, int key) { } } - /** - * Copies this file into a rebuilt archive at a new position, filling in the - * new block table entry. - *

- * Encryption policy. An encrypted file's sector key depends on its - * name and, for {@code ADJUSTED_ENCRYPTED} files, on its offset inside the - * archive and its size. Relocating the file therefore invalidates its key. - * This method decrypts the sectors and stores them plain, clearing - * {@code ENCRYPTED} and {@code ADJUSTED_ENCRYPTED} on the new block, so the - * flags always describe the bytes actually written. Re-encrypting at the - * new position would be equally valid; storing plain is the behaviour JMPQ3 - * has always had and what Warcraft III accepts. It was previously - * undocumented, and some paths left the old flags in place. - *

- * Everything else about the encoding — {@code COMPRESSED}, - * {@code IMPLODED}, {@code SINGLE_UNIT}, {@code SECTOR_CRC} — is preserved, - * and the stored bytes are copied through unchanged apart from decryption, - * which is length preserving. That keeps sector offset tables and - * per-sector checksums valid without re-encoding anything. - * - * @param newBlock block entry to fill in; its file position must already - * be set. - * @param writeBuffer destination, positioned where the file data starts. - * @throws JMpqException if the source data is inconsistent with its block. - */ - public void writeFileAndBlock(Block newBlock, ByteBuffer writeBuffer) throws JMpqException { - newBlock.setNormalSize(normalSize); - newBlock.setCompressedSize(compressedSize); - newBlock.setFlags((flags | EXISTS) & ~ENCRYPTION_FLAGS); - - if (normalSize == 0 || compressedSize == 0) { - newBlock.setCompressedSize(0); - return; - } - if (!isEncrypted) { - // Nothing to re-encode: hand the stored bytes straight through. - writeBuffer.put(readAt(0, compressedSize)); - return; - } - - // Decrypt in place, chunk by chunk, because each sector uses its own - // key. Chunk boundaries come from the sector offset table when the file - // has one. - if (block.hasSectorOffsetTable()) { - final int[] offsets = readSectorOffsets(); - final byte[] plain = new byte[compressedSize]; - - final byte[] table = readAt(0, offsets.length * 4); - decrypt(table, baseKey - 1); - System.arraycopy(table, 0, plain, 0, table.length); - - for (int i = 0; i < offsets.length - 1; i++) { - final int start = offsets[i]; - final int end = offsets[i + 1]; - validateSectorRange(i, start, end); - final byte[] sector = readAt(start, end - start); - decrypt(sector, baseKey + i); - System.arraycopy(sector, 0, plain, start, sector.length); - } - - // Any trailing bytes the offset table does not describe are copied - // verbatim so the block's compressed size stays truthful. - final int described = offsets[offsets.length - 1]; - if (described < compressedSize) { - final byte[] tail = readAt(described, compressedSize - described); - System.arraycopy(tail, 0, plain, described, tail.length); - } - writeBuffer.put(plain); - } else if (block.hasFlag(SINGLE_UNIT)) { - // One contiguous blob: a single key for the whole thing. - final byte[] data = readAt(0, compressedSize); - decrypt(data, baseKey); - writeBuffer.put(data); - } else { - // Stored without compression: no offset table, but still sectored, - // so each sector has its own key. Decrypting the whole block wrote - // a correct first sector and corrupt ones after it, and the new - // flags then claim the data is plain, making it permanent. - final byte[] data = readAt(0, compressedSize); - for (int i = 0, offset = 0; offset < data.length; i++, offset += sectorSize) { - final int length = Math.min(sectorSize, data.length - offset); - final byte[] sector = new byte[length]; - System.arraycopy(data, offset, sector, 0, length); - decrypt(sector, baseKey + i); - System.arraycopy(sector, 0, data, offset, length); - } - writeBuffer.put(data); - } - } - /** - * Encodes a new file into a rebuilt archive. - * - * @param file file content. - * @param b block entry to fill in; its file position and flags - * must already be set. - * @param buf destination. - * @param sectorSize archive sector size. - * @param recompress compression strategy. - */ - public static void writeFileAndBlock(byte[] file, Block b, ByteBuffer buf, int sectorSize, - RecompressOptions recompress) { - writeFileAndBlock(file, b, buf, sectorSize, "", recompress); - } - - /** - * Encodes a new file into a rebuilt archive. - * - * @param fileArr file content. - * @param b block entry to fill in; its file position and flags - * must already be set. - * @param buf destination. - * @param sectorSize archive sector size. - * @param pathlessName file name used to derive the encryption key, if the - * block asks for encryption. - * @param recompress compression strategy. - */ - public static void writeFileAndBlock(byte[] fileArr, Block b, ByteBuffer buf, int sectorSize, - String pathlessName, RecompressOptions recompress) { - b.setNormalSize(fileArr.length); - if (b.getFlags() == 0) { - if (fileArr.length > 0) { - b.setFlags(EXISTS | COMPRESSED); - } else { - b.setFlags(EXISTS); - b.setCompressedSize(0); - return; - } - } - if (fileArr.length == 0) { - b.setCompressedSize(0); - return; - } - - final int dataSectors = sectorCount(fileArr.length, sectorSize); - final int sotEntries = dataSectors + 1; - final int sotBytes = sotEntries * 4; - - // Resolve the sector key once instead of re-deriving it per sector, as - // the old code did in three separate places. - final int baseKey = MpqNames.sectorKey(pathlessName, b.getFlags(), b.getFilePosition(), b.getNormalSize()); - final boolean encrypt = b.hasFlag(ENCRYPTED); - - final ByteBuffer sot = ByteBuffer.allocate(sotBytes).order(ByteOrder.LITTLE_ENDIAN); - sot.putInt(sotBytes); - - final int dataStart = buf.position() + sotBytes; - buf.position(dataStart); - int sotPos = sotBytes; - - for (int i = 0; i < dataSectors; i++) { - final int from = i * sectorSize; - final int len = Math.min(sectorSize, fileArr.length - from); - final byte[] raw = new byte[len]; - System.arraycopy(fileArr, from, raw, 0, len); - - byte[] compressed = null; - try { - compressed = CompressionUtil.compress(raw, recompress); - } catch (ArrayIndexOutOfBoundsException ignored) { - // Codec could not handle this input; fall back to storing it. - } - - final byte[] payload; - if (compressed != null && compressed.length + 1 < raw.length) { - // Prefix the deflate compression indicator. - payload = new byte[compressed.length + 1]; - payload[0] = 0x02; - System.arraycopy(compressed, 0, payload, 1, compressed.length); - } else { - // Incompressible: store the sector as is. The sector's stored - // length then equals its natural length, which is how a reader - // knows there is no type byte. - payload = raw; - } - - if (encrypt) { - new MPQEncryption(baseKey + i, false).processSingle(ByteBuffer.wrap(payload)); - } - buf.put(payload); - sotPos += payload.length; - sot.putInt(sotPos); - } - - b.setCompressedSize(sotPos); - - final byte[] sotBytesOut = sot.array(); - if (encrypt) { - new MPQEncryption(baseKey - 1, false).processSingle(ByteBuffer.wrap(sotBytesOut)); - } - // Rewind to the slot reserved for the offset table and fill it in. - buf.position(dataStart - sotBytes); - buf.put(sotBytesOut); - buf.position(dataStart - sotBytes + sotPos); - } @Override public String toString() { From 7328cdf81dba0ffdb8d5f8788b615cbd8f8e761f Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 09:36:22 +0200 Subject: [PATCH 5/6] Keep the legacy write signatures; reserve only the generated listfile Three review findings. The P1 is a fair correction. I deleted the three public writeFileAndBlock overloads on the grounds that they took internal plumbing types and had no callers, but this release keeps systems.crigges.jmpq3 precisely so 1.x code goes on working, and removing public methods from it contradicts that. Whether a consumer ought to have called them is beside the point; binary compatibility does not care. Restored as deprecated adapters that delegate to MpqSectorWriter, so the signatures come back without the second encoder coming back with them. MpqSectorWriter is public for that reason and says so. The facade could accept an edit it could not persist: inserting (attributes), (signature) or (listfile) succeeded and then threw during close(), because the writer refused all three as reserved. Two separate mistakes were bundled there. Only (listfile) is actually the writer's to generate; nothing regenerates the other two, so refusing them meant they could never be preserved at all. The writer now reserves (listfile) alone, the other two can be written as ordinary files, and the facade refuses (listfile) at insertion time rather than at close, while the caller can still react. 1.x accepted it and wrote an archive with two entries under one name. A test asserted the wrong contract here and had to change: it expected all three names to be rejected, which is what made preserving attributes impossible. The migration guide pointed at unnamedBlockCount(), which reports zero for an archive whose (attributes) file a rebuild is about to discard. It now names filesLostOnRebuild() and says why. 150 tests, line coverage 80.54%. --- docs/migration-2.0.md | 7 +- .../java/org/inwc3/jmpq/MpqArchiveWriter.java | 29 +++-- .../java/org/inwc3/jmpq/MpqSectorWriter.java | 30 ++++- .../systems/crigges/jmpq3/JMpqEditor.java | 18 +++ .../java/systems/crigges/jmpq3/MpqFile.java | 118 ++++++++++++++++++ .../jmpq3test/BehaviourContractTests.java | 23 ++++ .../crigges/jmpq3test/ExtensionHookTests.java | 27 ++++ .../jmpq3test/MpqArchiveWriterTests.java | 18 ++- 8 files changed, 256 insertions(+), 14 deletions(-) diff --git a/docs/migration-2.0.md b/docs/migration-2.0.md index 50cca06..1d0710c 100644 --- a/docs/migration-2.0.md +++ b/docs/migration-2.0.md @@ -86,8 +86,11 @@ overloads taking a locale. 1.x registered everything as neutral and a rebuild dropped all but one variant. **An archive that cannot enumerate itself says so.** `MpqArchive.isEnumerable()` -and `unnamedBlockCount()` replace a log warning, so you can find out before a -rebuild how many files it would drop. +and `filesLostOnRebuild()` replace a log warning, so you can find out before a +rebuild how many files it would drop. Use `filesLostOnRebuild()`, not the +deprecated `unnamedBlockCount()`: the latter counts only nameless blocks, so it +reports zero for an archive whose `(attributes)` file a rebuild is about to +discard. **Malformed headers are repaired, not rejected.** A garbage header size is replaced by the version's real size and the archive opens with diff --git a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java index 0200afd..d617069 100644 --- a/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java +++ b/src/main/java/org/inwc3/jmpq/MpqArchiveWriter.java @@ -53,8 +53,24 @@ public final class MpqArchiveWriter { private static final int KEY_HASH_TABLE = tableKey("(hash table)"); private static final int KEY_BLOCK_TABLE = tableKey("(block table)"); - /** Internal files the writer owns and callers must not supply. */ - private static final List RESERVED = List.of("(listfile)", "(attributes)", "(signature)"); + /** + * Internal files the writer 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. + */ + private static final List GENERATED = List.of("(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 int tableKey(String name) { final MPQHashGenerator hasher = MPQHashGenerator.getFileKeyGenerator(); @@ -140,8 +156,7 @@ public static MpqArchiveWriter from(MpqArchive source, MpqWriteOptions options) writer.prefix = source.prefixBytes(); } for (String name : source.names()) { - if (RESERVED.contains(name)) { - // Regenerated rather than copied. + if (NOT_CARRIED_OVER.contains(name)) { continue; } // Every locale variant, not just the one a lookup resolves. @@ -273,10 +288,10 @@ private void requireUsableName(String name) { if (name == null || name.isEmpty()) { throw new IllegalArgumentException("A file name is required."); } - for (String reserved : RESERVED) { - if (reserved.equalsIgnoreCase(name)) { + for (String generated : GENERATED) { + if (generated.equalsIgnoreCase(name)) { throw new IllegalArgumentException(name + " is generated by the writer and cannot" - + " be supplied. Use MpqWriteOptions to control it."); + + " be supplied. Use MpqWriteOptions.withListfile to control it."); } } } diff --git a/src/main/java/org/inwc3/jmpq/MpqSectorWriter.java b/src/main/java/org/inwc3/jmpq/MpqSectorWriter.java index 0270932..48d16cd 100644 --- a/src/main/java/org/inwc3/jmpq/MpqSectorWriter.java +++ b/src/main/java/org/inwc3/jmpq/MpqSectorWriter.java @@ -10,12 +10,16 @@ /** * Encodes one file's content into an archive image as sectors. *

+ * Public only so the deprecated {@code MpqFile.writeFileAndBlock} overloads can + * delegate here rather than carrying a second copy of the encoder. New code + * should use {@link MpqArchiveWriter}. + *

* The layout produced is a sector offset table followed by the sectors it * describes. Each sector is compressed independently; a sector that does not * shrink is stored verbatim, which a reader detects because its stored length * equals its natural length. */ -final class MpqSectorWriter { +public final class MpqSectorWriter { /** Compression-type byte for deflate, the only codec this writer emits. */ private static final byte TYPE_DEFLATE = 0x02; @@ -119,6 +123,30 @@ private static byte[] encodeSector(byte[] raw, RecompressOptions recompress) { return payload; } + /** + * Encodes a file into a caller-supplied buffer. + *

+ * Exists for the deprecated {@code MpqFile.writeFileAndBlock} overloads, + * whose signatures take a {@link ByteBuffer}. Encoding happens here so + * there is still only one implementation of it. + * + * @param target destination, positioned where the file data starts. + * @param content the file's bytes. + * @param sectorSize the archive's sector size. + * @param name the file's path, for the encryption key. + * @param flags block flags. + * @param filePosition the file's offset relative to the archive header. + * @param recompress compression strategy. + * @return the number of bytes written. + */ + public static int writeInto(ByteBuffer target, byte[] content, int sectorSize, String name, + int flags, long filePosition, RecompressOptions recompress) { + final MpqImageBuffer staging = new MpqImageBuffer(Math.max(64, content.length + 64)); + final int written = write(staging, content, sectorSize, name, flags, filePosition, recompress); + target.put(staging.toByteArray(), 0, written); + return written; + } + /** * @param contentLength the file's decoded size. * @return the flags a newly encoded file should carry. diff --git a/src/main/java/systems/crigges/jmpq3/JMpqEditor.java b/src/main/java/systems/crigges/jmpq3/JMpqEditor.java index e586667..1e9abe9 100644 --- a/src/main/java/systems/crigges/jmpq3/JMpqEditor.java +++ b/src/main/java/systems/crigges/jmpq3/JMpqEditor.java @@ -513,6 +513,7 @@ public void deleteFile(String name) { */ public void insertByteArray(String name, byte[] input, boolean override) { requireWritable(); + rejectGeneratedName(name); requireAbsent(name, override); // Copy on insert: the caller may reuse its array afterwards. inserts.put(MpqNames.canonical(name), new Insert(name, input.clone(), null)); @@ -546,6 +547,7 @@ public void insertFile(String name, File file) throws IOException, IllegalArgume */ public void insertFile(String name, File file, boolean override) throws IOException { requireWritable(); + rejectGeneratedName(name); requireAbsent(name, override); log.debug("insert file: {}", name); // Stored as a path and read at close time, as 1.x did. @@ -559,6 +561,22 @@ private void requireWritable() { } } + /** + * The writer generates {@code (listfile)}, so supplying one would produce two + * entries under the same name. Rejected at insertion time rather than at + * close: 1.x accepted it and then wrote a broken archive, and failing when + * the caller can still do something about it is the lesser evil. + *

+ * {@code (attributes)} and {@code (signature)} are not generated, so they + * are accepted and written as ordinary files. + */ + private void rejectGeneratedName(String name) { + if ("(listfile)".equalsIgnoreCase(name)) { + throw new IllegalArgumentException("(listfile) is generated on close and cannot be" + + " inserted. Pass buildListfile to close(...) to control it."); + } + } + private void requireAbsent(String name, boolean override) { if (override || deleted.stream().anyMatch(gone -> sameName(gone, name))) { return; diff --git a/src/main/java/systems/crigges/jmpq3/MpqFile.java b/src/main/java/systems/crigges/jmpq3/MpqFile.java index 0972ced..f3175cc 100644 --- a/src/main/java/systems/crigges/jmpq3/MpqFile.java +++ b/src/main/java/systems/crigges/jmpq3/MpqFile.java @@ -2,6 +2,7 @@ import systems.crigges.jmpq3.BlockTable.Block; import systems.crigges.jmpq3.compression.CompressionUtil; +import systems.crigges.jmpq3.compression.RecompressOptions; import systems.crigges.jmpq3.security.MPQEncryption; import java.io.ByteArrayOutputStream; @@ -350,6 +351,123 @@ private void decrypt(byte[] data, int key) { + /** + * Copies this file into a rebuilt archive at a new position, filling in the + * new block table entry. + *

+ * Encryption policy: an encrypted file's sector key depends on its name and, + * for {@code ADJUSTED_ENCRYPTED} files, on its position and size, so + * relocating it invalidates the key. The sectors are therefore decrypted and + * stored plain, and the new block's encryption flags are cleared so they + * describe the bytes actually written. + * + * @param newBlock block entry to fill in; its file position must be set. + * @param writeBuffer destination, positioned where the file data starts. + * @throws JMpqException if the source data is inconsistent with its block. + * @deprecated use {@code MpqArchiveWriter}, which chooses between copying + * and re-encoding based on the target's sector size. Retained + * for source and binary compatibility with 1.x. + */ + @Deprecated + public void writeFileAndBlock(Block newBlock, ByteBuffer writeBuffer) throws JMpqException { + newBlock.setNormalSize(normalSize); + newBlock.setCompressedSize(compressedSize); + newBlock.setFlags((flags | EXISTS) & ~ENCRYPTION_FLAGS); + + if (normalSize == 0 || compressedSize == 0) { + newBlock.setCompressedSize(0); + return; + } + writeBuffer.put(storedBytesDecrypted()); + } + + /** + * Encodes a new file into a rebuilt archive. + * + * @param file file content. + * @param b block entry to fill in. + * @param buf destination. + * @param sectorSize archive sector size. + * @param recompress compression strategy. + * @deprecated use {@code MpqArchiveWriter}. Retained for compatibility. + */ + @Deprecated + public static void writeFileAndBlock(byte[] file, Block b, ByteBuffer buf, int sectorSize, + RecompressOptions recompress) { + writeFileAndBlock(file, b, buf, sectorSize, "", recompress); + } + + /** + * Encodes a new file into a rebuilt archive. + * + * @param fileArr file content. + * @param b block entry to fill in. + * @param buf destination. + * @param sectorSize archive sector size. + * @param pathlessName name used to derive the encryption key. + * @param recompress compression strategy. + * @deprecated use {@code MpqArchiveWriter}. Retained for compatibility. + */ + @Deprecated + public static void writeFileAndBlock(byte[] fileArr, Block b, ByteBuffer buf, int sectorSize, + String pathlessName, RecompressOptions recompress) { + b.setNormalSize(fileArr.length); + if (b.getFlags() == 0) { + b.setFlags(fileArr.length > 0 ? EXISTS | COMPRESSED : EXISTS); + } + if (fileArr.length == 0) { + b.setCompressedSize(0); + return; + } + // Delegates, so there is still one encoder rather than two. + b.setCompressedSize(org.inwc3.jmpq.MpqSectorWriter.writeInto( + buf, fileArr, sectorSize, pathlessName, b.getFlags(), b.getFilePosition(), recompress)); + } + + /** + * This file's stored bytes with any encryption removed. + * + * @return exactly {@link #getCompressedSize()} bytes. + * @throws JMpqException if the data is inconsistent with its block. + */ + private byte[] storedBytesDecrypted() throws JMpqException { + final byte[] stored = readAt(0, compressedSize); + if (!isEncrypted) { + return stored; + } + if (block.hasFlag(SINGLE_UNIT)) { + decrypt(stored, baseKey); + return stored; + } + if (!block.hasSectorOffsetTable()) { + // Stored without compression is still sectored, so each sector has + // its own key. See the note on extractStored. + for (int i = 0, offset = 0; offset < stored.length; i++, offset += sectorSize) { + final int length = Math.min(sectorSize, stored.length - offset); + final byte[] sector = new byte[length]; + System.arraycopy(stored, offset, sector, 0, length); + decrypt(sector, baseKey + i); + System.arraycopy(sector, 0, stored, offset, length); + } + return stored; + } + + 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++) { + final int start = offsets[i]; + final int end = offsets[i + 1]; + validateSectorRange(i, start, end); + final byte[] chunk = new byte[end - start]; + System.arraycopy(stored, start, chunk, 0, chunk.length); + decrypt(chunk, baseKey + i); + System.arraycopy(chunk, 0, stored, start, chunk.length); + } + return stored; + } + @Override public String toString() { return "MpqFile [sectorSize=" + sectorSize + ", compressedSize=" + compressedSize diff --git a/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java b/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java index b0090f9..7c70f84 100644 --- a/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java +++ b/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java @@ -241,6 +241,29 @@ public void closingTwiceIsHarmless() throws IOException { } } + /** + * The facade must not accept an edit it cannot persist. 1.x accepted a + * caller-supplied {@code (listfile)} and then wrote an archive with two + * entries under one name; failing at insertion, while the caller can still + * react, is the lesser evil. + */ + @Test + public void facadeRejectsAnInsertItCannotPersist() throws IOException { + Path mpq = TestResources.mpqCopy("normalMap"); + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.FORCE_V0)) { + Assert.expectThrows(IllegalArgumentException.class, + () -> editor.insertByteArray("(listfile)", new byte[1])); + + // The other internal files are not generated, so they are accepted + // and must survive the rebuild. + editor.insertByteArray("(attributes)", new byte[]{100, 0, 0, 0, 3, 0, 0, 0}, true); + } + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + Assert.assertTrue(archive.contains("(attributes)"), + "an explicitly supplied (attributes) file should be written"); + } + } + /** The writer refuses a format version it cannot write, at construction. */ @Test public void unwritableFormatVersionIsRefusedEarly() { diff --git a/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java b/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java index 41d7420..a98b565 100644 --- a/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java +++ b/src/test/java/systems/crigges/jmpq3test/ExtensionHookTests.java @@ -106,6 +106,33 @@ public void insufficientCapacityIsRefused() { Assert.expectThrows(IOException.class, writer::toByteArray); } + /** + * Only {@code (listfile)} is the writer's to generate. A caller holding + * {@code (attributes)} or {@code (signature)} bytes may write them as + * ordinary files, which is the only way to preserve them until attributes + * generation lands. + */ + @Test + public void onlyTheListfileIsReserved() throws IOException { + final byte[] attributes = new byte[]{100, 0, 0, 0, 3, 0, 0, 0}; + final byte[] image = MpqArchiveWriter.create(MpqWriteOptions.defaults()) + .put("(attributes)", attributes) + .put("(signature)", new byte[64]) + .put("war3map.j", "s".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + try (MpqArchive archive = MpqArchive.open(image, MpqOpenOptions.defaults())) { + Assert.assertEquals(archive.read("(attributes)"), attributes); + Assert.assertEquals(archive.read("(signature)").length, 64); + Assert.assertTrue(archive.contains("(listfile)")); + } + + // The generated one is refused, because supplying it would put two + // entries under one name. + Assert.expectThrows(IllegalArgumentException.class, + () -> MpqArchiveWriter.create(MpqWriteOptions.defaults()).put("(listfile)", new byte[1])); + } + /** Capacity must be a power of two, as the format requires. */ @Test public void capacityMustBeAPowerOfTwo() { diff --git a/src/test/java/systems/crigges/jmpq3test/MpqArchiveWriterTests.java b/src/test/java/systems/crigges/jmpq3test/MpqArchiveWriterTests.java index 800b5ab..4862df8 100644 --- a/src/test/java/systems/crigges/jmpq3test/MpqArchiveWriterTests.java +++ b/src/test/java/systems/crigges/jmpq3test/MpqArchiveWriterTests.java @@ -207,14 +207,24 @@ public void tableCapacityCanBeForced() throws IOException { () -> MpqWriteOptions.defaults().withHashTableCapacity(1000)); } - /** Reserved internal names are the writer's to generate. */ + /** + * Only the name the writer generates is refused. This test used to expect + * all three internal names to be rejected, which made (attributes) and + * (signature) impossible to preserve at all -- nothing regenerates them, so + * refusing them meant losing them. + */ @Test - public void reservedNamesAreRejected() { + public void onlyGeneratedNamesAreRejected() { final MpqArchiveWriter writer = MpqArchiveWriter.create(MpqWriteOptions.defaults()); - for (String reserved : List.of("(listfile)", "(ListFile)", "(attributes)", "(signature)")) { + for (String generated : List.of("(listfile)", "(ListFile)", "(LISTFILE)")) { Assert.expectThrows(IllegalArgumentException.class, - () -> writer.put(reserved, new byte[1])); + () -> writer.put(generated, new byte[1])); } + // Accepted, because the writer does not produce them itself. + writer.put("(attributes)", new byte[8]); + writer.put("(signature)", new byte[64]); + Assert.assertTrue(writer.contains("(attributes)")); + Assert.assertTrue(writer.contains("(signature)")); } /** put copies its input, so a later mutation cannot change what is written. */ From 1ab98df2eb12b5733b11cc263514ecdc249b972f Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 21 Aug 2026 09:47:37 +0200 Subject: [PATCH 6/6] Locale-blind overrides, no aliasing, accept an empty external listfile Final three review findings, all in the facade. An override through the 1.x API replaces the path as a whole, since that API has no locale parameter. Putting only the neutral variant left other locales behind holding stale content, so every variant is removed first. A writable in-memory editor no longer aliases the caller's array. It stays open across many calls and rebuilds at close, so a later mutation would change the live archive underneath it. Read-only editors still wrap without copying, because they never write. An empty external list file is accepted when the archive holds nothing a rebuild could lose. Requiring at least one resolved name was wrong twice over: it does not establish completeness, and it left a fresh archive unable to receive its first file. 153 tests. --- .../systems/crigges/jmpq3/JMpqEditor.java | 24 ++++-- .../jmpq3test/BehaviourContractTests.java | 82 +++++++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/src/main/java/systems/crigges/jmpq3/JMpqEditor.java b/src/main/java/systems/crigges/jmpq3/JMpqEditor.java index 1e9abe9..e7d7cf8 100644 --- a/src/main/java/systems/crigges/jmpq3/JMpqEditor.java +++ b/src/main/java/systems/crigges/jmpq3/JMpqEditor.java @@ -139,12 +139,16 @@ public JMpqEditor(Path mpqArchive, MPQOpenOption... openOptions) throws JMpqExce */ public JMpqEditor(byte[] mpqArchive, MPQOpenOption... openOptions) throws JMpqException { this.path = null; - this.memoryImage = mpqArchive; this.forceV0 = has(openOptions, MPQOpenOption.FORCE_V0); this.writeRequested = !has(openOptions, MPQOpenOption.READ_ONLY); this.canWrite = writeRequested; + // A writable editor stays open across many calls and rebuilds at close, + // so it must not alias the caller's array: mutating it afterwards would + // change the live archive underneath. A read-only editor never writes, + // so it can wrap and skip the copy. + this.memoryImage = writeRequested ? mpqArchive.clone() : mpqArchive; try { - this.archive = MpqArchive.open(mpqArchive, openOptions()); + this.archive = MpqArchive.open(memoryImage, openOptions()); } catch (JMpqException e) { throw e; } catch (IOException e) { @@ -255,9 +259,13 @@ public void setExternalListfile(File externalListfilePath) { log.debug("External listfile names <{}>, not held by the archive.", name); } } - // A list file that resolves nothing leaves the archive as it was, - // rather than claiming it became writable. - if (resolved > 0) { + // Parsing succeeded, so the question is whether the result is + // complete. Any resolved name means progress; none is still complete + // when the archive holds nothing a rebuild could lose, which is the + // case for an empty archive and its empty list file. Requiring a + // resolved name left such an archive read-only and unable to receive + // its first file. + if (resolved > 0 || archive.filesLostOnRebuild() == 0) { canWrite = true; } log.debug("Applied external listfile: {} of {} names resolved.", resolved, supplied.size()); @@ -690,8 +698,12 @@ private byte[] build(RecompressOptions options, boolean buildListfile) throws IO for (String gone : deleted) { writer.remove(gone); } - // Insertions last, so they win over whatever the archive held. + // Insertions last, so they win over whatever the archive held. Remove + // every locale variant of the path first: the 1.x API has no locale + // parameter, so an override means the path as a whole. Putting only the + // neutral variant would leave other locales behind with stale content. for (Insert insert : inserts.values()) { + writer.remove(insert.name()); if (insert.bytes() != null) { writer.put(insert.name(), insert.bytes()); } else { diff --git a/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java b/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java index 7c70f84..1b1a565 100644 --- a/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java +++ b/src/test/java/systems/crigges/jmpq3test/BehaviourContractTests.java @@ -264,6 +264,88 @@ public void facadeRejectsAnInsertItCannotPersist() throws IOException { } } + /** + * The 1.x API has no locale parameter, so an override replaces the path as a + * whole. Putting only the neutral variant would leave other locales behind + * holding stale content. + */ + @Test + public void legacyOverrideReplacesEveryLocaleVariant() throws IOException { + final short german = 0x407; + final byte[] image = MpqArchiveWriter.create(MpqWriteOptions.defaults()) + .put("war3map.wts", (short) 0, "old-neutral".getBytes(StandardCharsets.UTF_8)) + .put("war3map.wts", german, "old-german".getBytes(StandardCharsets.UTF_8)) + .toByteArray(); + + Path mpq = TestResources.scratchDir("locale-override").resolve("m.w3x"); + Files.write(mpq, image); + + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.FORCE_V0)) { + editor.insertByteArray("war3map.wts", "new".getBytes(StandardCharsets.UTF_8), true); + } + + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + Assert.assertEquals(archive.localesOf("war3map.wts").size(), 1, + "the override should have replaced the path, not one variant"); + Assert.assertEquals(new String(archive.read("war3map.wts"), StandardCharsets.UTF_8), + "new"); + } + } + + /** + * A writable in-memory editor must not alias the caller's array: it stays + * open across many calls and rebuilds at close, so a later mutation would + * change the live archive underneath it. + */ + @Test + public void writableInMemoryEditorDoesNotAliasTheInput() throws IOException { + final byte[] caller = Files.readAllBytes(TestResources.mpqCopy("normalMap")); + + JMpqEditor editor = new JMpqEditor(caller, MPQOpenOption.FORCE_V0); + // Scribble over the caller's array while the editor is open. + java.util.Arrays.fill(caller, (byte) 0); + editor.insertByteArray("added.txt", "x".getBytes(StandardCharsets.UTF_8)); + editor.close(); + + try (MpqArchive rebuilt = + MpqArchive.open(editor.getOutputByteArray(), MpqOpenOptions.warcraft3())) { + Assert.assertTrue(rebuilt.contains("added.txt")); + Assert.assertTrue(rebuilt.contains("war3map.j"), + "the original contents should have survived the caller's scribbling"); + } + } + + /** + * An empty external list file is complete for an archive that holds nothing, + * so it must restore writability. Requiring a resolved name left a fresh + * archive unable to receive its first file. + */ + @Test + public void emptyExternalListfileIsAcceptedWhenNothingIsAtRisk() throws IOException { + // An archive with no listfile and no files: nothing is at risk. + final byte[] image = MpqArchiveWriter + .create(MpqWriteOptions.defaults().withListfile(false)) + .toByteArray(); + Path mpq = TestResources.scratchDir("empty-external").resolve("m.w3x"); + Files.write(mpq, image); + + Path emptyListfile = TestResources.scratchDir("empty-lf").resolve("listfile.txt"); + Files.writeString(emptyListfile, ""); + + try (JMpqEditor editor = new JMpqEditor(mpq, MPQOpenOption.FORCE_V0)) { + Assert.assertFalse(editor.isCanWrite(), "no listfile means read-only to start"); + editor.setExternalListfile(emptyListfile.toFile()); + Assert.assertTrue(editor.isCanWrite(), + "an empty listfile is complete when the archive holds nothing"); + editor.insertByteArray("first.txt", "hello".getBytes(StandardCharsets.UTF_8)); + } + + try (MpqArchive archive = MpqArchive.open(mpq, MpqOpenOptions.warcraft3())) { + Assert.assertEquals(new String(archive.read("first.txt"), StandardCharsets.UTF_8), + "hello"); + } + } + /** The writer refuses a format version it cannot write, at construction. */ @Test public void unwritableFormatVersionIsRefusedEarly() {