Skip to content

os/bluestore: fix memory leak in HybridAllocator destructor#64193

Merged
SrinivasaBharath merged 1 commit intoceph:mainfrom
tchaikov:wip-bluestore-test-shutdown
Jul 25, 2025
Merged

os/bluestore: fix memory leak in HybridAllocator destructor#64193
SrinivasaBharath merged 1 commit intoceph:mainfrom
tchaikov:wip-bluestore-test-shutdown

Conversation

@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Jun 26, 2025

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
    #1 0x5614e057ee11 in std::__new_allocator<char>::allocate(unsigned long, void const*) /usr/include/c++/15.1.1/bits/new_allocator.h:151
    #2 0x5614e057d279 in std::allocator<char>::allocate(unsigned long) /usr/include/c++/15.1.1/bits/allocator.h:203
    #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
    #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
    #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
    #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
    #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
    #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
    #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
    #10 0x5614e06b8350 in AllocatorLevel02<AllocatorLevel01Loose>::AllocatorLevel02() /home/kefu/dev/ceph/src/os/bluestore/fastbmap_allocator_impl.h:526
    #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
    #12 0x5614e072755f in HybridAllocatorBase<AvlAllocator>::_spillover_range(unsigned long, unsigned long) /home/kefu/dev/ceph/src/os/bluestore/HybridAllocator_impl.h:123
    #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
    #14 0x5614e06c0104 in AvlAllocator::_add_to_tree(unsigned long, unsigned long) /home/kefu/dev/ceph/src/os/bluestore/AvlAllocator.cc:136
    #15 0x5614e0586e5f in HybridAllocatorBase<AvlAllocator>::_add_to_tree(unsigned long, unsigned long) /home/kefu/dev/ceph/src/os/bluestore/HybridAllocator.h:110
    #16 0x5614e06c6559 in AvlAllocator::init_add_free(unsigned long, unsigned long) /home/kefu/dev/ceph/src/os/bluestore/AvlAllocator.cc:494
    #17 0x5614e0582943 in HybridAllocator_fragmentation_Test::TestBody() /home/kefu/dev/ceph/src/test/objectstore/hybrid_allocator_test.cc:285
    #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
    #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`

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands

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
    #1 0x5614e057ee11 in std::__new_allocator<char>::allocate(unsigned long, void const*) /usr/include/c++/15.1.1/bits/new_allocator.h:151
    #2 0x5614e057d279 in std::allocator<char>::allocate(unsigned long) /usr/include/c++/15.1.1/bits/allocator.h:203
    #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
    #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
    #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
    #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
    #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
    #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
    #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
    #10 0x5614e06b8350 in AllocatorLevel02<AllocatorLevel01Loose>::AllocatorLevel02() /home/kefu/dev/ceph/src/os/bluestore/fastbmap_allocator_impl.h:526
    #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
    #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>
@tchaikov tchaikov requested a review from ifed01 June 26, 2025 04:04
@tchaikov tchaikov requested a review from a team as a code owner June 26, 2025 04:04
@github-actions github-actions bot added the core label Jun 26, 2025
@tchaikov tchaikov mentioned this pull request Jun 26, 2025
14 tasks
@tchaikov
Copy link
Contributor Author

@ifed01 hi Igor, could you please help review this change?

@ifed01 ifed01 added the needs-qa label Jul 7, 2025
@tchaikov
Copy link
Contributor Author

@yuriw hi Yuri, could you please include this change in your next batch?

@connorfawcett
Copy link
Contributor

@SrinivasaBharath SrinivasaBharath merged commit 9301078 into ceph:main Jul 25, 2025
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants