Gary Helmling created HBASE-18072: ------------------------------------- Summary: Malformed Cell from client causes Regionserver abort on flush Key: HBASE-18072 URL: https://issues.apache.org/jira/browse/HBASE-18072 Project: HBase Issue Type: Bug Components: regionserver, rpc Affects Versions: 1.3.0 Reporter: Gary Helmling Assignee: Gary Helmling Priority: Critical
When a client writes a mutation with a Cell with a corrupted value length field, it is possible for the corrupt cell to trigger an exception on memstore flush, which will trigger regionserver aborts until the region is manually recovered. This boils down to a lack of validation on the client submitted byte[] backing the cell. Consider the following sequence: 1. Client creates a new Put with a cell with value of byte[16] 2. When the backing KeyValue for the Put is created, we serialize 16 for the value length field in the backing array 3. Client calls Table.put() 4. RpcClientImpl calls KeyValueEncoder.encode() to serialize the Cell to the OutputStream 5. Memory corruption in the backing array changes the serialized contents of the value length field from 16 to 48 6. Regionserver handling the put uses KeyValueDecoder.decode() to create a KeyValue with the byte[] read directly off the InputStream. The overall length of the array is correct, but the integer value serialized at the value length offset has been corrupted from the original value of 16 to 48. 7. The corrupt KeyValue is appended to the WAL and added to the memstore 8. After some time, the memstore flushes. As HFileWriter is writing out the corrupted cell, it reads the serialized int from the value length position in the cell's byte[] to determine the number of bytes to write for the value. Because value offset + 48 is greater than the length of the cell's byte[], we hit an IndexOutOfBoundsException: {noformat} java.lang.IndexOutOfBoundsException at java.io.ByteArrayOutputStream.write(ByteArrayOutputStream.java:151) at java.io.DataOutputStream.write(DataOutputStream.java:107) at org.apache.hadoop.hbase.io.hfile.NoOpDataBlockEncoder.encode(NoOpDataBlockEncoder.java:56) at org.apache.hadoop.hbase.io.hfile.HFileBlock$Writer.write(HFileBlock.java:954) at org.apache.hadoop.hbase.io.hfile.HFileWriterV2.append(HFileWriterV2.java:284) at org.apache.hadoop.hbase.io.hfile.HFileWriterV3.append(HFileWriterV3.java:87) at org.apache.hadoop.hbase.regionserver.StoreFile$Writer.append(StoreFile.java:1041) at org.apache.hadoop.hbase.regionserver.StoreFlusher.performFlush(StoreFlusher.java:138) at org.apache.hadoop.hbase.regionserver.DefaultStoreFlusher.flushSnapshot(DefaultStoreFlusher.java:75) at org.apache.hadoop.hbase.regionserver.HStore.flushCache(HStore.java:937) at org.apache.hadoop.hbase.regionserver.HStore$StoreFlusherImpl.flushCache(HStore.java:2413) at org.apache.hadoop.hbase.regionserver.HRegion.internalFlushCacheAndCommit(HRegion.java:2456) {noformat} 9. Regionserver aborts due to the failed flush 10. The regionserver WAL is split into recovered.edits files, one of these containing the same corrupted cell 11. A new regionserver is assigned the region with the corrupted write 12. The new regionserver replays the recovered.edits entries into memstore and then tries to flush the memstore to an HFile 13. The flush triggers the same IndexOutOfBoundsException, causing us to go back to step #8 and loop on repeat until manual intervention is taken The corrupted cell basically becomes a poison pill that aborts regionservers one at a time as the region with the problem edit is passed around. This also means that a malicious client could easily construct requests allowing a denial of service attack against regionservers hosting any tables that the client has write access to. At bare minimum, I think we need to do a sanity check on all the lengths for Cells read off the CellScanner for incoming requests. This would allow us to reject corrupt cells before we append them to the WAL and succeed the request, putting us in a position where we cannot recover. This would only detect the corruption of length fields which puts us in a bad state. Whether or not Cells should carry some checksum generated at the time the Cell is created, which could then validated on the server-side, is a separate question. This would allow detection of other parts of the backing cell byte[], such as within the key fields or the value field. But the computer overhead of this may be too heavyweight to be practical. -- This message was sent by Atlassian JIRA (v6.3.15#6346)