Fix a signed overflow in the sector checksum, via the JDK intrinsic

The hand-written Adler-32 loop used int accumulators with zlib's NMAX fold
interval. zlib picks that interval so s2 cannot overflow an *unsigned*
32-bit accumulator; Java has no such type and a signed int overflows at
half that, so a sector of a few thousand high-valued bytes was checksummed
wrongly. Reachable in practice: the default recompression setting never
shrinks a sector, so raw bytes reach the checksum as they are.

Now computed with java.util.zip.Adler32 and corrected for the seed, which
differs by a closed form -- 1 in the low half, one per byte in the high
half. That is also the faster path, since Adler32.update is a HotSpot
intrinsic and a Java loop is not, and sectors can be 16 MiB.

The bug survived a round trip, a green suite and a review for the same
reason the seed itself did: reader and writer shared the wrong arithmetic
and agreed with each other. It took an independent implementation plus a
fixture with high-valued stored sectors, which is now exported for CI. The
old loop survives as the oracle the fast path is fuzzed against, with long
accumulators so it is actually correct.

Also aligns the hi-block table with StormLib: an archive declaring one it
cannot hold is reported rather than read with the low words alone, which
silently relocated every file. FORCE_V0 still opens such an archive, since
a version 0 header has no such field.

Author: Frotty

docs/mpq-format-notes.md

@@ -243,9 +243,31 @@ with `0` starts them at `s1 = 0, s2 = 0`. The two results differ by 1 in the low
 half and by the byte count in the high half — for every input, without
 exception.
 
-**Decision.** `MpqChecksums.adler32` implements the seeded-zero form.
-`java.util.zip.Adler32` cannot be used: it offers no way to seed, so it always
-computes the standard variant.
+**Decision.** `MpqChecksums.adler32` produces the seeded-zero form *via*
+`java.util.zip.Adler32`. The seeds differ by a closed form rather than anything
+structural — running the recurrence from `s1 = 1` adds 1 to the low half and one
+per byte to the high half — so the JDK's intrinsic does the work and the result
+is corrected:
+
+```
+s1(seed 0) = s1(seed 1) - 1          (mod 65521)
+s2(seed 0) = s2(seed 1) - n          (mod 65521)
+```
+
+The first implementation was a hand-written loop instead, and it was **wrong**.
+zlib chooses its `NMAX = 5552` fold interval so that `s2` cannot overflow an
+*unsigned* 32-bit accumulator; Java has no such type, and a signed `int`
+overflows at half that. A single sector of a few thousand high-valued bytes was
+therefore checksummed incorrectly — reachable in practice, because the default
+recompression setting never shrinks a sector, so raw bytes reach the checksum as
+they are.
+
+That bug survived a round trip, an all-green suite and a code review, for the
+same reason as the seed itself: the reader and the writer shared the wrong
+arithmetic and agreed with each other. `tools/mpqref.py` caught it once a
+fixture with high-valued stored sectors existed, which is now part of the
+exported set. The hand-written loop survives as `adler32Reference`, used only as
+the oracle the fast path is fuzzed against.
 
 This one is worth dwelling on, because no self-consistent test can catch it. A
 reader and a writer that both use the standard seed agree with each other on

src/main/java/org/inwc3/jmpq/MpqArchive.java

@@ -633,11 +633,18 @@ private int[] readHiBlockTable(int blockCount) throws IOException {
         }
         final long tableBytes = (long) blockCount * MpqHeader.HI_BLOCK_ENTRY_SIZE;
         if (!source.contains(header.hiBlockTableFileOffset(), tableBytes)) {
-            // The archive claims a hi-block table it does not hold. Reading the
-            // low words alone at least matches what a version 0 reader sees.
-            log.warn("{} declares a hi-block table at {} that does not fit; ignoring it.",
-                source.origin(), header.hiBlockTableFileOffset());
-            return new int[0];
+            // Reported, not worked around. An archive declaring a hi-block table
+            // is declaring that its file positions do not fit in 32 bits, so
+            // carrying on with the low words alone puts every file at the wrong
+            // offset -- reads that fail, or worse, succeed with wrong bytes.
+            // StormLib treats an unreadable hi-block table as fatal too
+            // (BuildFileTable_Classic sets dwErrCode and stops). A version 0
+            // archive never reaches here, so MPQOpenOption.FORCE_V0 remains the
+            // way to read one whose header claims a table it does not have.
+            throw new JMpqException("Archive declares a hi-block table at "
+                + header.hiBlockTableFileOffset() + " spanning " + tableBytes
+                + " bytes, which is not inside " + source.origin()
+                + ". Its file positions cannot be resolved.");
         }
         final int[] highWords = new int[blockCount];
         for (int i = 0; i < blockCount; i++) {

src/main/java/org/inwc3/jmpq/MpqChecksums.java

@@ -1,31 +1,47 @@
 package org.inwc3.jmpq;
 
+import java.util.zip.Adler32;
+
 /**
  * The Adler-32 variant MPQ sector checksums use.
  *
- * <h2>Why this is not {@link java.util.zip.Adler32}</h2>
+ * <h2>Why this is not plain {@link Adler32}</h2>
  * StormLib computes sector checksums as {@code adler32(0, buffer, length)} —
  * both when writing them ({@code SFileAddFile.cpp}) and when checking them
  * ({@code ReadMpqSectors} in {@code SFileReadFile.cpp}). Passing zlib a seed of
  * {@code 0} starts the accumulators at {@code s1 = 0, s2 = 0}, whereas a
- * standard Adler-32 — and so {@code java.util.zip.Adler32}, which offers no way
- * to seed it — starts at {@code s1 = 1}. The results differ by 1 in the low half
- * and by the byte count in the high half, for every input.
+ * standard Adler-32 — and so {@link Adler32}, which offers no way to seed it —
+ * starts at {@code s1 = 1}. The results differ by 1 in the low half and by the
+ * byte count in the high half, for every input.
  * <p>
  * That is a difference no self-consistent test can see: a reader and a writer
  * that both use the standard seed agree with each other perfectly and disagree
  * with every archive StormLib ever wrote. It was caught by
  * {@code tools/mpqref.py}, which computes the value independently, and is the
  * reason that cross-check exists.
+ *
+ * <h2>Getting it from the intrinsic anyway</h2>
+ * The two seeds differ by a closed form rather than by anything structural, so
+ * the JDK's implementation can still do the work. Running the recurrence from
+ * {@code s1 = 1} instead of {@code s1 = 0} adds exactly 1 to the low half, and
+ * adds 1 per byte to the high half:
+ * <pre>
+ * s1(seed 1) = s1(seed 0) + 1
+ * s2(seed 1) = s2(seed 0) + n
+ * </pre>
+ * So the seeded-zero value is recovered by subtracting those, modulo 65521.
+ * {@link Adler32#update(byte[], int, int)} is a HotSpot intrinsic, which a
+ * hand-written loop in Java is not — worth having when a single sector can be
+ * 16 MiB and every sector of every file passes through here.
  */
 final class MpqChecksums {
 
     /** Largest Adler-32 modulus below 65536. */
     private static final int BASE = 65521;
 
     /**
-     * Largest number of bytes that can be accumulated before {@code s2} could
-     * overflow a signed 32-bit int. zlib calls this {@code NMAX}.
+     * Largest number of bytes the reference implementation accumulates before
+     * reducing. zlib calls this {@code NMAX}.
      */
     private static final int NMAX = 5552;
 
@@ -48,8 +64,38 @@ static int adler32(byte[] data) {
      * @return the sector checksum MPQ records.
      */
     static int adler32(byte[] data, int offset, int length) {
-        int s1 = 0;
-        int s2 = 0;
+        final Adler32 standard = new Adler32();
+        standard.update(data, offset, length);
+        final int seededOne = (int) standard.getValue();
+
+        // Undo the seed: 1 from the low half, one per byte from the high half.
+        final int low = Math.floorMod((seededOne & 0xFFFF) - 1, BASE);
+        final int high = Math.floorMod(((seededOne >>> 16) & 0xFFFF) - length % BASE, BASE);
+        return (high << 16) | low;
+    }
+
+    /**
+     * The definition, computed directly.
+     * <p>
+     * Kept as the oracle {@code MpqChecksumTests} checks {@link #adler32}
+     * against, so the seed correction above cannot drift from what it claims to
+     * compute. Not used in production: it is the same arithmetic without the
+     * intrinsic.
+     *
+     * @param data   bytes to checksum.
+     * @param offset first byte to include.
+     * @param length how many bytes to include.
+     * @return the sector checksum MPQ records.
+     */
+    static int adler32Reference(byte[] data, int offset, int length) {
+        // long accumulators, deliberately. zlib picks NMAX so that s2 cannot
+        // overflow an *unsigned* 32-bit accumulator; Java has no such type, and
+        // a signed int overflows at half that. An earlier version of this method
+        // was the production path and used int, so it silently produced wrong
+        // checksums for a sector of a few thousand high-valued bytes -- reachable
+        // as soon as an archive uses a sector size above the 4 KiB default.
+        long s1 = 0;
+        long s2 = 0;
         int at = offset;
         int remaining = length;
 
@@ -63,6 +109,6 @@ static int adler32(byte[] data, int offset, int length) {
             s2 %= BASE;
             remaining -= block;
         }
-        return (s2 << 16) | s1;
+        return (int) ((s2 << 16) | s1);
     }
 }

src/main/java/org/inwc3/jmpq/MpqHeader.java

@@ -692,9 +692,10 @@ private static MpqHeader parseAt(MpqSource source, Located located, boolean forc
 
         if (hiBlockTablePosition < 0
             || (hiBlockTablePosition != 0 && !source.contains(offset + hiBlockTablePosition, 0))) {
-            // A position outside the file cannot be a table. Dropping it leaves
-            // the low words, which is what a version 0 reader would use.
-            hiBlockTablePosition = 0;
+            // Kept, not dropped. Dropping it left the low words in place, which
+            // silently relocates every file in an archive that needs the high
+            // ones; MpqArchive reports it instead, as StormLib does. Recording
+            // that the header is malformed is still worth doing.
             malformed = true;
         }
 

src/test/java/systems/crigges/jmpq3test/MpqChecksumTests.java

@@ -0,0 +1,99 @@
+package systems.crigges.jmpq3test;
+
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.lang.reflect.Method;
+import java.nio.charset.StandardCharsets;
+import java.util.Random;
+
+/**
+ * The seeded-zero Adler-32 that MPQ sector checksums use.
+ * <p>
+ * Two things need pinning. That the value matches what StormLib computes, which
+ * is what the literal constants below are for; and that the fast path -- the
+ * JDK intrinsic plus a seed correction -- agrees with the definition computed
+ * directly, which is what the fuzz comparison is for. Deriving one from the
+ * other is an algebraic shortcut, and a shortcut nobody checks is a bug waiting
+ * to happen.
+ */
+public class MpqChecksumTests {
+
+    private static Method fast;
+    private static Method reference;
+
+    private static void load() throws Exception {
+        if (fast == null) {
+            final Class<?> type = Class.forName("org.inwc3.jmpq.MpqChecksums");
+            fast = type.getDeclaredMethod("adler32", byte[].class, int.class, int.class);
+            reference = type.getDeclaredMethod("adler32Reference", byte[].class, int.class, int.class);
+            fast.setAccessible(true);
+            reference.setAccessible(true);
+        }
+    }
+
+    private static int fast(byte[] data, int offset, int length) throws Exception {
+        load();
+        return (int) fast.invoke(null, data, offset, length);
+    }
+
+    private static int reference(byte[] data, int offset, int length) throws Exception {
+        load();
+        return (int) reference.invoke(null, data, offset, length);
+    }
+
+    /** Values taken from {@code zlib.adler32(data, 0)}, not from this code. */
+    @Test
+    public void knownValuesMatchZlibSeededWithZero() throws Exception {
+        final byte[] abc = "abc".getBytes(StandardCharsets.UTF_8);
+        Assert.assertEquals(fast(abc, 0, abc.length), 0x024A0126,
+            "seeding with 1 would give 0x024D0127");
+        Assert.assertEquals(fast(new byte[0], 0, 0), 0);
+
+        final byte[] many = new byte[10_000];
+        java.util.Arrays.fill(many, (byte) 'a');
+        Assert.assertEquals(fast(many, 0, many.length), 0x78ABCDE2,
+            "long enough to cross the block boundary the accumulator folds at");
+    }
+
+    /**
+     * The intrinsic-plus-correction path against the definition, over lengths
+     * that straddle every boundary that matters: empty, one byte, and either
+     * side of zlib's 5552-byte fold.
+     */
+    @Test
+    public void theFastPathAgreesWithTheDefinition() throws Exception {
+        final Random random = new Random(11);
+        final int[] lengths = {0, 1, 2, 15, 16, 255, 4096, 5551, 5552, 5553, 11_104, 40_000};
+
+        for (int length : lengths) {
+            final byte[] data = new byte[length];
+            random.nextBytes(data);
+            Assert.assertEquals(fast(data, 0, length), reference(data, 0, length),
+                "length " + length);
+
+            // All-zero and all-0xFF exercise the modulus at both extremes.
+            Assert.assertEquals(fast(new byte[length], 0, length),
+                reference(new byte[length], 0, length), "zeroes, length " + length);
+            final byte[] high = new byte[length];
+            java.util.Arrays.fill(high, (byte) 0xFF);
+            Assert.assertEquals(fast(high, 0, length), reference(high, 0, length),
+                "0xFF, length " + length);
+        }
+    }
+
+    /** Offsets and lengths inside a larger array must be honoured. */
+    @Test
+    public void slicesAreHonoured() throws Exception {
+        final Random random = new Random(12);
+        final byte[] data = new byte[8192];
+        random.nextBytes(data);
+
+        for (int offset : new int[]{0, 1, 7, 4095}) {
+            for (int length : new int[]{0, 1, 100, 4000}) {
+                Assert.assertEquals(fast(data, offset, length), reference(data, offset, length),
+                    "offset " + offset + " length " + length);
+            }
+        }
+    }
+}

src/test/java/systems/crigges/jmpq3test/Phase2FormatTests.java

@@ -538,12 +538,21 @@ public void aNonZeroHiBlockEntryMovesTheFilePosition() throws IOException {
     }
 
     /**
-     * An archive claiming a hi-block table it does not hold is read with the low
-     * words alone, which is what a version 0 reader would do anyway. Refusing it
-     * would lose an archive that is entirely readable.
+     * An archive claiming a hi-block table it does not hold is reported, not
+     * quietly read with the low words alone.
+     * <p>
+     * Ignoring the table looked like leniency and was not: declaring one means
+     * the file positions do not fit in 32 bits, so dropping the high words puts
+     * every file at the wrong offset — reads that fail, or worse, succeed with
+     * the wrong bytes. StormLib treats an unreadable hi-block table as fatal for
+     * the same reason.
+     * <p>
+     * The escape hatch is real rather than notional: a version 0 archive never
+     * consults the field, so a Warcraft III map whose header was corrupted into
+     * claiming one still opens under {@code FORCE_V0}.
      */
     @Test
-    public void aHiBlockTableOutsideTheFileIsIgnored() throws IOException {
+    public void aHiBlockTableOutsideTheFileIsReported() throws IOException {
         final byte[] plain = MpqArchiveWriter.create(MpqWriteOptions.defaults()
                 .withFormatVersion(1))
             .put("a.txt", "content".getBytes(StandardCharsets.UTF_8))
@@ -553,13 +562,36 @@ public void aHiBlockTableOutsideTheFileIsIgnored() throws IOException {
         final int headerAt = headerOffset(plain);
         edit.putLong(headerAt + 0x20, 0x7FFF_FFFFL);
 
-        try (MpqArchive archive = MpqArchive.open(plain, MpqOpenOptions.defaults())) {
-            Assert.assertFalse(archive.header().hasHiBlockTable(), "dropped as implausible");
-            Assert.assertTrue(archive.header().malformed());
+        final JMpqException thrown = Assert.expectThrows(JMpqException.class,
+            () -> MpqArchive.open(plain, MpqOpenOptions.defaults()).close());
+        Assert.assertTrue(thrown.getMessage().contains("hi-block table"), thrown.getMessage());
+
+        // Read as version 0, the field is not part of the header at all.
+        try (MpqArchive archive = MpqArchive.open(plain, MpqOpenOptions.warcraft3())) {
+            Assert.assertFalse(archive.header().hasHiBlockTable());
             Assert.assertEquals(archive.read("a.txt"), "content".getBytes(StandardCharsets.UTF_8));
         }
     }
 
+    /** A table whose position is in the file but which runs off the end, likewise. */
+    @Test
+    public void aTruncatedHiBlockTableIsReported() throws IOException {
+        final byte[] plain = MpqArchiveWriter.create(MpqWriteOptions.defaults()
+                .withFormatVersion(1))
+            .put("a.txt", "content".getBytes(StandardCharsets.UTF_8))
+            .toByteArray();
+
+        // One byte before the end: inside the file, but far too small to hold
+        // one entry per block.
+        final ByteBuffer edit = ByteBuffer.wrap(plain).order(ByteOrder.LITTLE_ENDIAN);
+        final int headerAt = headerOffset(plain);
+        edit.putLong(headerAt + 0x20, plain.length - headerAt - 1);
+
+        final JMpqException thrown = Assert.expectThrows(JMpqException.class,
+            () -> MpqArchive.open(plain, MpqOpenOptions.defaults()).close());
+        Assert.assertTrue(thrown.getMessage().contains("hi-block table"), thrown.getMessage());
+    }
+
     /**
      * Appends a hi-block table to a version 1 archive and points the header at
      * it. The table is neither encrypted nor compressed, per StormLib.
@@ -723,11 +755,25 @@ public void exportForReferenceVerification() throws IOException {
 
         final StringBuilder expected = new StringBuilder("# archive\tname\tsize\tmd5\n");
 
+        // 8 KiB sectors of incompressible data are stored raw, so a whole
+        // sector of high-valued bytes reaches the checksum. That is what
+        // overflows a signed 32-bit Adler accumulator -- and it is invisible to a
+        // round trip, because both sides would share the same wrong arithmetic.
+        // Only the reference can see it, which is why this shape is exported.
+        files.put("wide.bin", incompressible(40_000, 21));
+        // High-valued bytes, stored raw: the default recompression setting never
+        // shrinks a sector, so these reach the checksum as they are. A run of
+        // 0xFF is what pushes the Adler accumulator past a signed 32-bit range.
+        final byte[] high = new byte[40_000];
+        java.util.Arrays.fill(high, (byte) 0xFF);
+        files.put("high.bin", high);
+
         final MpqWriteOptions[] shapes = {
             MpqWriteOptions.defaults().withSectorChecksums(true),
             MpqWriteOptions.defaults().withAttributes(true).withAttributesTimestamp(0),
             MpqWriteOptions.defaults().withSectorChecksums(true).withAttributes(true)
                 .withAttributesTimestamp(0).withFormatVersion(1),
+            MpqWriteOptions.defaults().withSectorChecksums(true).withSectorSizeShift(4),
         };
 
         for (int shape = 0; shape < shapes.length; shape++) {