Reject malformed MVV regions before the stale-mark sweep

The up-front sweep introduced for issue #292 followed length fields
with no alignment validation: on an MVV with a corrupt length field
the traversal landed inside a value and unmark() cleared bit 15 of a
payload byte — inside the region, before the first pass's guard could
reject it. Hoist the verify() call that already runs on every prune
and sweep only well-formed regions; a malformed region is left to the
first pass's guard, which throws without writing anything.

Author: vharseko

persistit/core/src/main/java/com/persistit/MVV.java

@@ -381,11 +381,13 @@ else if (target[targetOffset] != TYPE_MVV_BYTE) {
      * </p>
      * <p>
      * This method leaves the byte array unchanged if any of its checked
-     * Exceptions is thrown, with one exception: stale mark bits left on disk
-     * by a prune interrupted before the issue #286 fix are cleared up front
-     * regardless of outcome (issue #292). Mark bits are transient state
-     * private to this method and invisible to the read paths, so clearing
-     * them repairs the corruption without altering any version's value.
+     * Exceptions is thrown, with one exception: on a well-formed MVV, stale
+     * mark bits left on disk by a prune interrupted before the issue #286 fix
+     * are cleared up front regardless of outcome (issue #292). Mark bits are
+     * transient state private to this method and invisible to the read paths,
+     * so clearing them repairs the corruption without altering any version's
+     * value. A malformed MVV is never swept — it is rejected with a
+     * <code>CorruptValueException</code> and left unchanged.
      * </p>
      * <p>
      * This method adds {@link PrunedVersion} instances to the supplied list.
@@ -424,7 +426,8 @@ static int prune(final byte[] bytes, final int offset, final int length, final T
             return length;
         }
 
-        Debug.$assert0.t(verify(bytes, offset, length));
+        final boolean wellFormed = verify(bytes, offset, length);
+        Debug.$assert0.t(wellFormed);
 
         boolean primordial = convertToPrimordial;
         int marked = 0;
@@ -448,13 +451,21 @@ static int prune(final byte[] bytes, final int offset, final int length, final T
              * primordial-conversion scan — promoting an obsolete version and
              * silently dropping the current one with no PrunedVersion
              * accounting (issue #292). Sweep the region clean before the
-             * first pass marks the real keepers.
+             * first pass marks the real keepers. This is a separate pass, not
+             * an unmark() at the first pass's loop head, so the whole region
+             * is repaired even when that pass exits early via an exception.
+             * Only a well-formed region may be swept: a corrupt length field
+             * would misalign the traversal and unmark() would clear a bit
+             * inside a value payload. On a malformed region skip the sweep —
+             * the first pass's guard throws for it anyway.
              */
-            while (from + LENGTH_PER_VERSION <= offset + length) {
-                unmark(bytes, from);
-                from += getLength(bytes, from) + LENGTH_PER_VERSION;
+            if (wellFormed) {
+                int sweep = offset + 1;
+                while (sweep + LENGTH_PER_VERSION <= offset + length) {
+                    unmark(bytes, sweep);
+                    sweep += getLength(bytes, sweep) + LENGTH_PER_VERSION;
+                }
             }
-            from = offset + 1;
             /*
              * First pass - mark all the versions to keep. Keep every
              * UNCOMMITTED version (there may be more than one created by the

persistit/core/src/test/java/com/persistit/MVVTest.java

@@ -586,7 +586,7 @@ public void pruneExceptionUnmarksVersionsAtNonZeroOffset() throws Exception {
             fail("expected CorruptValueException");
         } catch (final CorruptValueException expected) {
         }
-        assertArrayEquals("prune must leave the byte array unchanged when it throws", before, bytes);
+        assertArrayEquals("prune must leave an MVV without stale marks unchanged when it throws", before, bytes);
     }
 
     /**
@@ -740,6 +740,44 @@ public void pruneStaleMarkedVersionPrunedInMultiVersionPath() throws Exception {
         }
     }
 
+    /**
+     * The up-front sweep must not follow a corrupted length field: a
+     * misaligned traversal lands inside a value and unmark() clears bit 15 of
+     * a payload byte — inside the MVV region, past what
+     * {@link #pruneExceptionMustNotUnmarkPastRegionEnd} guards. Prune must
+     * reject the malformed region without writing anything.
+     */
+    @Test
+    public void pruneMustNotSweepMisalignedMvv() throws Exception {
+        final TimestampAllocator tsa = new TimestampAllocator();
+        final TransactionIndex ti = new TransactionIndex(tsa, 1);
+
+        /*
+         * Two aborted versions, so no pass marks anything and any byte that
+         * differs after prune was written by the sweep. The 0xFF fill of the
+         * second value makes a misaligned unmark() visible (bit 15 cleared).
+         */
+        final int offset = 7;
+        final byte[] bytes = new byte[offset + 100];
+        final byte[] v1 = { 0x1, 0x2, 0x3, 0x4 };
+        final byte[] v2 = new byte[20];
+        Arrays.fill(v2, (byte) 0xFF);
+        int length = storeAbortedVersion(tsa, ti, bytes, offset, -1, v1);
+        length = storeAbortedVersion(tsa, ti, bytes, offset, length, v2);
+        ti.updateActiveTransactionCache();
+
+        /* Corrupt the first version's length field: it claims 9 bytes, not 4. */
+        MVV.putLength(bytes, offset + 1, 9);
+
+        final byte[] before = bytes.clone();
+        try {
+            MVV.prune(bytes, offset, length, ti, true, new ArrayList<MVV.PrunedVersion>());
+            fail("expected CorruptValueException");
+        } catch (final CorruptValueException expected) {
+        }
+        assertArrayEquals("prune must not write into a misaligned MVV region", before, bytes);
+    }
+
     //
     // Test helper methods
     //
@@ -760,6 +798,22 @@ private static int storeCommittedVersion(final TimestampAllocator tsa, final Tra
         return newLength;
     }
 
+    /**
+     * Store <code>value</code> as a new version created by a registered
+     * transaction and abort it, so that {@link MVV#prune} sees an aborted
+     * version and marks nothing.
+     */
+    private static int storeAbortedVersion(final TimestampAllocator tsa, final TransactionIndex ti,
+            final byte[] bytes, final int offset, final int length, final byte[] value) throws Exception {
+        final TransactionStatus status = ti.registerTransaction();
+        final int newLength = MVV.storeVersion(bytes, offset, length, bytes.length,
+                TransactionIndex.ts2vh(status.getTs()), value, 0, value.length) & STORE_LENGTH_MASK;
+        status.incrementMvvCount();
+        status.abort();
+        ti.notifyCompleted(status, tsa.updateTimestamp());
+        return newLength;
+    }
+
     private static int writeArray(final byte[] array, final int... contents) {
         assert contents.length <= array.length : "Too many values for array";
         for (int i = 0; i < contents.length; ++i) {