From e85cbdb4e8135c928fe904034a62334995dd78ed Mon Sep 17 00:00:00 2001
From: mrambacher <mrambach@gmail.com>
Date: Thu, 4 Jun 2020 11:38:34 -0700
Subject: [PATCH] Fix two core dumps when files are missing (#6922)

Summary:
The LDB create and drop column family commands failed to check if theere was a valid database prior to dereferencing it, leading to a core dump.

The SstFileDumper prefetch code would dereference a file when the file did not exist as part of the Prefetch code.  This dereference was moved inside an st.ok() check.

Tests were added for both failure conditions.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6922

Reviewed By: gg814

Differential Revision: D21884024

Pulled By: pdillinger

fbshipit-source-id: bddd45c299aa9dc7e928c17a37a96521f8c9149e
---
 tools/ldb_cmd.cc       | 12 ++++++++++++
 tools/ldb_cmd_test.cc  | 23 +++++++++++++++++++++++
 tools/sst_dump_test.cc | 19 +++++++++++++++++++
 tools/sst_dump_tool.cc | 16 ++++++++--------
 4 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/tools/ldb_cmd.cc b/tools/ldb_cmd.cc
index 7d86fa3c4..8e63eee03 100644
--- a/tools/ldb_cmd.cc
+++ b/tools/ldb_cmd.cc
@@ -1302,6 +1302,10 @@ CreateColumnFamilyCommand::CreateColumnFamilyCommand(
 }
 
 void CreateColumnFamilyCommand::DoCommand() {
+  if (!db_) {
+    assert(GetExecuteState().IsFailed());
+    return;
+  }
   ColumnFamilyHandle* new_cf_handle = nullptr;
   Status st = db_->CreateColumnFamily(options_, new_cf_name_, &new_cf_handle);
   if (st.ok()) {
@@ -1335,6 +1339,10 @@ DropColumnFamilyCommand::DropColumnFamilyCommand(
 }
 
 void DropColumnFamilyCommand::DoCommand() {
+  if (!db_) {
+    assert(GetExecuteState().IsFailed());
+    return;
+  }
   auto iter = cf_handles_.find(cf_name_to_drop_);
   if (iter == cf_handles_.end()) {
     exec_state_ = LDBCommandExecuteResult::Failed(
@@ -2031,6 +2039,10 @@ Options ChangeCompactionStyleCommand::PrepareOptionsForOpenDB() {
 }
 
 void ChangeCompactionStyleCommand::DoCommand() {
+  if (!db_) {
+    assert(GetExecuteState().IsFailed());
+    return;
+  }
   // print db stats before we have made any change
   std::string property;
   std::string files_per_level;
diff --git a/tools/ldb_cmd_test.cc b/tools/ldb_cmd_test.cc
index 638ef9568..549e31514 100644
--- a/tools/ldb_cmd_test.cc
+++ b/tools/ldb_cmd_test.cc
@@ -643,6 +643,29 @@ TEST_F(LdbCmdTest, DisableConsistencyChecks) {
     SyncPoint::GetInstance()->DisableProcessing();
   }
 }
+
+TEST_F(LdbCmdTest, TestBadDbPath) {
+  Env* base_env = TryLoadCustomOrDefaultEnv();
+  std::unique_ptr<Env> env(NewMemEnv(base_env));
+  Options opts;
+  opts.env = env.get();
+  opts.create_if_missing = true;
+
+  std::string dbname = test::TmpDir();
+  char arg1[] = "./ldb";
+  char arg2[1024];
+  snprintf(arg2, sizeof(arg2), "--db=%s/.no_such_dir", dbname.c_str());
+  char arg3[1024];
+  snprintf(arg3, sizeof(arg3), "create_column_family");
+  char arg4[] = "bad cf";
+  char* argv[] = {arg1, arg2, arg3, arg4};
+
+  ASSERT_EQ(1,
+            LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));
+  snprintf(arg3, sizeof(arg3), "drop_column_family");
+  ASSERT_EQ(1,
+            LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr));
+}
 }  // namespace ROCKSDB_NAMESPACE
 
 #ifdef ROCKSDB_UNITTESTS_WITH_CUSTOM_OBJECTS_FROM_STATIC_LIBS
diff --git a/tools/sst_dump_test.cc b/tools/sst_dump_test.cc
index c6dfb0f6e..c2116fac8 100644
--- a/tools/sst_dump_test.cc
+++ b/tools/sst_dump_test.cc
@@ -286,6 +286,25 @@ TEST_F(SSTDumpToolTest, ReadaheadSize) {
   }
 }
 
+TEST_F(SSTDumpToolTest, NoSstFile) {
+  Options opts;
+  opts.env = env();
+  std::string file_path = MakeFilePath("no_such_file.sst");
+  char* usage[3];
+  PopulateCommandArgs(file_path, "", usage);
+  ROCKSDB_NAMESPACE::SSTDumpTool tool;
+  for (const auto& command :
+       {"--command=check", "--command=dump", "--command=raw",
+        "--command=verify", "--command=recompress", "--command=verify_checksum",
+        "--show_properties"}) {
+    snprintf(usage[1], kOptLength, "%s", command);
+    ASSERT_TRUE(!tool.Run(3, usage, opts));
+  }
+  for (int i = 0; i < 3; i++) {
+    delete[] usage[i];
+  }
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 
 #ifdef ROCKSDB_UNITTESTS_WITH_CUSTOM_OBJECTS_FROM_STATIC_LIBS
diff --git a/tools/sst_dump_tool.cc b/tools/sst_dump_tool.cc
index b0c7115dc..c52361877 100644
--- a/tools/sst_dump_tool.cc
+++ b/tools/sst_dump_tool.cc
@@ -100,15 +100,15 @@ Status SstFileDumper::GetTableReader(const std::string& file_path) {
 
   FilePrefetchBuffer prefetch_buffer(nullptr, 0, 0, true /* enable */,
                                      false /* track_min_offset */);
-  const uint64_t kSstDumpTailPrefetchSize = 512 * 1024;
-  uint64_t prefetch_size = (file_size > kSstDumpTailPrefetchSize)
-                               ? kSstDumpTailPrefetchSize
-                               : file_size;
-  uint64_t prefetch_off = file_size - prefetch_size;
-  prefetch_buffer.Prefetch(file_.get(), prefetch_off,
-                           static_cast<size_t>(prefetch_size));
-
   if (s.ok()) {
+    const uint64_t kSstDumpTailPrefetchSize = 512 * 1024;
+    uint64_t prefetch_size = (file_size > kSstDumpTailPrefetchSize)
+                                 ? kSstDumpTailPrefetchSize
+                                 : file_size;
+    uint64_t prefetch_off = file_size - prefetch_size;
+    prefetch_buffer.Prefetch(file_.get(), prefetch_off,
+                             static_cast<size_t>(prefetch_size));
+
     s = ReadFooterFromFile(file_.get(), &prefetch_buffer, file_size, &footer);
   }
   if (s.ok()) {