Proposal: Fix Memory Leaks in generate_test_instance() by Using shared_ptr
Hi folks, I've been working on resolving memory leaks detected by ASan in our unit tests and identified a significant issue with our current approach to test instance generation. Current Problem: Our generate_test_instance() function returns std::list<T*> for types that support encoding/decoding. While this works functionally, it creates memory management issues because: - The lifecycle of generated test instances is inconsistently managed - Callers don't always properly clean up allocated memory - This results in numerous ASan memory leak reports in both unit tests and ceph-dencoder Proposed Solution: I propose we modify the function to return std::list<std::shared_ptr<T>> instead of raw pointers. This change would: - Automatically handle memory cleanup through RAII - Work correctly with types that lack move constructors - Eliminate the memory leaks we're currently seeing when testing with ASan enabled - Require updates to calling code, but provide cleaner memory semantics I considered adding suppression rules for the affected tests and ceph-dencoder, but that would only mask the underlying issue rather than properly addressing the memory management problem. Next Steps: If this approach sounds reasonable, I can prepare a patch that implements this change along with the necessary caller updates. What are your thoughts on this approach? Are there any concerns or alternative solutions you'd like to consider? -- Regards Kefu Chai
On Mon, Jun 9, 2025 at 10:12 AM kefu chai <tchaikov@gmail.com> wrote:
Hi folks,
I've been working on resolving memory leaks detected by ASan in our unit tests and identified a significant issue with our current approach to test instance generation.
Current Problem: Our generate_test_instance() function returns std::list<T*> for types that support encoding/decoding. While this works functionally, it creates memory management issues because:
- The lifecycle of generated test instances is inconsistently managed - Callers don't always properly clean up allocated memory - This results in numerous ASan memory leak reports in both unit tests and ceph-dencoder
Proposed Solution: I propose we modify the function to return std::list<std::shared_ptr<T>> instead of raw pointers. This change would:
- Automatically handle memory cleanup through RAII - Work correctly with types that lack move constructors - Eliminate the memory leaks we're currently seeing when testing with ASan enabled - Require updates to calling code, but provide cleaner memory semantics
could we not use `std::list<T>` and rely on emplace for types that can't be moved/copied?
I considered adding suppression rules for the affected tests and ceph-dencoder, but that would only mask the underlying issue rather than properly addressing the memory management problem.
Next Steps: If this approach sounds reasonable, I can prepare a patch that implements this change along with the necessary caller updates.
What are your thoughts on this approach? Are there any concerns or alternative solutions you'd like to consider?
-- Regards Kefu Chai _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io
On Tue, Jun 10, 2025 at 9:31 PM Casey Bodley <cbodley@redhat.com> wrote:
On Mon, Jun 9, 2025 at 10:12 AM kefu chai <tchaikov@gmail.com> wrote:
Hi folks,
I've been working on resolving memory leaks detected by ASan in our unit
tests and identified a significant issue with our current approach to test instance generation.
Current Problem: Our generate_test_instance() function returns
std::list<T*> for types that support encoding/decoding. While this works functionally, it creates memory management issues because:
- The lifecycle of generated test instances is inconsistently managed - Callers don't always properly clean up allocated memory - This results in numerous ASan memory leak reports in both unit tests
and ceph-dencoder
Proposed Solution: I propose we modify the function to return
std::list<std::shared_ptr<T>> instead of raw pointers. This change would:
- Automatically handle memory cleanup through RAII - Work correctly with types that lack move constructors - Eliminate the memory leaks we're currently seeing when testing with
ASan enabled
- Require updates to calling code, but provide cleaner memory semantics
could we not use `std::list<T>` and rely on emplace for types that can't be moved/copied?
Thank you for your insights. I was worried we might use it to append to an existing list. However, after reviewing the code, I confirmed that we never append to existing lists and only use it as an output parameter. Given this, we can (and probably should) proceed with the refactoring. I'm preparing a comprehensive patch that migrates from T::generate_test_instances(std::list<T*>&) to std::list<T> T::generate_test_instances(). For the migration strategy, while we could use SFINAE at caller sites to detect the new function signature and fall back to the old one, I think that approach might be overkill. Instead, I'm planning to implement this as a single comprehensive PR. Even though the change is substantial, it's essentially a semi-mechanical edit that should be straightforward to implement all at once. What are your thoughts on this approach?
I considered adding suppression rules for the affected tests and
ceph-dencoder, but that would only mask the underlying issue rather than properly addressing the memory management problem.
Next Steps: If this approach sounds reasonable, I can prepare a patch that
implements this change along with the necessary caller updates.
What are your thoughts on this approach? Are there any concerns or
alternative solutions you'd like to consider?
-- Regards Kefu Chai _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io
-- Regards Kefu Chai
On Wed, Jun 11, 2025 at 10:51 AM kefu chai <tchaikov@gmail.com> wrote:
On Tue, Jun 10, 2025 at 9:31 PM Casey Bodley <cbodley@redhat.com> wrote:
On Mon, Jun 9, 2025 at 10:12 AM kefu chai <tchaikov@gmail.com> wrote:
Hi folks,
I've been working on resolving memory leaks detected by ASan in our unit tests and identified a significant issue with our current approach to test instance generation.
Current Problem: Our generate_test_instance() function returns std::list<T*> for types that support encoding/decoding. While this works functionally, it creates memory management issues because:
- The lifecycle of generated test instances is inconsistently managed - Callers don't always properly clean up allocated memory - This results in numerous ASan memory leak reports in both unit tests and ceph-dencoder
Proposed Solution: I propose we modify the function to return std::list<std::shared_ptr<T>> instead of raw pointers. This change would:
- Automatically handle memory cleanup through RAII - Work correctly with types that lack move constructors - Eliminate the memory leaks we're currently seeing when testing with ASan enabled - Require updates to calling code, but provide cleaner memory semantics
could we not use `std::list<T>` and rely on emplace for types that can't be moved/copied?
Thank you for your insights. I was worried we might use it to append to an existing list. However, after reviewing the code, I confirmed that we never append to existing lists and only use it as an output parameter.
Given this, we can (and probably should) proceed with the refactoring. I'm preparing a comprehensive patch that migrates from T::generate_test_instances(std::list<T*>&) to std::list<T> T::generate_test_instances().
are you sure it matters whether we're appending? i'd expect `generate_test_instances(std::list<T>&)` to work too. if we return the list instead, each function body needs two extra lines (one to declare a `std::list<T>` on the stack, and another to return it)
For the migration strategy, while we could use SFINAE at caller sites to detect the new function signature and fall back to the old one, I think that approach might be overkill. Instead, I'm planning to implement this as a single comprehensive PR. Even though the change is substantial, it's essentially a semi-mechanical edit that should be straightforward to implement all at once.
What are your thoughts on this approach?
+1 this seems worth doing, and i'll gladly help with review
I considered adding suppression rules for the affected tests and ceph-dencoder, but that would only mask the underlying issue rather than properly addressing the memory management problem.
Next Steps: If this approach sounds reasonable, I can prepare a patch that implements this change along with the necessary caller updates.
What are your thoughts on this approach? Are there any concerns or alternative solutions you'd like to consider?
-- Regards Kefu Chai _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io
-- Regards Kefu Chai
On Thu, Jun 12, 2025 at 1:41 AM Casey Bodley <cbodley@redhat.com> wrote:
On Wed, Jun 11, 2025 at 10:51 AM kefu chai <tchaikov@gmail.com> wrote:
On Tue, Jun 10, 2025 at 9:31 PM Casey Bodley <cbodley@redhat.com> wrote:
On Mon, Jun 9, 2025 at 10:12 AM kefu chai <tchaikov@gmail.com> wrote:
Hi folks,
I've been working on resolving memory leaks detected by ASan in our
unit tests and identified a significant issue with our current approach to test instance generation.
Current Problem: Our generate_test_instance() function returns
std::list<T*> for types that support encoding/decoding. While this works functionally, it creates memory management issues because:
- The lifecycle of generated test instances is inconsistently managed - Callers don't always properly clean up allocated memory - This results in numerous ASan memory leak reports in both unit
tests and ceph-dencoder
Proposed Solution: I propose we modify the function to return
std::list<std::shared_ptr<T>> instead of raw pointers. This change would:
- Automatically handle memory cleanup through RAII - Work correctly with types that lack move constructors - Eliminate the memory leaks we're currently seeing when testing with
ASan enabled
- Require updates to calling code, but provide cleaner memory semantics
could we not use `std::list<T>` and rely on emplace for types that can't be moved/copied?
Thank you for your insights. I was worried we might use it to append to an existing list. However, after reviewing the code, I confirmed that we never append to existing lists and only use it as an output parameter.
Given this, we can (and probably should) proceed with the refactoring. I'm preparing a comprehensive patch that migrates from T::generate_test_instances(std::list<T*>&) to std::list<T> T::generate_test_instances().
are you sure it matters whether we're appending?
i don't think it matters, at least for now. i'd expect
`generate_test_instances(std::list<T>&)` to work too. if we return the list instead, each function body needs two extra lines (one to declare a `std::list<T>` on the stack, and another to return it)
yeah, agreed. but i have changed more than 300 instances of the functions to return std::list<T> so far, there are 600 more to go. i plan to finish all of them in this week. if you think it's important to save these two lines in the function body. i will rewrork them in the following days.
For the migration strategy, while we could use SFINAE at caller sites to
detect the new function signature and fall back to the old one, I think that approach might be overkill. Instead, I'm planning to implement this as a single comprehensive PR. Even though the change is substantial, it's essentially a semi-mechanical edit that should be straightforward to implement all at once.
What are your thoughts on this approach?
+1 this seems worth doing, and i'll gladly help with review
thank you, Casey!
I considered adding suppression rules for the affected tests and
ceph-dencoder, but that would only mask the underlying issue rather than properly addressing the memory management problem.
Next Steps: If this approach sounds reasonable, I can prepare a patch that
implements this change along with the necessary caller updates.
What are your thoughts on this approach? Are there any concerns or
alternative solutions you'd like to consider?
-- Regards Kefu Chai _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io
-- Regards Kefu Chai
-- Regards Kefu Chai
participants (2)
-
Casey Bodley
-
kefu chai