From 81918a46e5bd46a514937732182750991cfdc4bb Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Mon, 28 Feb 2022 17:16:19 +0100 Subject: [PATCH 1/4] HBASE-26777 BufferedDataBlockEncoder$OffheapDecodedExtendedCell.deepClone throws UnsupportedOperationException Change-Id: I45807821ebf4ae867b5c9c10e2700ecd3c009a4b --- .../hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java index bcac9deac448..533c304facfc 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java @@ -715,11 +715,6 @@ public void write(ByteBuffer buf, int offset) { throw new UnsupportedOperationException(); } - @Override - public ExtendedCell deepClone() { - // This is not used in actual flow. Throwing UnsupportedOperationException - throw new UnsupportedOperationException(); - } } protected abstract static class BufferedEncodedSeeker From 8b7d2a19b7b68ddf69ea6badba89604ff7a53b8a Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Tue, 1 Mar 2022 07:05:51 +0100 Subject: [PATCH 2/4] copy the Cell to a KeyValue explicitly Change-Id: I74fb38ad0dc45e9a808c06923f1f168576e7b5f4 --- .../hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java | 5 +++++ .../java/org/apache/hadoop/hbase/regionserver/HRegion.java | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java index 533c304facfc..bcac9deac448 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java @@ -715,6 +715,11 @@ public void write(ByteBuffer buf, int offset) { throw new UnsupportedOperationException(); } + @Override + public ExtendedCell deepClone() { + // This is not used in actual flow. Throwing UnsupportedOperationException + throw new UnsupportedOperationException(); + } } protected abstract static class BufferedEncodedSeeker 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 1acbf162b098..fca0f7129df2 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 @@ -7867,7 +7867,7 @@ private List getInternal(Get get, boolean withCoprocessor, long nonceGroup // See more details in HBASE-26036. for (Cell cell : tmp) { results.add(cell instanceof ByteBufferExtendedCell ? - ((ByteBufferExtendedCell) cell).deepClone(): cell); + new KeyValue(cell) : cell); } } From 47e8ad2e7fceb24afc06b282bd86799b2bfa735b Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Tue, 1 Mar 2022 08:58:25 +0100 Subject: [PATCH 3/4] use KeyValueUtil.copyToNewKeyValue, which should be faster. Change-Id: I7d12f6b03481f7ba0091af5b937b8d79136db452 --- .../src/main/java/org/apache/hadoop/hbase/KeyValueUtil.java | 2 ++ .../java/org/apache/hadoop/hbase/regionserver/HRegion.java | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) 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 a2fdcc4e6d8c..da843f1f7d6a 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 @@ -131,6 +131,8 @@ public static byte[] copyToNewByteArray(final Cell cell) { //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]; 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 fca0f7129df2..10c626d77e04 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 @@ -94,6 +94,7 @@ 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; @@ -7867,7 +7868,7 @@ private List getInternal(Get get, boolean withCoprocessor, long nonceGroup // See more details in HBASE-26036. for (Cell cell : tmp) { results.add(cell instanceof ByteBufferExtendedCell ? - new KeyValue(cell) : cell); + KeyValueUtil.copyToNewKeyValue(cell) : cell); } } From 904daa223ce4a0bddea904b04e6bc0f01d441ba2 Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Tue, 1 Mar 2022 12:49:38 +0100 Subject: [PATCH 4/4] add test Change-Id: I7a74d8c174d99b775a0f0eb48ece22068e1e1e05 --- .../TestCheckAndMutateWithByteBuff.java | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) 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 de1d02bfe734..1489c1f0400b 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 @@ -32,6 +32,7 @@ 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; @@ -87,8 +88,20 @@ public static void tearDownAfterClass() throws Exception { } @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"); @@ -104,10 +117,13 @@ public void testCheckAndMutateWithByteBuff() throws Exception { 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); }