From f2df336d0b2fd8d728cf65682771b88b9ce90d74 Mon Sep 17 00:00:00 2001 From: StefanoBelli Date: Wed, 18 Sep 2024 14:50:11 +0200 Subject: [PATCH 1/4] Added check into BufferedChannel's read to avoid endless loop if dest buffer's remaining capacity is not as much as length --- .../java/org/apache/bookkeeper/bookie/BufferedChannel.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java index 3197165827a..49b6112949c 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java @@ -243,6 +243,11 @@ public long forceWrite(boolean forceMetadata) throws IOException { @Override public synchronized int read(ByteBuf dest, long pos, int length) throws IOException { + if(dest.writableBytes() < length) { + throw new IllegalArgumentException("dest buffer remaining capacity is not enough" + + "(must be at least as \"length\"=" + length +")"); + } + long prevPos = pos; while (length > 0) { // check if it is in the write buffer From 753c908950983f7405d0a02bd143e7a1cb5771b4 Mon Sep 17 00:00:00 2001 From: ste Date: Wed, 18 Sep 2024 20:59:35 +0200 Subject: [PATCH 2/4] Fixing checkstyle issues --- .../org/apache/bookkeeper/bookie/BufferedChannel.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java index 49b6112949c..d80c48ab645 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java @@ -243,9 +243,9 @@ public long forceWrite(boolean forceMetadata) throws IOException { @Override public synchronized int read(ByteBuf dest, long pos, int length) throws IOException { - if(dest.writableBytes() < length) { - throw new IllegalArgumentException("dest buffer remaining capacity is not enough" + - "(must be at least as \"length\"=" + length +")"); + if (dest.writableBytes() < length) { + throw new IllegalArgumentException("dest buffer remaining capacity is not enough" + + "(must be at least as \"length\"=" + length +")"); } long prevPos = pos; @@ -300,4 +300,4 @@ public synchronized int getNumOfBytesInWriteBuffer() { long getUnpersistedBytes() { return unpersistedBytes.get(); } -} \ No newline at end of file +} From bcd5d66ab2d57485ad777ab70515e1d581344781 Mon Sep 17 00:00:00 2001 From: ste Date: Wed, 18 Sep 2024 21:07:55 +0200 Subject: [PATCH 3/4] Fixing checkstyle issues (2) --- .../main/java/org/apache/bookkeeper/bookie/BufferedChannel.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java index d80c48ab645..c55c432211d 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java @@ -245,7 +245,7 @@ public long forceWrite(boolean forceMetadata) throws IOException { public synchronized int read(ByteBuf dest, long pos, int length) throws IOException { if (dest.writableBytes() < length) { throw new IllegalArgumentException("dest buffer remaining capacity is not enough" - + "(must be at least as \"length\"=" + length +")"); + + "(must be at least as \"length\"=" + length + ")"); } long prevPos = pos; From 3353ee71c5829986935b240b8e63b6059fc9494f Mon Sep 17 00:00:00 2001 From: StefanoBelli Date: Thu, 19 Sep 2024 10:03:03 +0200 Subject: [PATCH 4/4] Fixing checkstyle issues (3) and new unit test case (BufferedChannelTest) --- .../bookkeeper/bookie/BufferedChannel.java | 2 +- .../bookie/BufferedChannelTest.java | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java index c55c432211d..dbba31083d9 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java @@ -244,7 +244,7 @@ public long forceWrite(boolean forceMetadata) throws IOException { @Override public synchronized int read(ByteBuf dest, long pos, int length) throws IOException { if (dest.writableBytes() < length) { - throw new IllegalArgumentException("dest buffer remaining capacity is not enough" + throw new IllegalArgumentException("dest buffer remaining capacity is not enough" + "(must be at least as \"length\"=" + length + ")"); } diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BufferedChannelTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BufferedChannelTest.java index cd3e34d35e3..81e7c62af4a 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BufferedChannelTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BufferedChannelTest.java @@ -21,10 +21,13 @@ package org.apache.bookkeeper.bookie; +import static org.junit.Assert.assertThrows; + import io.netty.buffer.ByteBuf; import io.netty.buffer.Unpooled; import io.netty.buffer.UnpooledByteBufAllocator; import java.io.File; +import java.io.IOException; import java.io.RandomAccessFile; import java.nio.channels.FileChannel; import java.util.Random; @@ -126,6 +129,41 @@ public void testBufferedChannel(int byteBufLength, int numOfWrites, int unpersis fileChannel.close(); } + @Test + public void testBufferedChannelReadWhenDestBufSizeExceedsReadLength() throws IOException { + doTestBufferedChannelReadThrowing(100, 60); + } + + @Test + public void testBufferedChannelReadWhenDestBufSizeDoesNotExceedReadLength() throws IOException { + doTestBufferedChannelReadThrowing(100, 110); + } + + private void doTestBufferedChannelReadThrowing(int destBufSize, int readLength) throws IOException { + File newLogFile = File.createTempFile("test", "log"); + newLogFile.deleteOnExit(); + + try (RandomAccessFile raf = new RandomAccessFile(newLogFile, "rw")) { + FileChannel fileChannel = raf.getChannel(); + + try (BufferedChannel bufferedChannel = new BufferedChannel( + UnpooledByteBufAllocator.DEFAULT, fileChannel, + INTERNAL_BUFFER_WRITE_CAPACITY, INTERNAL_BUFFER_READ_CAPACITY, 0)) { + + bufferedChannel.write(generateEntry(500)); + + ByteBuf destBuf = UnpooledByteBufAllocator.DEFAULT.buffer(destBufSize); + + if (destBufSize < readLength) { + assertThrows(IllegalArgumentException.class, + () -> bufferedChannel.read(destBuf, 0, readLength)); + } else { + bufferedChannel.read(destBuf, 0, readLength); + } + } + } + } + private static ByteBuf generateEntry(int length) { byte[] data = new byte[length]; ByteBuf bb = Unpooled.buffer(length);