Fix bug in Java9IntHash calculation

- Java9IntHash uses private methods from java.util.zip.CRC32C class, updateBytes and updateDirectByteBuffer. When inspecting the use and interface contract, it doesn't match how it is used in Java9IntHash. This PR addresses that by introducing a separate initial value for initializing the accumulated value so that the initial value could match the logic in java.util.zip.CRC32C.reset method. There's also a separate method for finalizing the accumulated value into a final checksum value. This is to match the java.util.zip.CRC32C.getValue method's logic (uses bitwise complement operator ~).
lhotari · Feb 2, 2024 · 9df74ef · 9df74ef
1 parent fc93b5e
commit 9df74ef
Show file tree

Hide file tree

Showing 8 changed files with 101 additions and 28 deletions.
diff --git a/...keeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java b/...keeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32CDigestManager.java
@@ -54,4 +54,14 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) {
     int internalUpdate(int digest, byte[] buffer, int offset, int len) {
         return Crc32cIntChecksum.resumeChecksum(digest, buffer, offset, len);
     }
-}
+
+    @Override
+    protected int initialDigest() {
+        return Crc32cIntChecksum.initialValue();
+    }
+
+    @Override
+    protected int finalizeDigest(int digest) {
+        return Crc32cIntChecksum.finalizeValue(digest);
+    }
+}
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java
@@ -61,12 +61,31 @@ public abstract class DigestManager {
 
     abstract int internalUpdate(int digest, byte[] buffer, int offset, int len);
 
+    protected int initialDigest() {
+        return 0;
+    };
+
+    protected int finalizeDigest(int digest) {
+        return digest;
+    }
+
     final int update(int digest, ByteBuf buffer, int offset, int len) {
+        if (len == 0) {
+            return digest;
+        }
         MutableInt digestRef = new MutableInt(digest);
         ByteBufVisitor.visitBuffers(buffer, offset, len, byteBufVisitorCallback, digestRef);
         return digestRef.intValue();
     }
 
+    final protected int initializeAndUpdate(ByteBuf buf, int offset, int len) {
+        return update(initialDigest(), buf, offset, len);
+    }
+
+    private int updateAndFinalize(ByteBuf data, int digest, int offset, int len) {
+        return finalizeDigest(update(digest, data, offset, len));
+    }
+
     abstract void populateValueAndReset(int digest, ByteBuf buffer);
 
     abstract boolean isInt32Digest();
@@ -145,8 +164,8 @@ private ReferenceCounted computeDigestAndPackageForSendingV2(long entryId, long
         buf.writeLong(length);
 
         // Compute checksum over the headers
-        int digest = update(0, buf, buf.readerIndex(), buf.readableBytes());
-        digest = update(digest, data, data.readerIndex(), data.readableBytes());
+        int digest = initializeAndUpdate(buf, buf.readerIndex(), buf.readableBytes());
+        digest = updateAndFinalize(data, digest, data.readerIndex(), data.readableBytes());
 
         populateValueAndReset(digest, buf);
 
@@ -170,8 +189,8 @@ private ByteBufList computeDigestAndPackageForSendingV3(long entryId, long lastA
         headersBuffer.writeLong(lastAddConfirmed);
         headersBuffer.writeLong(length);
 
-        int digest = update(0, headersBuffer, 0, METADATA_LENGTH);
-        digest = update(digest, data, data.readerIndex(), data.readableBytes());
+        int digest = initializeAndUpdate(headersBuffer, 0, METADATA_LENGTH);
+        digest = updateAndFinalize(data, digest, data.readerIndex(), data.readableBytes());
         populateValueAndReset(digest, headersBuffer);
         return ByteBufList.get(headersBuffer, data);
     }
@@ -193,7 +212,7 @@ public ByteBufList computeDigestAndPackageForSendingLac(long lac) {
         headersBuffer.writeLong(ledgerId);
         headersBuffer.writeLong(lac);
 
-        int digest = update(0, headersBuffer, 0, LAC_METADATA_LENGTH);
+        int digest = finalizeDigest(initializeAndUpdate(headersBuffer, 0, LAC_METADATA_LENGTH));
         populateValueAndReset(digest, headersBuffer);
 
         return ByteBufList.get(headersBuffer);
@@ -229,10 +248,10 @@ private void verifyDigest(long entryId, ByteBuf dataReceived, boolean skipEntryI
                     this.getClass().getName(), dataReceived.readableBytes());
             throw new BKDigestMatchException();
         }
-        int digest = update(0, dataReceived, 0, METADATA_LENGTH);
+        int digest = initializeAndUpdate(dataReceived, 0, METADATA_LENGTH);
 
         int offset = METADATA_LENGTH + macCodeLength;
-        digest = update(digest, dataReceived, offset, dataReceived.readableBytes() - offset);
+        digest = updateAndFinalize(dataReceived, digest, offset, dataReceived.readableBytes() - offset);
 
         if (isInt32Digest()) {
             int receivedDigest = dataReceived.getInt(METADATA_LENGTH);
@@ -277,7 +296,7 @@ public long verifyDigestAndReturnLac(ByteBuf dataReceived) throws BKDigestMatchE
             throw new BKDigestMatchException();
         }
 
-        int digest = update(0, dataReceived, 0, LAC_METADATA_LENGTH);
+        int digest = finalizeDigest(initializeAndUpdate(dataReceived, 0, LAC_METADATA_LENGTH));
 
         if (isInt32Digest()) {
             int receivedDigest = dataReceived.getInt(LAC_METADATA_LENGTH);

diff --git a/...est/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java b/...est/java/org/apache/bookkeeper/proto/checksum/CompositeByteBufUnwrapBugReproduceTest.java
@@ -129,6 +129,16 @@ int internalUpdate(int digest, ByteBuf data, int offset, int len) {
         int internalUpdate(int digest, byte[] buffer, int offset, int len) {
             return intHash.resume(digest, buffer, offset, len);
         }
+
+        @Override
+        protected int initialDigest() {
+            return intHash.initialValue();
+        }
+
+        @Override
+        protected int finalizeDigest(int digest) {
+            return intHash.finalizeValue(digest);
+        }
     }
 
     @Test

diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Crc32cIntChecksum.java
@@ -94,4 +94,11 @@ public static int resumeChecksum(int previousChecksum, byte[] payload, int offse
         return CRC32C_HASH.resume(previousChecksum, payload, offset, len);
     }
 
+    public static int initialValue() {
+        return CRC32C_HASH.initialValue();
+    }
+
+    public static int finalizeValue(int current) {
+        return CRC32C_HASH.finalizeValue(current);
+    }
 }
diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/IntHash.java
@@ -30,4 +30,12 @@ public interface IntHash {
     int resume(int current, ByteBuf buffer, int offset, int len);
 
     int resume(int current, byte[] buffer, int offset, int len);
+
+    default int initialValue() {
+        return 0;
+    }
+
+    default int finalizeValue(int current) {
+        return current;
+    }
 }
diff --git a/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java b/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java
@@ -68,12 +68,12 @@ protected byte[] initialValue() {
 
     @Override
     public int calculate(ByteBuf buffer) {
-        return resume(0, buffer);
+        return finalizeValue(resume(initialValue(), buffer));
     }
 
     @Override
     public int calculate(ByteBuf buffer, int offset, int len) {
-        return resume(0, buffer, offset, len);
+        return finalizeValue(resume(initialValue(), buffer, offset, len));
     }
 
     private int resume(int current, long address, int offset, int length) {
@@ -100,26 +100,35 @@ public int resume(int current, ByteBuf buffer) {
 
     @Override
     public int resume(int current, ByteBuf buffer, int offset, int len) {
-        int negCrc = ~current;
-
         if (buffer.hasMemoryAddress()) {
-            negCrc = resume(negCrc, buffer.memoryAddress(), offset, len);
+            current = resume(current, buffer.memoryAddress(), offset, len);
         } else if (buffer.hasArray()) {
             int arrayOffset = buffer.arrayOffset() + offset;
-            negCrc = resume(negCrc, buffer.array(), arrayOffset, len);
+            current = resume(current, buffer.array(), arrayOffset, len);
         } else {
             byte[] b = TL_BUFFER.get();
             int toRead = len;
             int loopOffset = offset;
             while (toRead > 0) {
                 int length = Math.min(toRead, b.length);
                 buffer.slice(loopOffset, length).readBytes(b, 0, length);
-                negCrc = resume(negCrc, b, 0, length);
+                current = resume(current, b, 0, length);
                 toRead -= length;
                 loopOffset += length;
             }
         }
+        return current;
+    }
+
+    @Override
+    public int initialValue() {
+        // match java.util.zip.CRC32C.reset logic
+        return 0xFFFFFFFF;
+    }
 
-        return ~negCrc;
+    @Override
+    public int finalizeValue(int current) {
+        // match java.util.zip.CRC32C.getValue logic
+        return ~current;
     }
 }
diff --git a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/ChecksumTest.java
@@ -16,6 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+
 package com.scurrilous.circe.checksum;
 
 import static com.scurrilous.circe.params.CrcParameters.CRC32C;
@@ -44,7 +45,9 @@ public void testCrc32cValue() {
     @Test
     public void testCrc32cValueResume() {
         final byte[] bytes = "Some String".getBytes();
-        int checksum = Crc32cIntChecksum.resumeChecksum(0, Unpooled.wrappedBuffer(bytes), 0, bytes.length);
+        int checksum = Crc32cIntChecksum.finalizeValue(
+                Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes), 0,
+                        bytes.length));
 
         assertEquals(608512271, checksum);
     }
@@ -56,22 +59,26 @@ public void testCrc32cValueIncremental() {
         int checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes));
         assertEquals(608512271, checksum);
 
-        checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes, 0, 1));
+        checksum =
+                Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes, 0, 1));
         for (int i = 1; i < bytes.length; i++) {
             checksum = Crc32cIntChecksum.resumeChecksum(checksum, Unpooled.wrappedBuffer(bytes), i, 1);
         }
+        checksum = Crc32cIntChecksum.finalizeValue(checksum);
         assertEquals(608512271, checksum);
 
-        checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes, 0, 4));
+        checksum =
+                Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes, 0, 4));
         checksum = Crc32cIntChecksum.resumeChecksum(checksum, Unpooled.wrappedBuffer(bytes), 4, 7);
+        checksum = Crc32cIntChecksum.finalizeValue(checksum);
         assertEquals(608512271, checksum);
 
 
         ByteBuf buffer = Unpooled.wrappedBuffer(bytes, 0, 4);
-        checksum = Crc32cIntChecksum.computeChecksum(buffer);
+        checksum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), buffer);
         checksum = Crc32cIntChecksum.resumeChecksum(
-            checksum, Unpooled.wrappedBuffer(bytes), 4, bytes.length - 4);
-
+                checksum, Unpooled.wrappedBuffer(bytes), 4, bytes.length - 4);
+        checksum = Crc32cIntChecksum.finalizeValue(checksum);
         assertEquals(608512271, checksum);
     }
 
@@ -86,7 +93,10 @@ public void testCrc32cLongValue() {
     @Test
     public void testCrc32cLongValueResume() {
         final byte[] bytes = "Some String".getBytes();
-        long checksum = Crc32cIntChecksum.resumeChecksum(0, Unpooled.wrappedBuffer(bytes), 0, bytes.length);
+        long checksum =
+                Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes), 0,
+                        bytes.length);
+        checksum = Crc32cIntChecksum.finalizeValue((int) checksum);
 
         assertEquals(608512271L, checksum);
     }

diff --git a/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java b/circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java
@@ -65,8 +65,8 @@ public void calculateCheckSumUsingCompositeByteBuf() {
         int checksumRes1 = Crc32cIntChecksum.computeChecksum(bTotal);
 
         // Calculate: case-2.
-        int b1CheckSum = Crc32cIntChecksum.computeChecksum(b1);
-        int checksumRes2 = Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2);
+        int b1CheckSum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), b1);
+        int checksumRes2 = Crc32cIntChecksum.finalizeValue(Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2));
 
         // Verify: the results of both ways to calculate the checksum are same.
         Assert.assertEquals(checksumRes1, checksumRes2);
@@ -90,8 +90,8 @@ public void calculateCheckSumUsingNoArrayNoMemoryAddrByteBuf() {
         int checksumRes1 = Crc32cIntChecksum.computeChecksum(bTotal);
 
         // Calculate: case-2.
-        int b1CheckSum = Crc32cIntChecksum.computeChecksum(b1);
-        int checksumRes2 = Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2);
+        int b1CheckSum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), b1);
+        int checksumRes2 = Crc32cIntChecksum.finalizeValue(Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2));
 
         // Verify: the results of both ways to calculate the checksum are same.
         Assert.assertEquals(checksumRes1, checksumRes2);