Fix possible SIGSEGV in CompactRange (github issue #596)

Summary: For very detailed explanation of what's happening read this: https://github.com/facebook/rocksdb/issues/596

Test Plan: make check + new unit test

Reviewers: yhchiang, anthony, rven

Reviewed By: rven

Subscribers: adamretter, dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D37779

Conflicts:
	db/db_test.cc

diff --git a/db/db_impl.cc b/db/db_impl.cc
index ca462b693e5ad9f19ae96be1fa1eceb7467dba4f..6de8535762c170cda2135041ae7c727a2f72521b 100644 (file)
--- a/db/db_impl.cc
+++ b/db/db_impl.cc
@@ -1243,7 +1243,8 @@ Status DBImpl::CompactRange(ColumnFamilyHandle* column_family,
   {
     InstrumentedMutexLock l(&mutex_);
     Version* base = cfd->current();
-    for (int level = 1; level < cfd->NumberLevels(); level++) {
+    for (int level = 1; level < base->storage_info()->num_non_empty_levels();
+         level++) {
       if (base->storage_info()->OverlapInLevel(level, begin, end)) {
         max_level_with_files = level;
       }

diff --git a/db/db_test.cc b/db/db_test.cc
index a27511cf53c81e10622ee613f8718dd57f1a7690..3c013e756926b8886bdc17ef2f26dc0a16f3220d 100644 (file)
--- a/db/db_test.cc
+++ b/db/db_test.cc
@@ -11963,6 +11963,25 @@ TEST_F(DBTest, EmptyCompactedDB) {
   Close();
 }
 
+// Github issue #596
+TEST_F(DBTest, HugeNumberOfLevels) {
+  Options options = CurrentOptions();
+  options.write_buffer_size = 2 * 1024 * 1024;         // 2MB
+  options.max_bytes_for_level_base = 2 * 1024 * 1024;  // 2MB
+  options.num_levels = 12;
+  options.max_background_compactions = 10;
+  options.max_bytes_for_level_multiplier = 2;
+  options.level_compaction_dynamic_level_bytes = true;
+  DestroyAndReopen(options);
+
+  Random rnd(301);
+  for (int i = 0; i < 300000; ++i) {
+    ASSERT_OK(Put(Key(i), RandomString(&rnd, 1024)));
+  }
+
+  ASSERT_OK(db_->CompactRange(nullptr, nullptr));
+}
+
 }  // namespace rocksdb
 
 int main(int argc, char** argv) {

diff --git a/db/file_indexer.cc b/db/file_indexer.cc
index c59036bd66943b5134d78b2d9a3048a293dccb4e..222cca9c03b56b979fbb2bc62a25a2a6f61d2069 100644 (file)
--- a/db/file_indexer.cc
+++ b/db/file_indexer.cc
@@ -20,6 +20,9 @@ FileIndexer::FileIndexer(const Comparator* ucmp)
 size_t FileIndexer::NumLevelIndex() const { return next_level_index_.size(); }
 
 size_t FileIndexer::LevelIndexSize(size_t level) const {
+  if (level >= next_level_index_.size()) {
+    return 0;
+  }
   return next_level_index_[level].num_index;
 }
 

diff --git a/db/file_indexer_test.cc b/db/file_indexer_test.cc
index 85e083546dac4c303dde0bb22c5e4ed0c19f8e52..98fea47feb5ce0c0ef43f0234884c1dd1ee41f17 100644 (file)
--- a/db/file_indexer_test.cc
+++ b/db/file_indexer_test.cc
@@ -11,6 +11,7 @@
 #include "db/file_indexer.h"
 #include "db/dbformat.h"
 #include "db/version_edit.h"
+#include "port/stack_trace.h"
 #include "rocksdb/comparator.h"
 #include "util/testharness.h"
 #include "util/testutil.h"
@@ -343,6 +344,7 @@ TEST_F(FileIndexerTest, mixed) {
 }  // namespace rocksdb
 
 int main(int argc, char** argv) {
+  rocksdb::port::InstallStackTraceHandler();
   ::testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }

diff --git a/db/version_set.cc b/db/version_set.cc
index 17cddcb9f2c9ffc1e4e608edb00773de95973069..fd6d88fae5ad0c77fdba9a0ce8b24b65c4c87c8c 100644 (file)
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -1184,6 +1184,10 @@ bool Version::Unref() {
 bool VersionStorageInfo::OverlapInLevel(int level,
                                         const Slice* smallest_user_key,
                                         const Slice* largest_user_key) {
+  if (level >= num_non_empty_levels_) {
+    // empty level, no overlap
+    return false;
+  }
   return SomeFileOverlapsRange(*internal_comparator_, (level > 0),
                                level_files_brief_[level], smallest_user_key,
                                largest_user_key);
@@ -1227,6 +1231,11 @@ int VersionStorageInfo::PickLevelForMemTableOutput(
 void VersionStorageInfo::GetOverlappingInputs(
     int level, const InternalKey* begin, const InternalKey* end,
     std::vector<FileMetaData*>* inputs, int hint_index, int* file_index) {
+  if (level >= num_non_empty_levels_) {
+    // this level is empty, no overlapping inputs
+    return;
+  }
+
   inputs->clear();
   Slice user_begin, user_end;
   if (begin != nullptr) {

diff --git a/util/autovector.h b/util/autovector.h
index 9362536d3d0619049bf656e1c9e62f1f5fa1baea..c9befe965b3d96785246d434932ccfc9d3d2ae0a 100644 (file)
--- a/util/autovector.h
+++ b/util/autovector.h
@@ -190,16 +190,16 @@ class autovector {
 
   bool empty() const { return size() == 0; }
 
-  // will not check boundry
   const_reference operator[](size_type n) const {
+    assert(n < size());
     return n < kSize ? values_[n] : vect_[n - kSize];
   }
 
   reference operator[](size_type n) {
+    assert(n < size());
     return n < kSize ? values_[n] : vect_[n - kSize];
   }
 
-  // will check boundry
   const_reference at(size_type n) const {
     assert(n < size());
     return (*this)[n];