Re: [PR] Fix checksum calculation bug when the payload is a CompositeByteBuf with readerIndex > 0 [bookkeeper]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari commented on code in PR #4196:
URL: https://github.com/apache/bookkeeper/pull/4196#discussion_r1472960488


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DigestManager.java:
##########
@@ -49,14 +49,89 @@ public abstract class DigestManager {
 
     public static final int METADATA_LENGTH = 32;
     public static final int LAC_METADATA_LENGTH = 16;
+    private static final int MAX_SUB_BUFFER_VISIT_RECURSION_DEPTH = 10;
 
     final long ledgerId;
     final boolean useV2Protocol;
     private final ByteBufAllocator allocator;
 
     abstract int getMacCodeLength();
 
-    abstract int update(int digest, ByteBuf buffer, int offset, int len);
+    abstract int internalUpdate(int digest, ByteBuf buffer, int offset, int len);
+
+    final int update(int digest, ByteBuf buffer, int offset, int len) {
+        return recursiveSubBufferVisitForDigestUpdate(digest, buffer, offset, len, 0);
+    }
+
+    private int recursiveSubBufferVisitForDigestUpdate(int digest, ByteBuf buffer, int offset, int len, int depth) {
+        if (len == 0) {
+            return digest;
+        }
+        if (depth < MAX_SUB_BUFFER_VISIT_RECURSION_DEPTH && !buffer.hasMemoryAddress() && !buffer.hasArray()) {
+            return visitWrappedBuffersAndCallInternalUpdateForEach(digest, buffer, offset, len, depth);
+        }
+        return internalUpdate(digest, buffer, offset, len);
+    }
+
+    /**
+     * This method is used to visit the wrapped buffers and call internalUpdate for each of them.
+     * CompositeByteBuf is one of the wrapped buffer types that will be visited. It can contain multiple
+     * sub buffers. The sub buffers can be wrapped buffers as well.
+     *
+     * Netty doesn't provide an API to visit the wrapped buffers, so we have to use the a hack here.
+     *
+     * @param digest current digest value
+     * @param buffer the buffer to visit
+     * @param offset the offset in the buffer
+     * @param len the length in the buffer
+     * @param depth the recursion depth of the wrapped buffer visit
+     * @return updated digest value
+     */
+    private int visitWrappedBuffersAndCallInternalUpdateForEach(int digest, ByteBuf buffer, int offset, int len,
+                                                                int depth) {
+        // hold the digest in a MutableInt so that it can be updated in the wrapped buffer visit
+        MutableInt digestRef = new MutableInt(digest);
+        ByteBuf sliced = buffer.slice(offset, len);
+        // call getBytes to trigger the wrapped buffer visit
+        sliced.getBytes(0, new DuplicatedByteBuf(Unpooled.EMPTY_BUFFER) {

Review Comment:
   @BewareMyPower I have address your review feedback and attempted to cover your concerns by refactoring the ByteBuf visiting solution. It no longer uses `DuplicatedByteBuf` as the base class. Instead a full custom `ByteBuf` implementation is used with `throw new UnsupportedOperationException()` bodies for all other methods than the ones that are part of the `getBytes`/`setBytes` interface contract. This will ensure that things will break if something unexpected is passed as input.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org