diff --git a/db/corruption_test.cc b/db/corruption_test.cc index 8ccac6130..7544d098c 100644 --- 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(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(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(db_); + ASSERT_OK(dbi->TEST_FlushMemTable()); + Check(100, 100); + + // ******************************************** + // Make the file bigger by appending to it + std::vector 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 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 1ab3dc4cb..39af94559 100644 --- 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 5b9fe3dbd..bbea91b54 100644 --- 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 43962ba1d..a64412990 100644 --- 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 82caee282..f87b23c3a 100644 --- 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 efde5e169..d3347cdb8 100644 --- 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 ffb9fb0ca..23e6b891c 100644 --- 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 78a62359d..6530f6a80 100644 --- 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 122f0995a..3357099e8 100644 --- 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(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 af9c177e8..d5fff82da 100644 --- 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 72fdfdbcc..1156b10ef 100644 --- 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;