From 1eb9a9764eca54c7c31e16729b5a91e69aa6e4e7 Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Wed, 8 Sep 2021 20:02:59 +0200 Subject: [PATCH] Buffer should not expose nativeAddress() directly (#11665) Motivation: Accessing the native address, if a buffer has any, violates the no-aliasing rule for Buffers. Also, it inherently assumes that a buffer has a single, native memory allocation. The native address, or addresses, are already available via the Readable- and WritableComponents. Accessing these via forEachReadable and forEachWritable will also ensure that composite buffers will be handled correctly. Modification: Remove the nativeAddress() method from the Buffer API. Update the ByteBufAdaptor and a few tests to cope with this change. Result: Less error prone code, and make unsafe APIs a bit more hidden. --- .../main/java/io/netty/buffer/api/Buffer.java | 6 --- .../io/netty/buffer/api/BufferAllocator.java | 16 +++--- .../java/io/netty/buffer/api/BufferStub.java | 5 -- .../io/netty/buffer/api/CompositeBuffer.java | 5 -- .../buffer/api/adaptor/ByteBufAdaptor.java | 36 +++++++++++-- .../buffer/api/bytebuffer/NioBuffer.java | 3 +- .../netty/buffer/api/unsafe/UnsafeBuffer.java | 17 ++++--- .../api/tests/BufferBulkAccessTest.java | 50 ++++++++++++++++++- 8 files changed, 99 insertions(+), 39 deletions(-) diff --git a/buffer/src/main/java/io/netty/buffer/api/Buffer.java b/buffer/src/main/java/io/netty/buffer/api/Buffer.java index 1cdd0f48af..e0090ad568 100644 --- a/buffer/src/main/java/io/netty/buffer/api/Buffer.java +++ b/buffer/src/main/java/io/netty/buffer/api/Buffer.java @@ -172,12 +172,6 @@ public interface Buffer extends Resource, BufferAccessor { */ Buffer fill(byte value); - /** - * Gives the native memory address backing this buffer, or return 0 if this buffer has no native memory address. - * @return The native memory address, if any, otherwise 0. - */ - long nativeAddress(); - /** * Makes this buffer read-only. This is irreversible. * This operation is also idempotent, so calling this method multiple times on the same buffer makes no difference. diff --git a/buffer/src/main/java/io/netty/buffer/api/BufferAllocator.java b/buffer/src/main/java/io/netty/buffer/api/BufferAllocator.java index 0d91b8af91..1d0f4dfa21 100644 --- a/buffer/src/main/java/io/netty/buffer/api/BufferAllocator.java +++ b/buffer/src/main/java/io/netty/buffer/api/BufferAllocator.java @@ -25,8 +25,9 @@ import java.util.function.Supplier; public interface BufferAllocator extends AutoCloseable { /** * Produces a {@link BufferAllocator} that allocates unpooled, on-heap buffers. - * On-heap buffers have a {@code byte[]} internally, and their {@linkplain Buffer#nativeAddress() native address} - * is zero. + * On-heap buffers have a {@code byte[]} internally, and their + * {@linkplain ReadableComponent#readableNativeAddress() readable} and + * {@linkplain WritableComponent#writableNativeAddress() writable} native addresses are zero. *

* The concrete {@link Buffer} implementation is chosen by {@link MemoryManager#instance()}. * @@ -39,7 +40,8 @@ public interface BufferAllocator extends AutoCloseable { /** * Produces a {@link BufferAllocator} that allocates unpooled, off-heap buffers. * Off-heap buffers a native memory pointer internally, which can be obtained from their - * {@linkplain Buffer#nativeAddress() native address method. + * {@linkplain ReadableComponent#readableNativeAddress() readable} and + * {@linkplain WritableComponent#writableNativeAddress() writable} native address methods. *

* The concrete {@link Buffer} implementation is chosen by {@link MemoryManager#instance()}. * @@ -51,8 +53,9 @@ public interface BufferAllocator extends AutoCloseable { /** * Produces a pooling {@link BufferAllocator} that allocates and recycles on-heap buffers. - * On-heap buffers have a {@code byte[]} internally, and their {@linkplain Buffer#nativeAddress() native address} - * is zero. + * On-heap buffers have a {@code byte[]} internally, and their + * {@linkplain ReadableComponent#readableNativeAddress() readable} and + * {@linkplain WritableComponent#writableNativeAddress() writable} native addresses are zero. *

* The concrete {@link Buffer} implementation is chosen by {@link MemoryManager#instance()}. * @@ -65,7 +68,8 @@ public interface BufferAllocator extends AutoCloseable { /** * Produces a pooling {@link BufferAllocator} that allocates and recycles off-heap buffers. * Off-heap buffers a native memory pointer internally, which can be obtained from their - * {@linkplain Buffer#nativeAddress() native address method. + * {@linkplain ReadableComponent#readableNativeAddress() readable} and + * {@linkplain WritableComponent#writableNativeAddress() writable} native address methods. *

* The concrete {@link Buffer} implementation is chosen by {@link MemoryManager#instance()}. * diff --git a/buffer/src/main/java/io/netty/buffer/api/BufferStub.java b/buffer/src/main/java/io/netty/buffer/api/BufferStub.java index c1fa48577e..d962ba9fb9 100644 --- a/buffer/src/main/java/io/netty/buffer/api/BufferStub.java +++ b/buffer/src/main/java/io/netty/buffer/api/BufferStub.java @@ -76,11 +76,6 @@ public class BufferStub implements Buffer { return delegate.fill(value); } - @Override - public long nativeAddress() { - return delegate.nativeAddress(); - } - @Override public Buffer makeReadOnly() { return delegate.makeReadOnly(); diff --git a/buffer/src/main/java/io/netty/buffer/api/CompositeBuffer.java b/buffer/src/main/java/io/netty/buffer/api/CompositeBuffer.java index 6c123cf8fc..57d2325ff9 100644 --- a/buffer/src/main/java/io/netty/buffer/api/CompositeBuffer.java +++ b/buffer/src/main/java/io/netty/buffer/api/CompositeBuffer.java @@ -384,11 +384,6 @@ public final class CompositeBuffer extends ResourceSupport { + if (index == 0) { + nativeAddress = component.readableNativeAddress(); + return true; + } else { + nativeAddress = 0; + return false; + } + }); + } else if (buffer.countWritableComponents() == 1) { + buffer.forEachWritable(0, (index, component) -> { + if (index == 0) { + nativeAddress = component.writableNativeAddress(); + return true; + } else { + nativeAddress = 0; + return false; + } + }); + } else { + nativeAddress = 0; + } + } + /** * Extracts the underlying {@link Buffer} instance that is backing this {@link ByteBuf}, if any. * This is similar to {@link #unwrap()} except the return type is a {@link Buffer}. @@ -154,7 +180,7 @@ public final class ByteBufAdaptor extends ByteBuf { @Override public boolean isDirect() { - return hasMemoryAddress; + return nativeAddress != 0; } @Override @@ -1549,7 +1575,7 @@ public final class ByteBufAdaptor extends ByteBuf { @Override public boolean hasMemoryAddress() { - return hasMemoryAddress; + return isDirect(); } @Override @@ -1557,7 +1583,7 @@ public final class ByteBufAdaptor extends ByteBuf { if (!hasMemoryAddress()) { throw new UnsupportedOperationException("No memory address associated with this buffer."); } - return buffer.nativeAddress(); + return nativeAddress; } @Override diff --git a/buffer/src/main/java/io/netty/buffer/api/bytebuffer/NioBuffer.java b/buffer/src/main/java/io/netty/buffer/api/bytebuffer/NioBuffer.java index 0fa0971b3e..2b25173da5 100644 --- a/buffer/src/main/java/io/netty/buffer/api/bytebuffer/NioBuffer.java +++ b/buffer/src/main/java/io/netty/buffer/api/bytebuffer/NioBuffer.java @@ -165,8 +165,7 @@ class NioBuffer extends AdaptableBuffer implements ReadableComponent, return this; } - @Override - public long nativeAddress() { + private long nativeAddress() { return rmem.isDirect() && PlatformDependent.hasUnsafe()? PlatformDependent.directBufferAddress(rmem) : 0; } diff --git a/buffer/src/main/java/io/netty/buffer/api/unsafe/UnsafeBuffer.java b/buffer/src/main/java/io/netty/buffer/api/unsafe/UnsafeBuffer.java index ddaaed5cf3..ae814871ef 100644 --- a/buffer/src/main/java/io/netty/buffer/api/unsafe/UnsafeBuffer.java +++ b/buffer/src/main/java/io/netty/buffer/api/unsafe/UnsafeBuffer.java @@ -135,8 +135,7 @@ class UnsafeBuffer extends AdaptableBuffer implements ReadableComp return this; } - @Override - public long nativeAddress() { + private long nativeAddress() { return base == null? address : 0; } @@ -232,14 +231,16 @@ class UnsafeBuffer extends AdaptableBuffer implements ReadableComp if (dest.readOnly()) { throw bufferIsReadOnly(this); } - long nativeAddress = dest.nativeAddress(); try { - if (nativeAddress != 0) { - PlatformDependent.copyMemory(base, address + srcPos, null, nativeAddress + destPos, length); - } else if (dest instanceof UnsafeBuffer) { + if (dest instanceof UnsafeBuffer) { UnsafeBuffer destUnsafe = (UnsafeBuffer) dest; - PlatformDependent.copyMemory( - base, address + srcPos, destUnsafe.base, destUnsafe.address + destPos, length); + long nativeAddress = destUnsafe.nativeAddress(); + if (nativeAddress != 0) { + PlatformDependent.copyMemory(base, address + srcPos, null, nativeAddress + destPos, length); + } else { + PlatformDependent.copyMemory( + base, address + srcPos, destUnsafe.base, destUnsafe.address + destPos, length); + } } else { Statics.copyToViaReverseLoop(this, srcPos, dest, destPos, length); } diff --git a/buffer/src/test/java/io/netty/buffer/api/tests/BufferBulkAccessTest.java b/buffer/src/test/java/io/netty/buffer/api/tests/BufferBulkAccessTest.java index ba9a495ffc..13da19b308 100644 --- a/buffer/src/test/java/io/netty/buffer/api/tests/BufferBulkAccessTest.java +++ b/buffer/src/test/java/io/netty/buffer/api/tests/BufferBulkAccessTest.java @@ -233,7 +233,30 @@ public class BufferBulkAccessTest extends BufferTestSupport { public void heapBufferMustHaveZeroAddress(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(8)) { - assertThat(buf.nativeAddress()).isZero(); + assertThat(buf.countReadableComponents()).isZero(); + assertThat(buf.countWritableComponents()).isOne(); + buf.forEachWritable(0, (index, component) -> { + assertThat(component.writableNativeAddress()).isZero(); + return true; + }); + buf.writeInt(42); + assertThat(buf.countReadableComponents()).isOne(); + assertThat(buf.countWritableComponents()).isOne(); + buf.forEachReadable(0, (index, component) -> { + assertThat(component.readableNativeAddress()).isZero(); + return true; + }); + buf.forEachWritable(0, (index, component) -> { + assertThat(component.writableNativeAddress()).isZero(); + return true; + }); + buf.writeInt(42); + assertThat(buf.countReadableComponents()).isOne(); + assertThat(buf.countWritableComponents()).isZero(); + buf.forEachReadable(0, (index, component) -> { + assertThat(component.readableNativeAddress()).isZero(); + return true; + }); } } @@ -242,7 +265,30 @@ public class BufferBulkAccessTest extends BufferTestSupport { public void directBufferMustHaveNonZeroAddress(Fixture fixture) { try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(8)) { - assertThat(buf.nativeAddress()).isNotZero(); + assertThat(buf.countReadableComponents()).isZero(); + assertThat(buf.countWritableComponents()).isOne(); + buf.forEachWritable(0, (index, component) -> { + assertThat(component.writableNativeAddress()).isNotZero(); + return true; + }); + buf.writeInt(42); + assertThat(buf.countReadableComponents()).isOne(); + assertThat(buf.countWritableComponents()).isOne(); + buf.forEachReadable(0, (index, component) -> { + assertThat(component.readableNativeAddress()).isNotZero(); + return true; + }); + buf.forEachWritable(0, (index, component) -> { + assertThat(component.writableNativeAddress()).isNotZero(); + return true; + }); + buf.writeInt(42); + assertThat(buf.countReadableComponents()).isOne(); + assertThat(buf.countWritableComponents()).isZero(); + buf.forEachReadable(0, (index, component) -> { + assertThat(component.readableNativeAddress()).isNotZero(); + return true; + }); } }