From 9ab21dad5236c40ac2a6a14545596dc413a2c5ed Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Wed, 21 Jul 2021 12:18:09 -0700 Subject: [PATCH] Fix use-after-free on implicit temporary FileOptions Summary: FileOptions has an implicit conversion from EnvOptions and some internal APIs take `const FileOptions&` and save the reference, which is counter to Google C++ guidelines, > Avoid defining functions that require a const reference parameter to outlive the call, because const reference parameters bind to temporaries. Instead, find a way to eliminate the lifetime requirement (for example, by copying the parameter), or pass it by const pointer and document the lifetime and non-null requirements. This is at least a problem for repair.cc, which passes an EnvOptions to TableCache(), which would save a reference to the temporary copy as FileOptions. This was unfortunately only caught as a side effect of changes in #8544. This change fixes the repair.cc case and updates the involved internal APIs that save a reference to use `const FileOptions*` instead. Unfortunately, I don't know how to get any of our sanitizers to reliably report bugs like this, so I can't rule out more existing in our codebase. Test Plan: Test that issues seen with #8544 are fixed (can reproduce on AWS EC2) --- db/column_family.cc | 8 ++++---- db/column_family.h | 5 +++-- db/repair.cc | 19 ++++++++++--------- db/table_cache.cc | 4 ++-- db/table_cache.h | 2 +- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/db/column_family.cc b/db/column_family.cc index 47389a670ad..7e6a0b3f651 100644 --- a/db/column_family.cc +++ b/db/column_family.cc @@ -505,7 +505,7 @@ ColumnFamilyData::ColumnFamilyData( uint32_t id, const std::string& name, Version* _dummy_versions, Cache* _table_cache, WriteBufferManager* write_buffer_manager, const ColumnFamilyOptions& cf_options, const ImmutableDBOptions& db_options, - const FileOptions& file_options, ColumnFamilySet* column_family_set, + const FileOptions* file_options, ColumnFamilySet* column_family_set, BlockCacheTracer* const block_cache_tracer, const std::shared_ptr& io_tracer, const std::string& db_session_id) @@ -1462,14 +1462,14 @@ ColumnFamilySet::ColumnFamilySet(const std::string& dbname, const std::shared_ptr& io_tracer, const std::string& db_session_id) : max_column_family_(0), + file_options_(file_options), dummy_cfd_(new ColumnFamilyData( ColumnFamilyData::kDummyColumnFamilyDataId, "", nullptr, nullptr, - nullptr, ColumnFamilyOptions(), *db_options, file_options, nullptr, + nullptr, ColumnFamilyOptions(), *db_options, &file_options_, nullptr, block_cache_tracer, io_tracer, db_session_id)), default_cfd_cache_(nullptr), db_name_(dbname), db_options_(db_options), - file_options_(file_options), table_cache_(table_cache), write_buffer_manager_(_write_buffer_manager), write_controller_(_write_controller), @@ -1541,7 +1541,7 @@ ColumnFamilyData* ColumnFamilySet::CreateColumnFamily( assert(column_families_.find(name) == column_families_.end()); ColumnFamilyData* new_cfd = new ColumnFamilyData( id, name, dummy_versions, table_cache_, write_buffer_manager_, options, - *db_options_, file_options_, this, block_cache_tracer_, io_tracer_, + *db_options_, &file_options_, this, block_cache_tracer_, io_tracer_, db_session_id_); column_families_.insert({name, id}); column_family_data_.insert({id, new_cfd}); diff --git a/db/column_family.h b/db/column_family.h index d73c3dda998..c4b8665331b 100644 --- a/db/column_family.h +++ b/db/column_family.h @@ -531,7 +531,7 @@ class ColumnFamilyData { WriteBufferManager* write_buffer_manager, const ColumnFamilyOptions& options, const ImmutableDBOptions& db_options, - const FileOptions& file_options, + const FileOptions* file_options, ColumnFamilySet* column_family_set, BlockCacheTracer* const block_cache_tracer, const std::shared_ptr& io_tracer, @@ -722,6 +722,8 @@ class ColumnFamilySet { std::unordered_map column_family_data_; uint32_t max_column_family_; + const FileOptions file_options_; + ColumnFamilyData* dummy_cfd_; // We don't hold the refcount here, since default column family always exists // We are also not responsible for cleaning up default_cfd_cache_. This is @@ -731,7 +733,6 @@ class ColumnFamilySet { const std::string db_name_; const ImmutableDBOptions* const db_options_; - const FileOptions file_options_; Cache* table_cache_; WriteBufferManager* write_buffer_manager_; WriteController* write_controller_; diff --git a/db/repair.cc b/db/repair.cc index 4a710db965c..c93fe5f1f70 100644 --- a/db/repair.cc +++ b/db/repair.cc @@ -94,7 +94,7 @@ class Repairer { const ColumnFamilyOptions& unknown_cf_opts, bool create_unknown_cfs) : dbname_(dbname), env_(db_options.env), - env_options_(), + file_options_(), db_options_(SanitizeOptions(dbname_, db_options)), immutable_db_options_(ImmutableDBOptions(db_options_)), icmp_(default_cf_opts.comparator), @@ -112,14 +112,15 @@ class Repairer { table_cache_( // TODO: db_session_id for TableCache should be initialized after // db_session_id_ is set. - new TableCache(default_iopts_, env_options_, raw_table_cache_.get(), + new TableCache(default_iopts_, &file_options_, + raw_table_cache_.get(), /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr, /*db_session_id*/ "")), wb_(db_options_.db_write_buffer_size), wc_(db_options_.delayed_write_rate), // TODO: db_session_id for VersionSet should be initialized after // db_session_id_ is set and use it for initialization. - vset_(dbname_, &immutable_db_options_, env_options_, + vset_(dbname_, &immutable_db_options_, file_options_, raw_table_cache_.get(), &wb_, &wc_, /*block_cache_tracer=*/nullptr, /*io_tracer=*/nullptr, /*db_session_id*/ ""), @@ -249,7 +250,7 @@ class Repairer { std::string const dbname_; std::string db_session_id_; Env* const env_; - const EnvOptions env_options_; + const FileOptions file_options_; const DBOptions db_options_; const ImmutableDBOptions immutable_db_options_; const InternalKeyComparator icmp_; @@ -366,7 +367,7 @@ class Repairer { const auto& fs = env_->GetFileSystem(); std::unique_ptr lfile_reader; Status status = SequentialFileReader::Create( - fs, logname, fs->OptimizeForLogRead(env_options_), &lfile_reader, + fs, logname, fs->OptimizeForLogRead(file_options_), &lfile_reader, nullptr); if (!status.ok()) { return status; @@ -458,7 +459,7 @@ class Repairer { meta.fd.GetNumber()); status = BuildTable( dbname_, /* versions */ nullptr, immutable_db_options_, tboptions, - env_options_, table_cache_.get(), iter.get(), + file_options_, table_cache_.get(), iter.get(), std::move(range_del_iters), &meta, nullptr /* blob_file_additions */, {}, kMaxSequenceNumber, snapshot_checker, false /* paranoid_file_checks*/, nullptr /* internal_stats */, &io_s, @@ -510,8 +511,8 @@ class Repairer { file_size); std::shared_ptr props; if (status.ok()) { - status = table_cache_->GetTableProperties(env_options_, icmp_, t->meta.fd, - &props); + status = table_cache_->GetTableProperties(file_options_, icmp_, + t->meta.fd, &props); } if (status.ok()) { t->column_family_id = static_cast(props->column_family_id); @@ -551,7 +552,7 @@ class Repairer { ReadOptions ropts; ropts.total_order_seek = true; InternalIterator* iter = table_cache_->NewIterator( - ropts, env_options_, cfd->internal_comparator(), t->meta, + ropts, file_options_, cfd->internal_comparator(), t->meta, nullptr /* range_del_agg */, cfd->GetLatestMutableCFOptions()->prefix_extractor.get(), /*table_reader_ptr=*/nullptr, /*file_read_hist=*/nullptr, diff --git a/db/table_cache.cc b/db/table_cache.cc index 2e4d2a58ae4..53a6323bf0d 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -66,12 +66,12 @@ void AppendVarint64(IterKey* key, uint64_t v) { const int kLoadConcurency = 128; TableCache::TableCache(const ImmutableOptions& ioptions, - const FileOptions& file_options, Cache* const cache, + const FileOptions* file_options, Cache* const cache, BlockCacheTracer* const block_cache_tracer, const std::shared_ptr& io_tracer, const std::string& db_session_id) : ioptions_(ioptions), - file_options_(file_options), + file_options_(*file_options), cache_(cache), immortal_tables_(false), block_cache_tracer_(block_cache_tracer), diff --git a/db/table_cache.h b/db/table_cache.h index 0c263afe56e..2138eb4a207 100644 --- a/db/table_cache.h +++ b/db/table_cache.h @@ -49,7 +49,7 @@ class HistogramImpl; class TableCache { public: TableCache(const ImmutableOptions& ioptions, - const FileOptions& storage_options, Cache* cache, + const FileOptions* storage_options, Cache* cache, BlockCacheTracer* const block_cache_tracer, const std::shared_ptr& io_tracer, const std::string& db_session_id);