[hbase] 67/91: HubSpot Backport: HBASE-26777 BufferedDataBlockEncoder$OffheapDecodedExtendedCell.deepC… (#4139)

[bb...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

bbeaudreault pushed a commit to branch hubspot-2
in repository https://gitbox.apache.org/repos/asf/hbase.git

commit 421db7914a431bac0383eab5885f7fa56105f85a
Author: Istvan Toth <st...@stoty.hu>
AuthorDate: Fri Mar 4 22:49:19 2022 +0100

    HubSpot Backport: HBASE-26777 BufferedDataBlockEncoder$OffheapDecodedExtendedCell.deepC… (#4139)
    
    Signed-off-by: Andrew Purtell <ap...@apache.org>
---
 .../java/org/apache/hadoop/hbase/KeyValueUtil.java |  8 +++++++-
 .../apache/hadoop/hbase/regionserver/HRegion.java  |  3 ++-
 .../client/TestCheckAndMutateWithByteBuff.java     | 24 ++++++++++++++++++----
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValueUtil.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValueUtil.java
index 8a17ce975dc..bdac750a0dc 100644
--- a/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValueUtil.java
+++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValueUtil.java
@@ -128,7 +128,13 @@ public class KeyValueUtil {
   }
 
   public static byte[] copyToNewByteArray(final Cell cell) {
-    int v1Length = cell.getSerializedSize();
+    //Cell#getSerializedSize returns the serialized size of the Source cell, which may
+    //not serialize all fields. We are constructing a KeyValue backing array here,
+    //which does include all fields, and must allocate accordingly.
+    //TODO we could probably use Cell#getSerializedSize safely, the errors were
+    //caused by cells corrupted by use-after-free bugs
+    int v1Length = length(cell.getRowLength(), cell.getFamilyLength(),
+      cell.getQualifierLength(), cell.getValueLength(), cell.getTagsLength(), true);
     byte[] backingBytes = new byte[v1Length];
     appendToByteArray(cell, backingBytes, 0, true);
     return backingBytes;
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
index d304fa3c8fd..2f767d685f0 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
@@ -93,6 +93,7 @@ import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.HConstants.OperationStatusCode;
 import org.apache.hadoop.hbase.HDFSBlocksDistribution;
 import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.KeyValueUtil;
 import org.apache.hadoop.hbase.MetaCellComparator;
 import org.apache.hadoop.hbase.NamespaceDescriptor;
 import org.apache.hadoop.hbase.NotServingRegionException;
@@ -8272,7 +8273,7 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
       // See more details in HBASE-26036.
       for (Cell cell : tmp) {
         results.add(cell instanceof ByteBufferExtendedCell ?
-          ((ByteBufferExtendedCell) cell).deepClone(): cell);
+            KeyValueUtil.copyToNewKeyValue(cell) : cell);
       }
     }
 
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestCheckAndMutateWithByteBuff.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestCheckAndMutateWithByteBuff.java
index 876259f4e81..813ff2eeb1d 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestCheckAndMutateWithByteBuff.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestCheckAndMutateWithByteBuff.java
@@ -31,6 +31,7 @@ import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.io.ByteBuffAllocator;
 import org.apache.hadoop.hbase.io.DeallocateRewriteByteBuffAllocator;
+import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding;
 import org.apache.hadoop.hbase.io.hfile.BlockCacheFactory;
 import org.apache.hadoop.hbase.regionserver.HRegion;
 import org.apache.hadoop.hbase.regionserver.HRegionFileSystem;
@@ -86,8 +87,20 @@ public class TestCheckAndMutateWithByteBuff {
   }
 
   @Test
-  public void testCheckAndMutateWithByteBuff() throws Exception {
-    Table testTable = createTable(TableName.valueOf(name.getMethodName()));
+  public void testCheckAndMutateWithByteBuffNoEncode() throws Exception {
+    testCheckAndMutateWithByteBuff(TableName.valueOf(name.getMethodName()), DataBlockEncoding.NONE);
+  }
+
+  @Test
+  public void testCheckAndMutateWithByteBuffEncode() throws Exception {
+    // Tests for HBASE-26777.
+    // As most HBase.getRegion() calls have been factored out from HBase, you'd need to revert
+    // both HBASE-26777, and the HBase.get() replacements from HBASE-26036 for this test to fail
+    testCheckAndMutateWithByteBuff(TableName.valueOf(name.getMethodName()), DataBlockEncoding.FAST_DIFF);
+  }
+
+  private void testCheckAndMutateWithByteBuff(TableName tableName, DataBlockEncoding dbe) throws Exception {
+    Table testTable = createTable(tableName, dbe);
     byte[] checkRow = Bytes.toBytes("checkRow");
     byte[] checkQualifier = Bytes.toBytes("cq");
     byte[] checkValue = Bytes.toBytes("checkValue");
@@ -103,10 +116,13 @@ public class TestCheckAndMutateWithByteBuff {
         Bytes.toBytes("testValue"))));
   }
 
-  private Table createTable(TableName tableName)
+  private Table createTable(TableName tableName, DataBlockEncoding dbe)
     throws IOException {
     TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName)
-      .setColumnFamily(ColumnFamilyDescriptorBuilder.newBuilder(CF).setBlocksize(100).build())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.newBuilder(CF)
+        .setBlocksize(100)
+        .setDataBlockEncoding(dbe)
+        .build())
       .build();
     return TEST_UTIL.createTable(td, null);
   }