[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5366: HBASE-28010: Prevent connection attribute corruption

["bbeaudreault (via GitHub)" <gi...@apache.org>]

bbeaudreault commented on code in PR #5366:
URL: https://github.com/apache/hbase/pull/5366#discussion_r1302015880


##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java:
##########
@@ -118,6 +126,15 @@ public void testConnectionAttributes() throws IOException {
     }
   }
 
+  private void ensureBuffersAreOverwritten(Table table, byte[] cf) throws IOException {
+    // this will cause unread connection attributes on the

Review Comment:
   This comment seems unfinished, but to simplify this I think you can get the same effect by just using a large rowkey on the get. I reproduced with a 300 byte random rowkey in the get above. 
   
   As we discussed, this involves only netty's allocator. What I think is happening here is that the connection header ends up being backed by a buffer which is around 371 bytes (from debugger). The attributes exist at some offset in in that buffer, towards the end since the field is new on the proto messasge. A single Get with a small int-sized rowkey and no filters ends up only ~120 bytes (also from debugger). Since nothing else is happening on the server, when the Get comes it it does corrupt the connection header buffer but only the first 120 bytes. If you use a rowkey of 300 bytes the overall payload of 120 becomes closer to 420 which is large enough that the corruption extends into the range of affecting the attributes.



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCall.java:
##########
@@ -83,7 +83,7 @@ public interface RpcCall extends RpcCallContext {
   /** Returns The request header of this call. */
   RequestHeader getHeader();
 
-  ConnectionHeader getConnectionHeader();

Review Comment:
   While we are here, I wonder if we should retain parity with request attributes. We could add a `Map<String, byte[]> getRequestAttributes()`. I think the impl for that should be lazy since there's no point spending cpu/memory creating the map if it's not going to be used. You can use this in your slow logs PR, or you could add it in that PR if you want. I just want to try to see if we can get it in before 2.6.0 drops.
   
   Please add javadoc to this new method



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java:
##########
@@ -405,6 +409,9 @@ private CodedInputStream createCis(ByteBuff buf) {
   // Reads the connection header following version
   private void processConnectionHeader(ByteBuff buf) throws IOException {
     this.connectionHeader = ConnectionHeader.parseFrom(createCis(buf));
+    this.connectionAttributes = connectionHeader.getAttributeList().stream()

Review Comment:
   lets initialize the map with an explicit size. and set it to null or Collections.emptyMap() if there are no attributes on the header.
   
   it's good to keep things sized int his context because a regionserver may have many open connections and a little wasted memory per connection can add up



##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRequestAndConnectionAttributes.java:
##########
@@ -84,6 +86,8 @@ public class TestRequestAndConnectionAttributes {
   @BeforeClass
   public static void setUp() throws Exception {
     TEST_UTIL = new HBaseTestingUtil();
+    Configuration conf = TEST_UTIL.getConfiguration();
+    conf.setBoolean(ALLOCATOR_POOL_ENABLED_KEY, true);

Review Comment:
   i dont think we need this



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org