From 7699439b7c7415cea47ccf2efc80a6e8d101788f Mon Sep 17 00:00:00 2001
From: Jay Edgar <jkedgar@fb.com>
Date: Mon, 4 Jan 2016 10:51:00 -0800
Subject: [PATCH] Prevent the user from setting block_restart_interval to less
 than 1

Summary: If block_restart_interval gets set to less than 1 an assert will be triggered in BlockBuilder::BlockBuilder().  This prevents the user from doing this by silently setting any value less than 1 to 1.

Test Plan: Added a test (in BlockBasedTableTest in table_test) that checks invalid values to make sure that they are reset to the expected values.  The block_restart_interval value is checked along with block_size_deviation which also silently sets the value if it is outside a specific range.

Reviewers: yoshinorim, IslamAbdelRahman

Reviewed By: IslamAbdelRahman

Subscribers: dhruba

Differential Revision: https://reviews.facebook.net/D52509
---
 include/rocksdb/table.h            |  3 +-
 table/block_based_table_factory.cc |  3 ++
 table/table_test.cc                | 44 ++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/include/rocksdb/table.h b/include/rocksdb/table.h
index f522bf869..2e1a91de9 100644
--- a/include/rocksdb/table.h
+++ b/include/rocksdb/table.h
@@ -116,7 +116,8 @@ struct BlockBasedTableOptions {
 
   // Number of keys between restart points for delta encoding of keys.
   // This parameter can be changed dynamically.  Most clients should
-  // leave this parameter alone.
+  // leave this parameter alone.  The minimum value allowed is 1.  Any smaller
+  // value will be silently overwritten with 1.
   int block_restart_interval = 16;
 
   // Use delta encoding to compress keys in blocks.
diff --git a/table/block_based_table_factory.cc b/table/block_based_table_factory.cc
index dfcb9cde0..a6484c4ee 100644
--- a/table/block_based_table_factory.cc
+++ b/table/block_based_table_factory.cc
@@ -39,6 +39,9 @@ BlockBasedTableFactory::BlockBasedTableFactory(
       table_options_.block_size_deviation > 100) {
     table_options_.block_size_deviation = 0;
   }
+  if (table_options_.block_restart_interval < 1) {
+    table_options_.block_restart_interval = 1;
+  }
 }
 
 Status BlockBasedTableFactory::NewTableReader(
diff --git a/table/table_test.cc b/table/table_test.cc
index eb2b52ef9..38ec8400a 100644
--- a/table/table_test.cc
+++ b/table/table_test.cc
@@ -1698,6 +1698,50 @@ TEST_F(BlockBasedTableTest, FilterBlockInBlockCache) {
   props.AssertFilterBlockStat(0, 0);
 }
 
+void ValidateBlockSizeDeviation(int value, int expected) {
+  BlockBasedTableOptions table_options;
+  table_options.block_size_deviation = value;
+  BlockBasedTableFactory* factory = new BlockBasedTableFactory(table_options);
+
+  const BlockBasedTableOptions* normalized_table_options =
+      (const BlockBasedTableOptions*)factory->GetOptions();
+  ASSERT_EQ(normalized_table_options->block_size_deviation, expected);
+
+  delete factory;
+}
+
+void ValidateBlockRestartInterval(int value, int expected) {
+  BlockBasedTableOptions table_options;
+  table_options.block_restart_interval = value;
+  BlockBasedTableFactory* factory = new BlockBasedTableFactory(table_options);
+
+  const BlockBasedTableOptions* normalized_table_options =
+      (const BlockBasedTableOptions*)factory->GetOptions();
+  ASSERT_EQ(normalized_table_options->block_restart_interval, expected);
+
+  delete factory;
+}
+
+TEST_F(BlockBasedTableTest, InvalidOptions) {
+  // invalid values for block_size_deviation (<0 or >100) are silently set to 0
+  ValidateBlockSizeDeviation(-10, 0);
+  ValidateBlockSizeDeviation(-1, 0);
+  ValidateBlockSizeDeviation(0, 0);
+  ValidateBlockSizeDeviation(1, 1);
+  ValidateBlockSizeDeviation(99, 99);
+  ValidateBlockSizeDeviation(100, 100);
+  ValidateBlockSizeDeviation(101, 0);
+  ValidateBlockSizeDeviation(1000, 0);
+
+  // invalid values for block_restart_interval (<1) are silently set to 1
+  ValidateBlockRestartInterval(-10, 1);
+  ValidateBlockRestartInterval(-1, 1);
+  ValidateBlockRestartInterval(0, 1);
+  ValidateBlockRestartInterval(1, 1);
+  ValidateBlockRestartInterval(2, 2);
+  ValidateBlockRestartInterval(1000, 1000);
+}
+
 TEST_F(BlockBasedTableTest, BlockReadCountTest) {
   // bloom_filter_type = 0 -- block-based filter
   // bloom_filter_type = 0 -- full filter
-- 
GitLab