From 81534dde6e09e2fc840067a61cd87a4c3924f881 Mon Sep 17 00:00:00 2001 From: yiguolei Date: Wed, 15 Jan 2025 11:01:38 +0800 Subject: [PATCH] [bugfix](memtable) arena is freed early and will cause use after free (#46997) ### What problem does this PR solve? Related PR: https://github.com/apache/doris/pull/40912 Problem Summary: Do not reset _arena in MemTable::to_block(), because it is still used in ~MemTable() when releasing agg places Fix the following use-after-free Use: ==3628099==ERROR: AddressSanitizer: heap-use-after-free on address 0x52100381be60 at pc 0x5648f30893f8 bp 0x7f8842433310 sp 0x7f8842433308 READ of size 8 at 0x52100381be60 thread T4767 (wg_flush_broker) #0 0x5648f30893f7 in phmap::priv::raw_hash_set, phmap::Hash, phmap::EqualTo, std::allocator>::destroy_slots() doris/thirdparty/installed/include/parallel_hashmap/phmap.h:1992:14 #1 0x5648f30936f6 in phmap::priv::raw_hash_set, phmap::Hash, phmap::EqualTo, std::allocator>::~raw_hash_set() doris/thirdparty/installed/include/parallel_hashmap/phmap.h:1236:23 #2 0x5648f3089276 in phmap::flat_hash_set, phmap::EqualTo, std::allocator>::~flat_hash_set() doris/thirdparty/installed/include/parallel_hashmap/phmap.h:4577:7 #3 0x5648f308922a in doris::BitmapValue::~BitmapValue() doris/be/src/util/bitmap_value.h:824:7 #4 0x56490d319fa6 in doris::vectorized::AggregateFunctionBitmapData::~AggregateFunctionBitmapData() doris/be/src/vec/aggregate_functions/aggregate_function_bitmap.h:127:8 #5 0x56490d49636a in doris::vectorized::IAggregateFunctionDataHelper, doris::vectorized::AggregateFunctionBitmapOp>::destroy(char*) const doris/be/src/vec/aggregate_functions/aggregate_function.h:563:92 #6 0x5648f68376e9 in doris::MemTable::~MemTable() doris/be/src/olap/memtable.cpp:159:27 Free: 0x52100381be60 is located 352 bytes inside of 4096-byte region [0x52100381bd00,0x52100381cd00) freed by thread T4767 (wg_flush_broker) here: #0 0x5648f2f3ee46 in free (doris/output/be/lib/doris_be+0x57418e46) (BuildId: 298b9c91a1ec8fe0) #1 0x5648f3080dfc in DefaultMemoryAllocator::free(void*) doris/be/src/vec/common/allocator.h:108:41 #2 0x5648f3080b3f in Allocator::free(void*, unsigned long) doris/be/src/vec/common/allocator.h:323:13 #3 0x5648f30b6dee in doris::vectorized::Arena::Chunk::~Chunk() doris/be/src/vec/common/arena.h:77:31 #4 0x5648f30b6d1f in doris::vectorized::Arena::~Arena() doris/be/src/vec/common/arena.h:151:16 #5 0x5648f30b695a in std::default_delete::operator()(doris::vectorized::Arena*) const env/ldb_toolchain/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/unique_ptr.h:99:2 #6 0x5648f30b67c8 in std::__uniq_ptr_impl>::reset(doris::vectorized::Arena*) env/ldb_toolchain/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/unique_ptr.h:211:4 #7 0x5648f30b5d8c in std::unique_ptr>::reset(doris::vectorized::Arena*) env/ldb_toolchain/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/unique_ptr.h:509:7 #8 0x5648f684253b in doris::MemTable::_to_block(std::unique_ptr>*) doris/be/src/olap/memtable.cpp:522:12 #9 0x5648f6842ac5 in doris::MemTable::to_block(std::unique_ptr>*) doris/be/src/olap/memtable.cpp:528:5 #10 0x5648f6907a72 in doris::FlushToken::_do_flush_memtable(doris::MemTable*, int, long*) doris/be/src/olap/memtable_flush_executor.cpp:144:9 #11 0x5648f690932c in doris::FlushToken::_flush_memtable(std::shared_ptr, int, long) doris/be/src/olap/memtable_flush_executor.cpp:183:16 #12 0x5648f6915d18 in doris::MemtableFlushTask::run() doris/be/src/olap/memtable_flush_executor.cpp:60:20 --- be/src/olap/memtable.cpp | 52 +++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/be/src/olap/memtable.cpp b/be/src/olap/memtable.cpp index f8cc79b205535f..047d995bc3e3ea 100644 --- a/be/src/olap/memtable.cpp +++ b/be/src/olap/memtable.cpp @@ -144,6 +144,34 @@ void MemTable::_init_agg_functions(const vectorized::Block* block) { MemTable::~MemTable() { SCOPED_SWITCH_THREAD_MEM_TRACKER_LIMITER(_query_thread_context.query_mem_tracker); + { + SCOPED_CONSUME_MEM_TRACKER(_mem_tracker); + g_memtable_cnt << -1; + if (_keys_type != KeysType::DUP_KEYS) { + for (auto it = _row_in_blocks.begin(); it != _row_in_blocks.end(); it++) { + if (!(*it)->has_init_agg()) { + continue; + } + // We should release agg_places here, because they are not released when a + // load is canceled. + for (size_t i = _tablet_schema->num_key_columns(); i < _num_columns; ++i) { + auto function = _agg_functions[i]; + DCHECK(function != nullptr); + function->destroy((*it)->agg_places(i)); + } + } + } + std::for_each(_row_in_blocks.begin(), _row_in_blocks.end(), + std::default_delete()); + // Arena has to be destroyed after agg state, because some agg state's memory may be + // allocated in arena. + _arena.reset(); + _vec_row_comparator.reset(); + _row_in_blocks.clear(); + _agg_functions.clear(); + _input_mutable_block.clear(); + _output_mutable_block.clear(); + } if (_is_flush_success) { // If the memtable is flush success, then its memtracker's consumption should be 0 if (_mem_tracker->consumption() != 0 && config::crash_in_memory_tracker_inaccurate) { @@ -151,28 +179,6 @@ MemTable::~MemTable() { << _mem_tracker->consumption(); } } - g_memtable_cnt << -1; - if (_keys_type != KeysType::DUP_KEYS) { - for (auto it = _row_in_blocks.begin(); it != _row_in_blocks.end(); it++) { - if (!(*it)->has_init_agg()) { - continue; - } - // We should release agg_places here, because they are not released when a - // load is canceled. - for (size_t i = _tablet_schema->num_key_columns(); i < _num_columns; ++i) { - auto function = _agg_functions[i]; - DCHECK(function != nullptr); - function->destroy((*it)->agg_places(i)); - } - } - } - std::for_each(_row_in_blocks.begin(), _row_in_blocks.end(), std::default_delete()); - _arena.reset(); - _vec_row_comparator.reset(); - _row_in_blocks.clear(); - _agg_functions.clear(); - _input_mutable_block.clear(); - _output_mutable_block.clear(); } int RowInBlockComparator::operator()(const RowInBlock* left, const RowInBlock* right) const { @@ -629,8 +635,6 @@ Status MemTable::_to_block(std::unique_ptr* res) { RETURN_IF_ERROR(_sort_by_cluster_keys()); } _input_mutable_block.clear(); - // After to block, all data in arena is saved in the block - _arena.reset(); *res = vectorized::Block::create_unique(_output_mutable_block.to_block()); return Status::OK(); }