Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix checksum calculation bug when the payload is a CompositeByteBuf with readerIndex > 0 #4196

Merged
merged 39 commits into from
Feb 7, 2024
Merged
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
9fb47f5
Add a test that reproduces a bug in checksum calculation
lhotari Jan 31, 2024
0707710
Revert "Fixed unnecessary copy to heap (#2701)" changes to ByteBufList
lhotari Jan 30, 2024
fe9301d
Remove CompositeBuffer unwrapping in DigestManager
lhotari Jan 30, 2024
532de4f
Rename update -> internalUpdate so that unwrapping logic could be add…
lhotari Jan 30, 2024
9a39d6b
Remove unnecessary unwrapping logic in Java9IntHash
lhotari Jan 30, 2024
986942d
Add safe way to handle CompositeByteBuf
lhotari Jan 31, 2024
0669a66
Add license header
lhotari Jan 31, 2024
4036a3a
Fix checkstyle
lhotari Jan 31, 2024
7a8c200
Refactor ByteBuf visitor solution
lhotari Jan 31, 2024
b5b42ee
Fix checkstyle
lhotari Jan 31, 2024
81e9d2d
Reformat
lhotari Jan 31, 2024
6c7e6e0
Refactor recursive visiting
lhotari Jan 31, 2024
c19ae5d
Revisit equals, hashCode and toString
lhotari Jan 31, 2024
8928f98
Refactor test case
lhotari Feb 1, 2024
19e33ca
Add support for UnpooledHeapByteBuf.getBytes which passes an array
lhotari Feb 1, 2024
86d002d
Add support for visiting buffers backed by byte[] arrays
lhotari Feb 1, 2024
e793872
Move ByteBufVisitor to org.apache.bookkeeper.util package
lhotari Feb 1, 2024
6508bf5
Update javadoc
lhotari Feb 1, 2024
5cd2dd8
Refactor to use stateless visitor so that instance can be shared
lhotari Feb 1, 2024
fc93b5e
Improve test so that a single scenario can be used for debugging
lhotari Feb 2, 2024
272e962
Fix bug in Java9IntHash calculation that assumed crc32c_update(x) == …
lhotari Feb 2, 2024
b4d0b18
Fix checkstyle
lhotari Feb 2, 2024
9c1b68f
Fix checkstyle
lhotari Feb 2, 2024
a751294
Fix missing depth increment that prevents StackOverflowException
lhotari Feb 3, 2024
5f8fef5
Properly handle the depth increase and decrease
lhotari Feb 3, 2024
bb703ab
Remove unnecessary condition
lhotari Feb 3, 2024
2b79716
Use more efficient way to read bytes to the target array
lhotari Feb 3, 2024
f650b9b
Don't use ByteBufVisitor if it's not necessary
lhotari Feb 3, 2024
d21a5cd
Revert "Fix bug in Java9IntHash calculation that assumed crc32c_updat…
lhotari Feb 3, 2024
a9ed5e4
Fix issue in resume byte[] version that was added
lhotari Feb 3, 2024
b7d5795
Polish ByteBufVisitor
lhotari Feb 4, 2024
34c1c8a
Use extracted method
lhotari Feb 4, 2024
be20948
Fix bug with array handling
lhotari Feb 4, 2024
7998659
Polish ByteBufVisitor
lhotari Feb 4, 2024
b691316
Optimize the buffer copying in the case where array or memory address…
lhotari Feb 4, 2024
0af1556
Check if memory address is accepted
lhotari Feb 4, 2024
317aab7
Improve comments about complement (current = ~current) in resume
lhotari Feb 4, 2024
2c73dca
Print thread dump when build is cancelled
lhotari Feb 1, 2024
c215db8
Filter empty buffers and arrays in ByteBufVisitor
lhotari Feb 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Revert "Fix bug in Java9IntHash calculation that assumed crc32c_updat…
…e(x) == ~crc32c_update(~x)"

This reverts commit 272e962.
lhotari committed Feb 3, 2024
commit d21a5cddd2e7d83ca6f99a40c99e7c3200c1f12f
Original file line number Diff line number Diff line change
@@ -54,14 +54,4 @@ 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);
}
}
}
Original file line number Diff line number Diff line change
@@ -61,18 +61,7 @@ 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;
}
if (buffer.hasMemoryAddress()) {
return internalUpdate(digest, buffer, offset, len);
} else if (buffer.hasArray()) {
@@ -84,14 +73,6 @@ final int update(int digest, ByteBuf buffer, int offset, int len) {
}
}

protected final 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();
@@ -170,8 +151,8 @@ private ReferenceCounted computeDigestAndPackageForSendingV2(long entryId, long
buf.writeLong(length);

// Compute checksum over the headers
int digest = initializeAndUpdate(buf, buf.readerIndex(), buf.readableBytes());
digest = updateAndFinalize(data, digest, data.readerIndex(), data.readableBytes());
int digest = update(0, buf, buf.readerIndex(), buf.readableBytes());
digest = update(digest, data, data.readerIndex(), data.readableBytes());

populateValueAndReset(digest, buf);

@@ -195,8 +176,8 @@ private ByteBufList computeDigestAndPackageForSendingV3(long entryId, long lastA
headersBuffer.writeLong(lastAddConfirmed);
headersBuffer.writeLong(length);

int digest = initializeAndUpdate(headersBuffer, 0, METADATA_LENGTH);
digest = updateAndFinalize(data, digest, data.readerIndex(), data.readableBytes());
int digest = update(0, headersBuffer, 0, METADATA_LENGTH);
digest = update(digest, data, data.readerIndex(), data.readableBytes());
populateValueAndReset(digest, headersBuffer);
return ByteBufList.get(headersBuffer, data);
}
@@ -218,7 +199,7 @@ public ByteBufList computeDigestAndPackageForSendingLac(long lac) {
headersBuffer.writeLong(ledgerId);
headersBuffer.writeLong(lac);

int digest = finalizeDigest(initializeAndUpdate(headersBuffer, 0, LAC_METADATA_LENGTH));
int digest = update(0, headersBuffer, 0, LAC_METADATA_LENGTH);
populateValueAndReset(digest, headersBuffer);

return ByteBufList.get(headersBuffer);
@@ -254,10 +235,10 @@ private void verifyDigest(long entryId, ByteBuf dataReceived, boolean skipEntryI
this.getClass().getName(), dataReceived.readableBytes());
throw new BKDigestMatchException();
}
int digest = initializeAndUpdate(dataReceived, 0, METADATA_LENGTH);
int digest = update(0, dataReceived, 0, METADATA_LENGTH);

int offset = METADATA_LENGTH + macCodeLength;
digest = updateAndFinalize(dataReceived, digest, offset, dataReceived.readableBytes() - offset);
digest = update(digest, dataReceived, offset, dataReceived.readableBytes() - offset);

if (isInt32Digest()) {
int receivedDigest = dataReceived.getInt(METADATA_LENGTH);
@@ -302,7 +283,7 @@ public long verifyDigestAndReturnLac(ByteBuf dataReceived) throws BKDigestMatchE
throw new BKDigestMatchException();
}

int digest = finalizeDigest(initializeAndUpdate(dataReceived, 0, LAC_METADATA_LENGTH));
int digest = update(0, dataReceived, 0, LAC_METADATA_LENGTH);

if (isInt32Digest()) {
int receivedDigest = dataReceived.getInt(LAC_METADATA_LENGTH);
Original file line number Diff line number Diff line change
@@ -131,16 +131,6 @@ 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
Original file line number Diff line number Diff line change
@@ -94,11 +94,4 @@ 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);
}
}
Original file line number Diff line number Diff line change
@@ -30,12 +30,4 @@ 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;
}
}
Original file line number Diff line number Diff line change
@@ -68,12 +68,12 @@ protected byte[] initialValue() {

@Override
public int calculate(ByteBuf buffer) {
return finalizeValue(resume(initialValue(), buffer));
return resume(0, buffer);
}

@Override
public int calculate(ByteBuf buffer, int offset, int len) {
return finalizeValue(resume(initialValue(), buffer, offset, len));
return resume(0, buffer, offset, len);
}

private int resume(int current, long address, int offset, int length) {
@@ -100,35 +100,26 @@ 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()) {
current = resume(current, buffer.memoryAddress(), offset, len);
negCrc = resume(negCrc, buffer.memoryAddress(), offset, len);
} else if (buffer.hasArray()) {
int arrayOffset = buffer.arrayOffset() + offset;
current = resume(current, buffer.array(), arrayOffset, len);
negCrc = resume(negCrc, 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.getBytes(loopOffset, b, 0, length);
current = resume(current, b, 0, length);
negCrc = resume(negCrc, b, 0, length);
toRead -= length;
loopOffset += length;
}
}
return current;
}

@Override
public int initialValue() {
// match java.util.zip.CRC32C.reset logic
return 0xFFFFFFFF;
}

@Override
public int finalizeValue(int current) {
// match java.util.zip.CRC32C.getValue logic
return ~current;
return ~negCrc;
}
}
Original file line number Diff line number Diff line change
@@ -16,7 +16,6 @@
* specific language governing permissions and limitations
* under the License.
*/

package com.scurrilous.circe.checksum;

import static com.scurrilous.circe.params.CrcParameters.CRC32C;
@@ -45,9 +44,7 @@ public void testCrc32cValue() {
@Test
public void testCrc32cValueResume() {
final byte[] bytes = "Some String".getBytes();
int checksum = Crc32cIntChecksum.finalizeValue(
Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes), 0,
bytes.length));
int checksum = Crc32cIntChecksum.resumeChecksum(0, Unpooled.wrappedBuffer(bytes), 0, bytes.length);

assertEquals(608512271, checksum);
}
@@ -59,26 +56,22 @@ public void testCrc32cValueIncremental() {
int checksum = Crc32cIntChecksum.computeChecksum(Unpooled.wrappedBuffer(bytes));
assertEquals(608512271, checksum);

checksum =
Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes, 0, 1));
checksum = Crc32cIntChecksum.computeChecksum(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.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes, 0, 4));
checksum = Crc32cIntChecksum.computeChecksum(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.resumeChecksum(Crc32cIntChecksum.initialValue(), buffer);
checksum = Crc32cIntChecksum.computeChecksum(buffer);
checksum = Crc32cIntChecksum.resumeChecksum(
checksum, Unpooled.wrappedBuffer(bytes), 4, bytes.length - 4);
checksum = Crc32cIntChecksum.finalizeValue(checksum);
checksum, Unpooled.wrappedBuffer(bytes), 4, bytes.length - 4);

assertEquals(608512271, checksum);
}

@@ -93,10 +86,7 @@ public void testCrc32cLongValue() {
@Test
public void testCrc32cLongValueResume() {
final byte[] bytes = "Some String".getBytes();
long checksum =
Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), Unpooled.wrappedBuffer(bytes), 0,
bytes.length);
checksum = Crc32cIntChecksum.finalizeValue((int) checksum);
long checksum = Crc32cIntChecksum.resumeChecksum(0, Unpooled.wrappedBuffer(bytes), 0, bytes.length);

assertEquals(608512271L, checksum);
}
Original file line number Diff line number Diff line change
@@ -65,8 +65,8 @@ public void calculateCheckSumUsingCompositeByteBuf() {
int checksumRes1 = Crc32cIntChecksum.computeChecksum(bTotal);

// Calculate: case-2.
int b1CheckSum = Crc32cIntChecksum.resumeChecksum(Crc32cIntChecksum.initialValue(), b1);
int checksumRes2 = Crc32cIntChecksum.finalizeValue(Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2));
int b1CheckSum = Crc32cIntChecksum.computeChecksum(b1);
int checksumRes2 = 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.resumeChecksum(Crc32cIntChecksum.initialValue(), b1);
int checksumRes2 = Crc32cIntChecksum.finalizeValue(Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2));
int b1CheckSum = Crc32cIntChecksum.computeChecksum(b1);
int checksumRes2 = Crc32cIntChecksum.resumeChecksum(b1CheckSum, b2);

// Verify: the results of both ways to calculate the checksum are same.
Assert.assertEquals(checksumRes1, checksumRes2);