From a366de2a637ef13648bb03f26ce7f5f17d5f2d83 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 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sstables/compaction.cc b/sstables/compaction.cc index 8eff060c0c..66b140eb12 100644 --- a/sstables/compaction.cc +++ b/sstables/compaction.cc @@ -212,16 +212,18 @@ public: }; struct compaction_writer { + shared_sstable sst; // We use a ptr for pointer stability and so that it can be null // when using a noop monitor. sstable_writer writer; // The order in here is important. A monitor must be destroyed before the writer it is monitoring since it has a // periodic timer that checks the writer. + // The writer must be destroyed before the shared_sstable since the it may depend on the sstable + // (as in the mx::writer over compressed_file_data_sink_impl case that depends on sstables::compression). std::unique_ptr monitor; - shared_sstable sst; compaction_writer(std::unique_ptr monitor, sstable_writer writer, shared_sstable sst) - : writer(std::move(writer)), monitor(std::move(monitor)), sst(std::move(sst)) {} + : sst(std::move(sst)), writer(std::move(writer)), monitor(std::move(monitor)) {} compaction_writer(sstable_writer writer, shared_sstable sst) : compaction_writer(nullptr, std::move(writer), std::move(sst)) {} };