From e05e7b2a9834c956f28e30da8317222dde31792f Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 27 Dec 2020 16:57:26 +0200 Subject: [PATCH] compaction: compaction_writer: destroy shared_sstable after the sstable_writer sstable_writer may depend on the sstable throughout its whole lifecycle. If the sstable is freed before the sstable_writer we might hit use-after-free as in the follwing case: ``` std::_Deque_iterator::operator+=(long) at /usr/include/c++/10/bits/stl_deque.h:240 (inlined by) std::operator+(std::_Deque_iterator const&, long) at /usr/include/c++/10/bits/stl_deque.h:378 (inlined by) std::_Deque_iterator::operator[](long) const at /usr/include/c++/10/bits/stl_deque.h:252 (inlined by) std::deque >::operator[](unsigned long) at /usr/include/c++/10/bits/stl_deque.h:1327 (inlined by) sstables::compression::segmented_offsets::push_back(unsigned long, sstables::compression::segmented_offsets::state&) at ./sstables/compress.cc:214 sstables::compression::segmented_offsets::writer::push_back(unsigned long) at ./sstables/compress.hh:123 (inlined by) compressed_file_data_sink_impl::put(seastar::temporary_buffer) at ./sstables/compress.cc:519 seastar::output_stream::put(seastar::temporary_buffer) at table.cc:? (inlined by) seastar::output_stream::put(seastar::temporary_buffer) at ././seastar/include/seastar/core/iostream-impl.hh:432 seastar::output_stream::flush() at table.cc:? seastar::output_stream::close() at table.cc:? sstables::file_writer::close() at sstables.cc:? sstables::mc::writer::~writer() at writer.cc:? (inlined by) sstables::mc::writer::~writer() at ./sstables/mx/writer.cc:790 sstables::mc::writer::~writer() at writer.cc:? flat_mutation_reader::impl::consumer_adapter > >::~consumer_adapter() at compaction.cc:? (inlined by) std::_Optional_payload_base::_M_destroy() at /usr/include/c++/10/optional:260 (inlined by) std::_Optional_payload_base::_M_reset() at /usr/include/c++/10/optional:280 (inlined by) std::_Optional_payload::~_Optional_payload() at /usr/include/c++/10/optional:401 (inlined by) std::_Optional_base::~_Optional_base() at /usr/include/c++/10/optional:474 (inlined by) std::optional::~optional() at /usr/include/c++/10/optional:659 (inlined by) sstables::compacting_sstable_writer::~compacting_sstable_writer() at ./sstables/compaction.cc:229 (inlined by) compact_mutation<(emit_only_live_rows)0, (compact_for_sstables)1, sstables::compacting_sstable_writer, noop_compacted_fragments_consumer>::~compact_mutation() at ././mutation_compactor.hh:468 (inlined by) compact_for_compaction::~compact_for_compaction() at ././mutation_compactor.hh:538 (inlined by) std::default_delete >::operator()(compact_for_compaction*) const at /usr/include/c++/10/bits/unique_ptr.h:85 (inlined by) std::unique_ptr, std::default_delete > >::~unique_ptr() at /usr/include/c++/10/bits/unique_ptr.h:361 (inlined by) stable_flattened_mutations_consumer >::~stable_flattened_mutations_consumer() at ././mutation_reader.hh:342 (inlined by) flat_mutation_reader::impl::consumer_adapter > >::~consumer_adapter() at ././flat_mutation_reader.hh:201 auto flat_mutation_reader::impl::consume_in_thread >, flat_mutation_reader::no_filter>(stable_flattened_mutations_consumer >, flat_mutation_reader::no_filter, std::chrono::time_point > >) at ././flat_mutation_reader.hh:272 (inlined by) auto flat_mutation_reader::consume_in_thread >, flat_mutation_reader::no_filter>(stable_flattened_mutations_consumer >, flat_mutation_reader::no_filter, std::chrono::time_point > >) at ././flat_mutation_reader.hh:383 (inlined by) auto flat_mutation_reader::consume_in_thread > >(stable_flattened_mutations_consumer >, std::chrono::time_point > >) at ././flat_mutation_reader.hh:389 (inlined by) seastar::future sstables::compaction::setup(noop_compacted_fragments_consumer)::{lambda(flat_mutation_reader)#1}::operator()(flat_mutation_reader)::{lambda()#1}::operator()() at ./sstables/compaction.cc:612 ``` What happens here is that: compressed_file_data_sink_impl(output_stream out, sstables::compression* cm, sstables::local_compression lc) : _out(std::move(out)) , _compression_metadata(cm) , _offsets(_compression_metadata->offsets.get_writer()) , _compression(lc) , _full_checksum(ChecksumType::init_checksum()) _compression_metadata points to a buffer held by the sstable object. and _compression_metadata->offsets.get_writer returns a writer that keeps a reference to the segmented_offsets in the sstables::compression that is used in the ~writer -> close path. Fixes #7821 Signed-off-by: Benny Halevy Message-Id: <20201227145726.33319-1-bhalevy@scylladb.com> (cherry picked from commit 8a745a0ee0cd53b047079ef4ad9f63a50b4712ff) --- sstables/compaction.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sstables/compaction.cc b/sstables/compaction.cc index 4176fd2312..2ac3b6fdd8 100644 --- a/sstables/compaction.cc +++ b/sstables/compaction.cc @@ -126,8 +126,8 @@ static std::vector get_uncompacting_sstables(column_family& cf, class compaction; struct compaction_writer { - sstable_writer writer; shared_sstable sst; + sstable_writer writer; }; class compacting_sstable_writer { @@ -786,7 +786,8 @@ public: cfg.max_sstable_size = _max_sstable_size; cfg.monitor = &_active_write_monitors.back(); cfg.run_identifier = _run_identifier; - return compaction_writer{sst->get_writer(*_schema, partitions_per_sstable(), cfg, get_encoding_stats(), priority), sst}; + auto writer = sst->get_writer(*_schema, partitions_per_sstable(), cfg, get_encoding_stats(), priority); + return compaction_writer{std::move(sst), std::move(writer)}; } virtual void stop_sstable_writer(compaction_writer* writer) override { @@ -1268,7 +1269,8 @@ public: // sstables generated for a given shard will share the same run identifier. cfg.run_identifier = _run_identifiers.at(shard); auto&& priority = service::get_local_compaction_priority(); - return compaction_writer{sst->get_writer(*_schema, partitions_per_sstable(shard), cfg, get_encoding_stats(), priority, shard), sst}; + auto writer = sst->get_writer(*_schema, partitions_per_sstable(shard), cfg, get_encoding_stats(), priority, shard); + return compaction_writer{std::move(sst), std::move(writer)}; } void on_new_partition() override {}