Skip to content

Commit 9aefe89

Browse files
tchaikovabitdrag
authored andcommitted
os/bluestore: fix memory leak in HybridAllocator destructor
Fix a memory leak where HybridAllocator's embedded BitmapAllocator (bmap_alloc) was not properly cleaned up during destruction. The issue occurred because virtual function calls in destructors don't dispatch to derived class implementations - when AvlAllocator::~AvlAllocator() calls shutdown(), it invokes AvlAllocator::shutdown() rather than HybridAllocatorBase::shutdown(), leaving bmap_alloc unfreed. This issue only affects unit tests and does not impact production, as BlueFS always calls shutdown() explicitly when stopping allocators in BlueFS::_stop_alloc() This change has minimal impact on existing code that already calls shutdown() explicitly, since shutdown() has idempotent behavior and can be safely called multiple times. Additionally, destructors are only invoked during system teardown, so the potential double shutdown() call does not affect runtime performance. Changes: - Replace raw pointer bmap_alloc with unique_ptr for automatic cleanup - Add explicit shutdown() call in BitmapAllocator destructor for RAII - Replace manual bmap_alloc->shutdown() with bmap_alloc.reset() - Add explicit default destructor to HybridAllocatorBase for clarity This eliminates the need for manual shutdown() calls and ensures proper resource cleanup through RAII principles. Fixes ASan-reported indirect leak of BitmapAllocator resources. Please see the leak reported by ASan for more details: ``` Indirect leak of 23 byte(s) in 1 object(s) allocated from: #0 0x7fe30b121a2d in operator new(unsigned long) /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_new_delete.cpp:86 ceph#1 0x5614e057ee11 in std::__new_allocator<char>::allocate(unsigned long, void const*) /usr/include/c++/15.1.1/bits/new_allocator.h:151 ceph#2 0x5614e057d279 in std::allocator<char>::allocate(unsigned long) /usr/include/c++/15.1.1/bits/allocator.h:203 ceph#3 0x5614e057d279 in std::allocator_traits<std::allocator<char> >::allocate(std::allocator<char>&, unsigned long) /usr/include/c++/15.1.1/bits/alloc_traits.h:614 ceph#4 0x5614e057d279 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_S_allocate(std::allocator<char>&, unsigned long) /usr/include/c++/15.1.1/bits/basic_string.h:142 ceph#5 0x5614e057cf27 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long) /usr/include/c++/15.1.1/bits/basic_string.tcc:164 ceph#6 0x5614e05fe91b in void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<true>(char const*, unsigned long) /usr/include/c++/15.1.1/bits/basic_string.tcc:291 ceph#7 0x5614e05f3cd4 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /usr/include/c++/15.1. 1/bits/basic_string.h:617 ceph#8 0x5614e069b892 in ceph::mutex_debug_detail::mutex_debug_impl<false>::mutex_debug_impl(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, bool) /home/kefu/dev/ceph/src/common/mutex_debug. h:103 ceph#9 0x5614e06b8586 in ceph::mutex_debug_detail::mutex_debug_impl<false> ceph::make_mutex<char const (&) [23]>(char const (&) [23]) /home/kefu/dev/ceph/src/common/ceph_mutex.h:118 ceph#10 0x5614e06b8350 in AllocatorLevel02<AllocatorLevel01Loose>::AllocatorLevel02() /home/kefu/dev/ceph/src/os/bluestore/fastbmap_allocator_impl.h:526 ceph#11 0x5614e06b393b in BitmapAllocator::BitmapAllocator(ceph::common::CephContext*, long, long, std::basic_string_view<char, std::char_traits<char> >) /home/kefu/dev/ceph/src/os/bluestore/BitmapAllocator.cc:16 ceph#12 0x5614e072755f in HybridAllocatorBase<AvlAllocator>::_spillover_range(unsigned long, unsigned long) /home/kefu/dev/ceph/src/os/bluestore/HybridAllocator_impl.h:123 ceph#13 0x5614e06c9045 in AvlAllocator::_try_insert_range(unsigned long, unsigned long, boost::intrusive::tree_iterator<boost::intrusive::mhtraits<range_seg_t, boost::intrusive::avl_set_member_hook<>, &range_seg_t::offset_hook>, false>*) /home/kefu/dev/ceph/src/os/bluestore/AvlAllocator.h:210 ceph#14 0x5614e06c0104 in AvlAllocator::_add_to_tree(unsigned long, unsigned long) /home/kefu/dev/ceph/src/os/bluestore/AvlAllocator.cc:136 ceph#15 0x5614e0586e5f in HybridAllocatorBase<AvlAllocator>::_add_to_tree(unsigned long, unsigned long) /home/kefu/dev/ceph/src/os/bluestore/HybridAllocator.h:110 ceph#16 0x5614e06c6559 in AvlAllocator::init_add_free(unsigned long, unsigned long) /home/kefu/dev/ceph/src/os/bluestore/AvlAllocator.cc:494 ceph#17 0x5614e0582943 in HybridAllocator_fragmentation_Test::TestBody() /home/kefu/dev/ceph/src/test/objectstore/hybrid_allocator_test.cc:285 ceph#18 0x5614e061bee3 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2653 ceph#19 0x5614e0606562 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2689` ``` Signed-off-by: Kefu Chai <tchaikov@gmail.com>
1 parent 23ad32d commit 9aefe89

File tree

3 files changed

+7
-7
lines changed

3 files changed

+7
-7
lines changed

src/os/bluestore/BitmapAllocator.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ class BitmapAllocator : public AllocatorBase,
2121
std::string_view name);
2222
~BitmapAllocator() override
2323
{
24+
shutdown();
2425
}
2526

2627
const char* get_type() const override

src/os/bluestore/HybridAllocator.h

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,14 @@
1111

1212
template <typename PrimaryAllocator>
1313
class HybridAllocatorBase : public PrimaryAllocator {
14-
BitmapAllocator* bmap_alloc = nullptr;
14+
std::unique_ptr<BitmapAllocator> bmap_alloc;
1515
public:
1616
HybridAllocatorBase(CephContext* cct, int64_t device_size, int64_t _block_size,
1717
uint64_t max_mem,
1818
std::string_view name) :
1919
PrimaryAllocator(cct, device_size, _block_size, max_mem, name) {
2020
}
21+
~HybridAllocatorBase() = default;
2122
int64_t allocate(
2223
uint64_t want,
2324
uint64_t unit,
@@ -59,19 +60,17 @@ class HybridAllocatorBase : public PrimaryAllocator {
5960
std::lock_guard l(PrimaryAllocator::get_lock());
6061
PrimaryAllocator::_shutdown();
6162
if (bmap_alloc) {
62-
bmap_alloc->shutdown();
63-
delete bmap_alloc;
64-
bmap_alloc = nullptr;
63+
bmap_alloc.reset();
6564
}
6665
}
6766

6867
protected:
6968
// intended primarily for UT
7069
BitmapAllocator* get_bmap() {
71-
return bmap_alloc;
70+
return bmap_alloc.get();
7271
}
7372
const BitmapAllocator* get_bmap() const {
74-
return bmap_alloc;
73+
return bmap_alloc.get();
7574
}
7675
private:
7776
void _spillover_range(uint64_t start, uint64_t end) override;

src/os/bluestore/HybridAllocator_impl.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ void HybridAllocatorBase<T>::_spillover_range(uint64_t start, uint64_t end)
120120
dout(1) << __func__
121121
<< " constructing fallback allocator"
122122
<< dendl;
123-
bmap_alloc = new BitmapAllocator(T::get_context(),
123+
bmap_alloc = std::make_unique<BitmapAllocator>(T::get_context(),
124124
T::get_capacity(),
125125
T::get_block_size(),
126126
T::get_name() + ".fallback");

0 commit comments

Comments
 (0)