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