Skip to content

test/test_not_before_queue: Fix memory leak by freeing ineligible items in destructor#63292

Merged
tchaikov merged 1 commit intoceph:mainfrom
tchaikov:test_not_before_queue-fix-leaks
May 22, 2025
Merged

test/test_not_before_queue: Fix memory leak by freeing ineligible items in destructor#63292
tchaikov merged 1 commit intoceph:mainfrom
tchaikov:test_not_before_queue-fix-leaks

Conversation

@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented May 15, 2025

Fix memory leak in test_not_before_queue identified by AddressSanitizer. Previously, the test was terminating without properly dequeueing all elements, causing resource leaks during teardown.

This change implements a proper cleanup mechanism that:

  1. Advances the queue's time until all elements become eligible for processing
  2. Dequeues all remaining elements to ensure proper destruction
  3. Guarantees clean teardown even for elements with future timestamps

The fix eliminates ASan-reported leaks occurring in the not_before_queue_t::enqueue method when test cases were torn down prematurely.

A sample of the error from ASan:

Direct leak of 1800 byte(s) in 15 object(s) allocated from:
    #0 0x7f71b1f1ab5b in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:86
    #1 0x55fb4c977058 in void not_before_queue_t<tv_t, test_time_t>::enqueue<tv_t const&>(tv_t const&) /home/kefu/dev/ceph/src/common/not_before_queue.h:164
    #2 0x55fb4c97748b in NotBeforeTest::load_test_data(std::vector<tv_t, std::allocator<tv_t> > const&) /home/kefu/dev/ceph/src/test/test_not_before_queue.cc:67
    #3 0x55fb4c961112 in NotBeforeTest_RemoveIfByClass_no_cond_Test::TestBody() /home/kefu/dev/ceph/src/test/test_not_before_queue.cc:213
    #4 0x55fb4ca05f67 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
    #5 0x55fb4ca1c4f7 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
    #6 0x55fb4c9e1104 in testing::Test::Run() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2728
    #7 0x55fb4c9e16e2 in testing::TestInfo::Run() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2874
    #8 0x55fb4c9e73b4 in testing::TestSuite::Run() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:3052
    #9 0x55fb4c9f059b in testing::internal::UnitTestImpl::RunAllTests() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:6004
    #10 0x55fb4ca064ff in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2653
    #11 0x55fb4ca1d1bf in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2689
    #12 0x55fb4c9e124d in testing::UnitTest::Run() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:5583
    #13 0x55fb4c97a0b6 in RUN_ALL_TESTS() /home/kefu/dev/ceph/src/googletest/googletest/include/gtest/gtest.h:2334
    #14 0x55fb4c979ffc in main /home/kefu/dev/ceph/src/googletest/googlemock/src/gmock_main.cc:71
    #15 0x7f71ae833ca7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: 1800 byte(s) leaked in 15 allocation(s).

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

…ms in destructor

Fix memory leak in test_not_before_queue identified by AddressSanitizer.
Previously, the test was terminating without properly dequeueing all elements,
causing resource leaks during teardown.

This change implements a proper cleanup mechanism that:
1. Advances the queue's time until all elements become eligible for processing
2. Dequeues all remaining elements to ensure proper destruction
3. Guarantees clean teardown even for elements with future timestamps

The fix eliminates ASan-reported leaks occurring in the not_before_queue_t::enqueue
method when test cases were torn down prematurely.

A sample of the error from ASan:

```
Direct leak of 1800 byte(s) in 15 object(s) allocated from:
    #0 0x7f71b1f1ab5b in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:86
    #1 0x55fb4c977058 in void not_before_queue_t<tv_t, test_time_t>::enqueue<tv_t const&>(tv_t const&) /home/kefu/dev/ceph/src/common/not_before_queue.h:164
    #2 0x55fb4c97748b in NotBeforeTest::load_test_data(std::vector<tv_t, std::allocator<tv_t> > const&) /home/kefu/dev/ceph/src/test/test_not_before_queue.cc:67
    #3 0x55fb4c961112 in NotBeforeTest_RemoveIfByClass_no_cond_Test::TestBody() /home/kefu/dev/ceph/src/test/test_not_before_queue.cc:213
    #4 0x55fb4ca05f67 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
    #5 0x55fb4ca1c4f7 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
    #6 0x55fb4c9e1104 in testing::Test::Run() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2728
    #7 0x55fb4c9e16e2 in testing::TestInfo::Run() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2874
    #8 0x55fb4c9e73b4 in testing::TestSuite::Run() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:3052
    #9 0x55fb4c9f059b in testing::internal::UnitTestImpl::RunAllTests() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:6004
    #10 0x55fb4ca064ff in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2653
    #11 0x55fb4ca1d1bf in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:2689
    #12 0x55fb4c9e124d in testing::UnitTest::Run() /home/kefu/dev/ceph/src/googletest/googletest/src/gtest.cc:5583
    ceph#13 0x55fb4c97a0b6 in RUN_ALL_TESTS() /home/kefu/dev/ceph/src/googletest/googletest/include/gtest/gtest.h:2334
    ceph#14 0x55fb4c979ffc in main /home/kefu/dev/ceph/src/googletest/googlemock/src/gmock_main.cc:71
    ceph#15 0x7f71ae833ca7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: 1800 byte(s) leaked in 15 allocation(s).
```

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@github-actions github-actions bot added the tests label May 15, 2025
@tchaikov tchaikov requested a review from athanatos May 15, 2025 09:00
@tchaikov tchaikov mentioned this pull request May 15, 2025
14 tasks
@tchaikov
Copy link
Contributor Author

jenkins test make check

@tchaikov
Copy link
Contributor Author

jenkins test make check arm64

@tchaikov
Copy link
Contributor Author

@athanatos hi Sam, could you please help review this change?

@tchaikov tchaikov requested a review from a team May 21, 2025 02:21
@tchaikov
Copy link
Contributor Author

@ceph/core hello, could you help review this change?

@ljflores ljflores added the core label May 21, 2025
@tchaikov
Copy link
Contributor Author

@athanatos hi Sam, could you please help review this change?

@tchaikov
Copy link
Contributor Author

Ronen, thanks for your review and approval. i am merging this change without running it against teuthology as it only impacts the unit test

@tchaikov tchaikov merged commit 3a75e1a into ceph:main May 22, 2025
18 of 22 checks passed
@tchaikov tchaikov deleted the test_not_before_queue-fix-leaks branch May 22, 2025 08:50
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.

3 participants