Improve error messages for SST footer and size errors (#11009)

Summary:
Previously, you could get a format_version error if SST file size was too small in manifest, or a weird "too short" error if too big in manifest. Now we ensure:
* Magic number error is reported first if we attempt to open an SST file and the footer is completely bad.
* Footer errors are reported with affected file.
* If manifest file size doesn't match actual, then the error includes expected and actual sizes (if an error is reported; in some cases we allow the file to be too big)

Pull Request resolved: https://github.com/facebook/rocksdb/pull/11009

Test Plan:
unit tests added, some manual

Previously, the code for "file too short" in footer processing was only covered by some tests attempting to verify SST checksums on non-SST files (fixed).

Reviewed By: siying

Differential Revision: D41656272

Pulled By: pdillinger

fbshipit-source-id: 3da32702eb5aaedbea0e5e74742ad57edd7ad3df

diff --git a/db/corruption_test.cc b/db/corruption_test.cc
index 8ccac61303c65fd8483700e7ab4dd8bb8c235420..7544d098c44025ba90a5385028c85354a7bb2732 100644 (file)
--- a/db/corruption_test.cc
+++ b/db/corruption_test.cc
@@ -223,7 +223,8 @@ class CorruptionTest : public testing::Test {
     }
     ASSERT_TRUE(!fname.empty()) << filetype;
 
-    ASSERT_OK(test::CorruptFile(env_, fname, offset, bytes_to_corrupt));
+    ASSERT_OK(test::CorruptFile(env_, fname, offset, bytes_to_corrupt,
+                                /*verify_checksum*/ filetype == kTableFile));
   }
 
   // corrupts exactly one file at level `level`. if no file found at level,
@@ -518,6 +519,90 @@ TEST_F(CorruptionTest, TableFileIndexData) {
   ASSERT_TRUE(TryReopen().IsCorruption());
 }
 
+TEST_F(CorruptionTest, TableFileFooterMagic) {
+  Build(100);
+  DBImpl* dbi = static_cast_with_check<DBImpl>(db_);
+  ASSERT_OK(dbi->TEST_FlushMemTable());
+  Check(100, 100);
+  // Corrupt the whole footer
+  Corrupt(kTableFile, -100, 100);
+  Status s = TryReopen();
+  ASSERT_TRUE(s.IsCorruption());
+  // Contains useful message, and magic number should be the first thing
+  // reported as corrupt.
+  ASSERT_TRUE(s.ToString().find("magic number") != std::string::npos);
+  // with file name
+  ASSERT_TRUE(s.ToString().find(".sst") != std::string::npos);
+}
+
+TEST_F(CorruptionTest, TableFileFooterNotMagic) {
+  Build(100);
+  DBImpl* dbi = static_cast_with_check<DBImpl>(db_);
+  ASSERT_OK(dbi->TEST_FlushMemTable());
+  Check(100, 100);
+  // Corrupt footer except magic number
+  Corrupt(kTableFile, -100, 92);
+  Status s = TryReopen();
+  ASSERT_TRUE(s.IsCorruption());
+  // The next thing checked after magic number is format_version
+  ASSERT_TRUE(s.ToString().find("format_version") != std::string::npos);
+  // with file name
+  ASSERT_TRUE(s.ToString().find(".sst") != std::string::npos);
+}
+
+TEST_F(CorruptionTest, TableFileWrongSize) {
+  Build(100);
+  DBImpl* dbi = static_cast_with_check<DBImpl>(db_);
+  ASSERT_OK(dbi->TEST_FlushMemTable());
+  Check(100, 100);
+
+  // ********************************************
+  // Make the file bigger by appending to it
+  std::vector<LiveFileMetaData> metadata;
+  db_->GetLiveFilesMetaData(&metadata);
+  ASSERT_EQ(1U, metadata.size());
+  std::string filename = dbname_ + metadata[0].name;
+  const auto& fs = options_.env->GetFileSystem();
+  {
+    std::unique_ptr<FSWritableFile> f;
+    ASSERT_OK(fs->ReopenWritableFile(filename, FileOptions(), &f, nullptr));
+    ASSERT_OK(f->Append("blahblah", IOOptions(), nullptr));
+    ASSERT_OK(f->Close(IOOptions(), nullptr));
+  }
+
+  // DB actually accepts this without paranoid checks, relying on size
+  // recorded in manifest to locate the SST footer.
+  options_.paranoid_checks = false;
+  options_.skip_checking_sst_file_sizes_on_db_open = false;
+  Reopen();
+  Check(100, 100);
+
+  // But reports the issue with paranoid checks
+  options_.paranoid_checks = true;
+  Status s = TryReopen();
+  ASSERT_TRUE(s.IsCorruption());
+  ASSERT_TRUE(s.ToString().find("file size mismatch") != std::string::npos);
+
+  // ********************************************
+  // Make the file smaller with truncation.
+  // First leaving a partial footer, and then completely removing footer.
+  for (size_t bytes_lost : {8, 100}) {
+    ASSERT_OK(
+        test::TruncateFile(env_, filename, metadata[0].size - bytes_lost));
+
+    // Reported well with paranoid checks
+    options_.paranoid_checks = true;
+    s = TryReopen();
+    ASSERT_TRUE(s.IsCorruption());
+    ASSERT_TRUE(s.ToString().find("file size mismatch") != std::string::npos);
+
+    // Without paranoid checks, not reported until read
+    options_.paranoid_checks = false;
+    Reopen();
+    Check(0, 0);  // Missing data
+  }
+}
+
 TEST_F(CorruptionTest, MissingDescriptor) {
   Build(1000);
   RepairDB();

diff --git a/include/rocksdb/status.h b/include/rocksdb/status.h
index 1ab3dc4cba828543b2090fbf15bad86f69424201..39af9455991c6e9cb3bbdf2afe95326ccacbcbdc 100644 (file)
--- a/include/rocksdb/status.h
+++ b/include/rocksdb/status.h
@@ -135,6 +135,9 @@ class Status {
   Status(Code _code, SubCode _subcode, Severity _sev, const Slice& msg)
       : Status(_code, _subcode, msg, "", _sev) {}
 
+  static Status CopyAppendMessage(const Status& s, const Slice& delim,
+                                  const Slice& msg);
+
   Severity severity() const {
     MarkChecked();
     return sev_;

diff --git a/table/adaptive/adaptive_table_factory.cc b/table/adaptive/adaptive_table_factory.cc
index 5b9fe3dbd786d1450c91621fb90d55dd1149f3a8..bbea91b542693270f220eba47dfb37737587cd8e 100644 (file)
--- a/table/adaptive/adaptive_table_factory.cc
+++ b/table/adaptive/adaptive_table_factory.cc
@@ -48,8 +48,9 @@ Status AdaptiveTableFactory::NewTableReader(
     bool prefetch_index_and_filter_in_cache) const {
   Footer footer;
   IOOptions opts;
-  auto s = ReadFooterFromFile(opts, file.get(), nullptr /* prefetch_buffer */,
-                              file_size, &footer);
+  auto s =
+      ReadFooterFromFile(opts, file.get(), *table_reader_options.ioptions.fs,
+                         nullptr /* prefetch_buffer */, file_size, &footer);
   if (!s.ok()) {
     return s;
   }

diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc
index 43962ba1d1fe1664a8e5530ad5be29b013629106..a64412990597eec6653c19135f5196e2defc4ffd 100644 (file)
--- a/table/block_based/block_based_table_reader.cc
+++ b/table/block_based/block_based_table_reader.cc
@@ -638,8 +638,9 @@ Status BlockBasedTable::Open(
   IOOptions opts;
   s = file->PrepareIOOptions(ro, opts);
   if (s.ok()) {
-    s = ReadFooterFromFile(opts, file.get(), prefetch_buffer.get(), file_size,
-                           &footer, kBlockBasedTableMagicNumber);
+    s = ReadFooterFromFile(opts, file.get(), *ioptions.fs,
+                           prefetch_buffer.get(), file_size, &footer,
+                           kBlockBasedTableMagicNumber);
   }
   if (!s.ok()) {
     return s;

diff --git a/table/block_fetcher_test.cc b/table/block_fetcher_test.cc
index 82caee28257d0c3d949bc88b0f2c3eaf8b550539..f87b23c3a4c2fa18a8a141aada08ffc2bdc1b9f8 100644 (file)
--- a/table/block_fetcher_test.cc
+++ b/table/block_fetcher_test.cc
@@ -282,9 +282,9 @@ class BlockFetcherTest : public testing::Test {
     uint64_t file_size = 0;
     ASSERT_OK(env_->GetFileSize(file->file_name(), &file_size));
     IOOptions opts;
-    ASSERT_OK(ReadFooterFromFile(opts, file, nullptr /* prefetch_buffer */,
-                                 file_size, footer,
-                                 kBlockBasedTableMagicNumber));
+    ASSERT_OK(ReadFooterFromFile(opts, file, *fs_,
+                                 nullptr /* prefetch_buffer */, file_size,
+                                 footer, kBlockBasedTableMagicNumber));
   }
 
   // NOTE: compression_type returns the compression type of the fetched block

diff --git a/table/format.cc b/table/format.cc
index efde5e16903694db698a8818be1c4f62504e6840..d3347cdb8c1a3da667491b8f87eccbf52e32e791 100644 (file)
--- a/table/format.cc
+++ b/table/format.cc
@@ -264,7 +264,8 @@ void FooterBuilder::Build(uint64_t magic_number, uint32_t format_version,
   }
 }
 
-Status Footer::DecodeFrom(Slice input, uint64_t input_offset) {
+Status Footer::DecodeFrom(Slice input, uint64_t input_offset,
+                          uint64_t enforce_table_magic_number) {
   (void)input_offset;  // Future use
 
   // Only decode to unused Footer
@@ -280,6 +281,11 @@ Status Footer::DecodeFrom(Slice input, uint64_t input_offset) {
   if (legacy) {
     magic = UpconvertLegacyFooterFormat(magic);
   }
+  if (enforce_table_magic_number != 0 && enforce_table_magic_number != magic) {
+    return Status::Corruption("Bad table magic number: expected " +
+                              std::to_string(enforce_table_magic_number) +
+                              ", found " + std::to_string(magic));
+  }
   table_magic_number_ = magic;
   block_trailer_size_ = BlockTrailerSizeForMagicNumber(magic);
 
@@ -346,7 +352,7 @@ std::string Footer::ToString() const {
 }
 
 Status ReadFooterFromFile(const IOOptions& opts, RandomAccessFileReader* file,
-                          FilePrefetchBuffer* prefetch_buffer,
+                          FileSystem& fs, FilePrefetchBuffer* prefetch_buffer,
                           uint64_t file_size, Footer* footer,
                           uint64_t enforce_table_magic_number) {
   if (file_size < Footer::kMinEncodedLength) {
@@ -390,29 +396,27 @@ Status ReadFooterFromFile(const IOOptions& opts, RandomAccessFileReader* file,
   // Check that we actually read the whole footer from the file. It may be
   // that size isn't correct.
   if (footer_input.size() < Footer::kMinEncodedLength) {
-    // FIXME: this error message is bad. We should be checking whether the
-    // provided file_size matches what's on disk, at least in this case.
-    // Unfortunately FileSystem/Env does not provide a way to get the size
-    // of an open file, so getting file size requires a full path seek.
-    return Status::Corruption("file is too short (" +
-                              std::to_string(file_size) +
-                              " bytes) to be an "
-                              "sstable" +
-                              file->file_name());
+    uint64_t size_on_disk = 0;
+    if (fs.GetFileSize(file->file_name(), IOOptions(), &size_on_disk, nullptr)
+            .ok()) {
+      // Similar to CheckConsistency message, but not completely sure the
+      // expected size always came from manifest.
+      return Status::Corruption("Sst file size mismatch: " + file->file_name() +
+                                ". Expected " + std::to_string(file_size) +
+                                ", actual size " +
+                                std::to_string(size_on_disk) + "\n");
+    } else {
+      return Status::Corruption(
+          "Missing SST footer data in file " + file->file_name() +
+          " File too short? Expected size: " + std::to_string(file_size));
+    }
   }
 
-  s = footer->DecodeFrom(footer_input, read_offset);
+  s = footer->DecodeFrom(footer_input, read_offset, enforce_table_magic_number);
   if (!s.ok()) {
+    s = Status::CopyAppendMessage(s, " in ", file->file_name());
     return s;
   }
-  if (enforce_table_magic_number != 0 &&
-      enforce_table_magic_number != footer->table_magic_number()) {
-    return Status::Corruption("Bad table magic number: expected " +
-                              std::to_string(enforce_table_magic_number) +
-                              ", found " +
-                              std::to_string(footer->table_magic_number()) +
-                              " in " + file->file_name());
-  }
   return Status::OK();
 }
 

diff --git a/table/format.h b/table/format.h
index ffb9fb0ca43286c0534d07a282510b9820059f04..23e6b891cba26e94e956d4e529bb4c15ac5dd1ef 100644 (file)
--- a/table/format.h
+++ b/table/format.h
@@ -138,7 +138,10 @@ class Footer {
   // Deserialize a footer (populate fields) from `input` and check for various
   // corruptions. `input_offset` is the offset within the target file of
   // `input` buffer (future use).
-  Status DecodeFrom(Slice input, uint64_t input_offset);
+  // If enforce_table_magic_number != 0, will return corruption if table magic
+  // number is not equal to enforce_table_magic_number.
+  Status DecodeFrom(Slice input, uint64_t input_offset,
+                    uint64_t enforce_table_magic_number = 0);
 
   // Table magic number identifies file as RocksDB SST file and which kind of
   // SST format is use.
@@ -238,7 +241,7 @@ class FooterBuilder {
 // If enforce_table_magic_number != 0, ReadFooterFromFile() will return
 // corruption if table_magic number is not equal to enforce_table_magic_number
 Status ReadFooterFromFile(const IOOptions& opts, RandomAccessFileReader* file,
-                          FilePrefetchBuffer* prefetch_buffer,
+                          FileSystem& fs, FilePrefetchBuffer* prefetch_buffer,
                           uint64_t file_size, Footer* footer,
                           uint64_t enforce_table_magic_number = 0);
 

diff --git a/table/meta_blocks.cc b/table/meta_blocks.cc
index 78a62359db8dc8998a8912dca0d9c3512053a990..6530f6a80c49950debb2bb7bb4ddbc2223cfb726 100644 (file)
--- a/table/meta_blocks.cc
+++ b/table/meta_blocks.cc
@@ -479,8 +479,8 @@ Status ReadMetaIndexBlockInFile(RandomAccessFileReader* file,
                                 Footer* footer_out) {
   Footer footer;
   IOOptions opts;
-  auto s = ReadFooterFromFile(opts, file, prefetch_buffer, file_size, &footer,
-                              table_magic_number);
+  auto s = ReadFooterFromFile(opts, file, *ioptions.fs, prefetch_buffer,
+                              file_size, &footer, table_magic_number);
   if (!s.ok()) {
     return s;
   }

diff --git a/table/sst_file_dumper.cc b/table/sst_file_dumper.cc
index 122f0995acd729b9eb522f50f46f6c60e1a7ae56..3357099e8297821fd9583846d2fe02a8c2d83176 100644 (file)
--- a/table/sst_file_dumper.cc
+++ b/table/sst_file_dumper.cc
@@ -113,7 +113,7 @@ Status SstFileDumper::GetTableReader(const std::string& file_path) {
                                  static_cast<size_t>(prefetch_size),
                                  Env::IO_TOTAL /* rate_limiter_priority */);
 
-    s = ReadFooterFromFile(opts, file_.get(), &prefetch_buffer, file_size,
+    s = ReadFooterFromFile(opts, file_.get(), *fs, &prefetch_buffer, file_size,
                            &footer);
   }
   if (s.ok()) {

diff --git a/table/table_test.cc b/table/table_test.cc
index af9c177e8141caf1e12879f341f050fee7d494d0..d5fff82da4fb69349866a778b8e5acfac4a55ccc 100644 (file)
--- a/table/table_test.cc
+++ b/table/table_test.cc
@@ -2256,8 +2256,9 @@ TEST_P(BlockBasedTableTest, BadChecksumType) {
   const MutableCFOptions new_moptions(options);
   Status s = c.Reopen(new_ioptions, new_moptions);
   ASSERT_NOK(s);
+  // "test" is file name
   ASSERT_EQ(s.ToString(),
-            "Corruption: Corrupt or unsupported checksum type: 123");
+            "Corruption: Corrupt or unsupported checksum type: 123 in test");
 }
 
 namespace {
@@ -4806,9 +4807,9 @@ TEST_P(BlockBasedTableTest, PropertiesBlockRestartPointTest) {
 
     Footer footer;
     IOOptions opts;
-    ASSERT_OK(ReadFooterFromFile(opts, file, nullptr /* prefetch_buffer */,
-                                 file_size, &footer,
-                                 kBlockBasedTableMagicNumber));
+    ASSERT_OK(ReadFooterFromFile(opts, file, *FileSystem::Default(),
+                                 nullptr /* prefetch_buffer */, file_size,
+                                 &footer, kBlockBasedTableMagicNumber));
 
     auto BlockFetchHelper = [&](const BlockHandle& handle, BlockType block_type,
                                 BlockContents* contents) {
@@ -4892,7 +4893,7 @@ TEST_P(BlockBasedTableTest, PropertiesMetaBlockLast) {
   // read footer
   Footer footer;
   IOOptions opts;
-  ASSERT_OK(ReadFooterFromFile(opts, table_reader.get(),
+  ASSERT_OK(ReadFooterFromFile(opts, table_reader.get(), *FileSystem::Default(),
                                nullptr /* prefetch_buffer */, table_size,
                                &footer, kBlockBasedTableMagicNumber));
 
@@ -4970,7 +4971,7 @@ TEST_P(BlockBasedTableTest, SeekMetaBlocks) {
   // read footer
   Footer footer;
   IOOptions opts;
-  ASSERT_OK(ReadFooterFromFile(opts, table_reader.get(),
+  ASSERT_OK(ReadFooterFromFile(opts, table_reader.get(), *FileSystem::Default(),
                                nullptr /* prefetch_buffer */, table_size,
                                &footer, kBlockBasedTableMagicNumber));
 

diff --git a/util/status.cc b/util/status.cc
index 72fdfdbcc917644f564ccc3f9440bd7cdc873888..1156b10ef4988840c75225ded3d0c485c92779b4 100644 (file)
--- a/util/status.cc
+++ b/util/status.cc
@@ -69,6 +69,13 @@ Status::Status(Code _code, SubCode _subcode, const Slice& msg,
   state_.reset(result);
 }
 
+Status Status::CopyAppendMessage(const Status& s, const Slice& delim,
+                                 const Slice& msg) {
+  // (No attempt at efficiency)
+  return Status(s.code(), s.subcode(), s.severity(),
+                std::string(s.getState()) + delim.ToString() + msg.ToString());
+}
+
 std::string Status::ToString() const {
 #ifdef ROCKSDB_ASSERT_STATUS_CHECKED
   checked_ = true;