HBASE-25010 assert on allocated buffer sizes before making calls to Unsafe.copyMemory
#2380
+14
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… `Unsafe.copyMemory`
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
commented
Sep 10, 2020
| assert (dest.capacity() - destOffset) >= length : "unsafe memory access: attempting to copy " | ||
| + length + " bytes into dest when only " + (dest.capacity() - destOffset) | ||
| + " capacity remains."; | ||
| unsafeCopy(src, srcAddress, destBase, destAddress, length); |
There was a problem hiding this comment.
These assert are protecting calls to the method invoked by unsafeCopy: Unsafe.copyMemory. The documentation on that method makes no mention of bounds checking, and indeed I don't think it can because it accepts Object,long as parameters describing the source and destination memory addresses.
From AdoptOpenJDK:
/**
* Sets all bytes in a given block of memory to a copy of another
* block.
*
* <p>This method determines each block's base address by means of two parameters,
* and so it provides (in effect) a <em>double-register</em> addressing mode,
* as discussed in {@link #getInt(Object,long)}. When the object reference is null,
* the offset supplies an absolute base address.
*
* <p>The transfers are in coherent (atomic) units of a size determined
* by the address and length parameters. If the effective addresses and
* length are all even modulo 8, the transfer takes place in 'long' units.
* If the effective addresses and length are (resp.) even modulo 4 or 2,
* the transfer takes place in units of 'int' or 'short'.
*
* @since 1.7
*/
public native void copyMemory(Object srcBase, long srcOffset,
Object destBase, long destOffset,
long bytes);
anoopsjohn
approved these changes
Sep 11, 2020
ramkrish86
approved these changes
Sep 11, 2020
There was a problem hiding this comment.
@ndimiduk - Did you face any issue due to the lack of assert ? +1 LGTM
Not as of yet. Test runs appear clear and I haven't yet gotten this deployed into the environment where we're experiencing the crashes. I just wanted to share the patch with you experts early, in case I'm doing something obviously stupid. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.