From 371f8066d2502f6c6d75c5d0f80cd25e19b5e58b Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 20 Jun 2014 18:19:45 +0200 Subject: [PATCH] [#2580] [#2587] Fix buffer corruption regression when ByteBuf.order(LITTLE_ENDIAN) is used Motivation: To improve the speed of ByteBuf with order LITTLE_ENDIAN and where the native order is also LITTLE_ENDIAN (intel) we introduces a new special SwappedByteBuf before in commit 4ad3984c8b725ef59856d174d09d1209d65933fc. Unfortunally the commit has a flaw which does not handle correctly the case when a ByteBuf expands. This was caused because the memoryAddress was cached and never changed again even if the underlying buffer expanded. This can lead to corrupt data or even to SEGFAULT the JVM if you are lucky enough. Modification: Always lookup the actual memoryAddress of the wrapped ByteBuf. Result: No more data-corruption for ByteBuf with order LITTLE_ENDIAN and no JVM crashes. --- .../io/netty/buffer/PooledUnsafeDirectByteBuf.java | 2 +- .../io/netty/buffer/UnpooledUnsafeDirectByteBuf.java | 2 +- .../io/netty/buffer/UnsafeDirectSwappedByteBuf.java | 10 ++++++---- .../java/io/netty/buffer/AbstractByteBufTest.java | 12 ++++++++++++ .../test/java/io/netty/buffer/SlicedByteBufTest.java | 8 ++++++++ 5 files changed, 28 insertions(+), 6 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java index 795335e4c3..efb3bb4308 100644 --- a/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/PooledUnsafeDirectByteBuf.java @@ -384,6 +384,6 @@ final class PooledUnsafeDirectByteBuf extends PooledByteBuf { @Override protected SwappedByteBuf newSwappedByteBuf() { - return new UnsafeDirectSwappedByteBuf(this, memoryAddress); + return new UnsafeDirectSwappedByteBuf(this); } } diff --git a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java index bb4c6e6add..26aaf66203 100644 --- a/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnpooledUnsafeDirectByteBuf.java @@ -517,6 +517,6 @@ public class UnpooledUnsafeDirectByteBuf extends AbstractReferenceCountedByteBuf @Override protected SwappedByteBuf newSwappedByteBuf() { - return new UnsafeDirectSwappedByteBuf(this, memoryAddress); + return new UnsafeDirectSwappedByteBuf(this); } } diff --git a/buffer/src/main/java/io/netty/buffer/UnsafeDirectSwappedByteBuf.java b/buffer/src/main/java/io/netty/buffer/UnsafeDirectSwappedByteBuf.java index 5c346f3894..e777c80578 100644 --- a/buffer/src/main/java/io/netty/buffer/UnsafeDirectSwappedByteBuf.java +++ b/buffer/src/main/java/io/netty/buffer/UnsafeDirectSwappedByteBuf.java @@ -27,17 +27,19 @@ final class UnsafeDirectSwappedByteBuf extends SwappedByteBuf { private static final boolean NATIVE_ORDER = ByteOrder.nativeOrder() == ByteOrder.BIG_ENDIAN; private final boolean nativeByteOrder; private final AbstractByteBuf wrapped; - private final long memoryAddress; - UnsafeDirectSwappedByteBuf(AbstractByteBuf buf, long memoryAddress) { + UnsafeDirectSwappedByteBuf(AbstractByteBuf buf) { super(buf); wrapped = buf; - this.memoryAddress = memoryAddress; nativeByteOrder = NATIVE_ORDER == (order() == ByteOrder.BIG_ENDIAN); } private long addr(int index) { - return memoryAddress + index; + // We need to call wrapped.memoryAddress() everytime and NOT cache it as it may change if the buffer expand. + // See: + // - https://github.com/netty/netty/issues/2587 + // - https://github.com/netty/netty/issues/2580 + return wrapped.memoryAddress() + index; } @Override diff --git a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java index 932bb17697..4d8353d3d9 100644 --- a/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java @@ -1943,6 +1943,18 @@ public abstract class AbstractByteBufTest { } } + // See: + // - https://github.com/netty/netty/issues/2587 + // - https://github.com/netty/netty/issues/2580 + @Test + public void testLittleEndianWithExpand() { + ByteBuf buffer = releaseLater(newBuffer(0)).order(LITTLE_ENDIAN); + System.out.println(buffer.getClass()); + buffer.writeInt(0x12345678); + System.out.println(ByteBufUtil.hexDump(buffer)); + assertEquals("78563412", ByteBufUtil.hexDump(buffer)); + } + static final class TestGatheringByteChannel implements GatheringByteChannel { private final ByteArrayOutputStream out = new ByteArrayOutputStream(); private final WritableByteChannel channel = Channels.newChannel(out); diff --git a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java index 505de88d70..32e0be795a 100644 --- a/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java +++ b/buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java @@ -15,6 +15,8 @@ */ package io.netty.buffer; +import static io.netty.buffer.Unpooled.LITTLE_ENDIAN; +import static io.netty.util.ReferenceCountUtil.releaseLater; import static org.junit.Assert.*; import java.util.Random; @@ -94,4 +96,10 @@ public class SlicedByteBufTest extends AbstractByteBufTest { public void testNioBufferExposeOnlyRegion() { super.testNioBufferExposeOnlyRegion(); } + + @Test + @Override + public void testLittleEndianWithExpand() { + // ignore for SlicedByteBuf + } }