Adding a case to handle read-only BB. #4013
Conversation
| @@ -195,6 +196,8 @@ public Object run() | |||
| { | |||
| BYTE_ARRAY_BASE_OFFSET = theUnsafe.arrayBaseOffset(byte[].class); | |||
| DIRECT_BUFFER_ADDRESS_OFFSET = theUnsafe.objectFieldOffset(Buffer.class.getDeclaredField("address")); | |||
|
|
|||
| // Heap ByteBuffer (Mutable) | ||
| if (srcBuf.hasArray() && !srcBuf.isReadOnly()) | ||
| { | ||
| src = theUnsafe.getObject(srcBuf, HEAP_HB_FIELD); |
There was a problem hiding this comment.
rather than rely on the new logic, can we keep the old logic that has been safe for years? unsafe can break on us in jdk bumps so if we can avoid it in the 99.999% case that is best
| @@ -163,6 +163,7 @@ public static final class UnsafeOperations implements ByteOperations | |||
| */ | |||
| static final long BYTE_ARRAY_BASE_OFFSET; | |||
| static final long DIRECT_BUFFER_ADDRESS_OFFSET; | |||
| static final long HEAP_HB_FIELD; | |||
There was a problem hiding this comment.
nit: append the same suffix, _OFFSET to this field.
| if (srcBuf.hasArray()) | ||
|
|
||
| // Heap ByteBuffer (Mutable) | ||
| if (srcBuf.hasArray() && !srcBuf.isReadOnly()) |
There was a problem hiding this comment.
nit: srcBuf.hasArray() already implies !srcBuf.isReadOnly(), the second condition is redundant.
| srcOffset = BYTE_ARRAY_BASE_OFFSET; | ||
|
|
||
| if (src == null) | ||
| throw new IllegalArgumentException("Unsupported ByteBuffer type: No backing array and not direct."); |
There was a problem hiding this comment.
nit: feel to me that it is an illegal state?
There was a problem hiding this comment.
Yes, I just wanted to be sure we do not crash. But yes it makes sense.
| if (src == null) | ||
| throw new IllegalArgumentException("Unsupported ByteBuffer type: No backing array and not direct."); | ||
| } | ||
| // Direct ByteBuffer |
There was a problem hiding this comment.
thanks for adding the comments for the conditions. 👍
| if (srcBuf.hasArray()) | ||
| { | ||
| src = srcBuf.array(); | ||
| srcOffset = BYTE_ARRAY_BASE_OFFSET + srcBuf.arrayOffset(); | ||
| } | ||
| // Read-Only Heap ByteBuffer (Still has hb but read-only) |
There was a problem hiding this comment.
can you add these comments to all code paths modified?
| else if (buffer1.isReadOnly() && !buffer1.isDirect()) | ||
| { | ||
| obj1 = theUnsafe.getObject(buffer1, HEAP_HB_FIELD_OFFSET); | ||
| offset1 = BYTE_ARRAY_BASE_OFFSET; |
There was a problem hiding this comment.
is this actually true? what about the following
ByteBuffer.wrap(new byte[42]).position(10).asReadOnly()
In this case won't the offset be BYTE_ARRAY_BASE_OFFSET + buffer1.arrayOffset()?
Cassandra-20431
Fix JVM crash when writing read-only ByteBuffer in BufferedDataOutputStreamPlus
Ensure
BufferedDataOutputStreamPlus#write(ByteBuffer)safely handles read-only buffers, preventing JVM crashes.patch by Sunil Ramchandra Pawar; reviewed by David Capwell for CASSANDRA-20431