HBASE-28010: Prevent connection attribute corruption #5366
Merged
Conversation
This file contains 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
rmdmattingly
force-pushed
the
HBASE-28010
branch
from
f1452d9 to
60885cc
Compare
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
force-pushed
the
HBASE-28010
branch
from
60885cc to
a99d6b9
Compare
bbeaudreault
requested changes
Aug 22, 2023
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java
Outdated
Show resolved
Hide resolved
...-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java
Outdated
Show resolved
Hide resolved
...-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| if (header.getAttributeList().isEmpty()) { | ||
| this.requestAttributes = Collections.emptyMap(); | ||
| } else { | ||
| this.requestAttributes = Maps.newHashMapWithExpectedSize(header.getAttributeList().size()); |
There was a problem hiding this comment.
I don't think we need to synchronize this method given the chances of threading issues (low) and the cost of the computation (low), but we should consider thread safety here nonetheless.
At the very least, we should make this.requestAttributes volatile. Then here we should construct a local map and add the values onto it, then only do this.requestAttributes = localAttributes at the end. This way if there did happen to be 2 callers, one would not get an incomplete view of the request attributes.
| @@ -405,6 +410,16 @@ private CodedInputStream createCis(ByteBuff buf) { | |||
| // Reads the connection header following version | |||
| private void processConnectionHeader(ByteBuff buf) throws IOException { | |||
| this.connectionHeader = ConnectionHeader.parseFrom(createCis(buf)); | |||
| if (connectionHeader.getAttributeList().isEmpty()) { | |||
There was a problem hiding this comment.
can you add a comment here that we want to copy the attributes prior to releasing the buffer so that they don't get corrupted down the line when the buffer's underlying memory is replaced for some other call?
| @@ -83,7 +86,17 @@ public interface RpcCall extends RpcCallContext { | |||
| /** Returns The request header of this call. */ | |||
| RequestHeader getHeader(); | |||
|
|
|||
| ConnectionHeader getConnectionHeader(); | |||
| /** | |||
| * Returns the map of attributes specified when building the Connection See the Map argument on | |||
There was a problem hiding this comment.
nitpick: since you already have to do one more revision can you fix this and below javadoc to at least add a period prior to See the..? Could also add a <br> if you want them to be on the next line, or better yet use @see, i.e.:
/**
* Returns the map of attributes specified when building the Connection.
* @see ConnectionFactory#createConnection(Configuration, ExecutorService, User, Map)
*/The see will automatically also do the linking.
bbeaudreault
approved these changes
Aug 22, 2023
|
🎊 +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. |
|
@rmdmattingly Looks like the Maps imports are wrong, so the build is failing. |
|
Yep I’ve pushed a fix already |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Looking better but now there are some unused imports in RpcCall, per checkstyle |
|
🤦I can never keep track of what spotless cleans up and what it leaves behind. Fixing now |
|
Ah ok, it's these imports that are only servicing the javadoc — I can remove those to appease checkstyle, but IMO it makes the javadoc marginally worse. Is that fine? |
|
Interesting. I feel like it's a bug due to the splitting of the |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Test failures don't seem relevant: |
bbeaudreault
approved these changes
Aug 23, 2023
bbeaudreault
pushed a commit
that referenced
this pull request
Aug 23, 2023
…side (#5366) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault
pushed a commit
that referenced
this pull request
Aug 23, 2023
…side (#5366) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault
pushed a commit
to HubSpot/hbase
that referenced
this pull request
Aug 23, 2023
…ted on the server side (apache#5366) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
…side (apache#5366) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In ServerRpcConnection.processOneRpc, it calls processConnectionHeader and then immediately calls callCleanupIfNeeded. The parsing of the ByteBuff into the ConnectionHeader does not copy the bytes. We keep a reference to ConnectionHeader for later use, but since the underlying ByteBuff gets released in callCleanupIfNeeded, later requests can override the memory locations that the ConnectionHeader points at.
In this PR we modified our test to ensure that it failed in the current state on master — enabling the byte buffer resevoir and sending a large enough put caused the ConnectionHeader attributes to disappear. We fixed this by copying the parsed ConnectionHeader's attributes to their own Map, and then making this Map accessible on the RpcCall.
@bbeaudreault