Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid leaking memory in case exceptions are thrown while generating FPs #6630

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

ptosco
Copy link
Contributor

@ptosco ptosco commented Aug 14, 2023

While running CFFI tests under valgrind I noticed that memory is leaked when an exception is thrown during generation of certain FPs; see the valgrind trace below:

$ valgrind --leak-check=full ./Code/MinimalLib/cffi_test
[...]
==255551==
==255551== HEAP SUMMARY:
==255551==     in use at exit: 192 bytes in 7 blocks
==255551==   total heap usage: 524,741 allocs, 524,734 frees, 39,361,697 bytes allocated
==255551==
==255551== 16 bytes in 1 blocks are definitely lost in loss record 1 of 7
==255551==    at 0x4C2C866: operator new[](unsigned long) (vg_replace_malloc.c:579)
==255551==    by 0x5162D5A: bool boost::vf2_all<boost::adjacency_list<boost::vecS, boost::vecS, boost::undirectedS, RDKit::Atom*, RDKit::Bond*, boost::no_property, boost::listS>, RDKit::detail::AtomLabelFunctor, RDKit::detail::BondL
abelFunctor, RDKit::MolMatchFinalCheckFunctor, std::__cxx11::list<std::__cxx11::list<std::pair<unsigned long, unsigned long>, std::allocator<std::pair<unsigned long, unsigned long> > >, std::allocator<std::__cxx11::list<std::pair<un
signed long, unsigned long>, std::allocator<std::pair<unsigned long, unsigned long> > > > > >(boost::adjacency_list<boost::vecS, boost::vecS, boost::undirectedS, RDKit::Atom*, RDKit::Bond*, boost::no_property, boost::listS> const&,
boost::adjacency_list<boost::vecS, boost::vecS, boost::undirectedS, RDKit::Atom*, RDKit::Bond*, boost::no_property, boost::listS> const&, RDKit::detail::AtomLabelFunctor&, RDKit::detail::BondLabelFunctor&, RDKit::MolMatchFinalCheckF
unctor&, std::__cxx11::list<std::__cxx11::list<std::pair<unsigned long, unsigned long>, std::allocator<std::pair<unsigned long, unsigned long> > >, std::allocator<std::__cxx11::list<std::pair<unsigned long, unsigned long>, std::allo
cator<std::pair<unsigned long, unsigned long> > > > >&, unsigned int) (vf2.hpp:666)
==255551==    by 0x516413A: RDKit::SubstructMatch(RDKit::ROMol const&, RDKit::ROMol const&, RDKit::SubstructMatchParameters const&) (SubstructMatch.cpp:512)
==255551==    by 0x5011D5E: bool RDKit::SubstructMatch<RDKit::ROMol const, RDKit::ROMol>(RDKit::ROMol const&, RDKit::ROMol const&, std::vector<std::pair<int, int>, std::allocator<std::pair<int, int> > >&, bool, bool, bool) (Substruc
tMatch.h:136)
==255551==    by 0x5381346: (anonymous namespace)::GenerateFP(RDKit::ROMol const&, ExplicitBitVect&) [clone .constprop.0] (MACCS.cpp:600)
==255551==    by 0x5382EBD: RDKit::MACCSFingerprints::getFingerprintAsBitVect(RDKit::ROMol const&) (MACCS.cpp:944)
==255551==    by 0x4FDB3DC: RDKit::MinimalLib::maccs_fp_as_bitvect(RDKit::RWMol const&) (common.h:829)
==255551==    by 0x4FE9A12: get_maccs_fp (cffiwrapper.cpp:636)
==255551==    by 0x111ABA: test_partial_sanitization (cffi_test.c:2157)
==255551==    by 0x10A11E: main (cffi_test.c:2192)
==255551==
==255551== 36 bytes in 1 blocks are definitely lost in loss record 5 of 7
==255551==    at 0x4C2C866: operator new[](unsigned long) (vg_replace_malloc.c:579)
==255551==    by 0x5162D8A: bool boost::vf2_all<boost::adjacency_list<boost::vecS, boost::vecS, boost::undirectedS, RDKit::Atom*, RDKit::Bond*, boost::no_property, boost::listS>, RDKit::detail::AtomLabelFunctor, RDKit::detail::BondL
abelFunctor, RDKit::MolMatchFinalCheckFunctor, std::__cxx11::list<std::__cxx11::list<std::pair<unsigned long, unsigned long>, std::allocator<std::pair<unsigned long, unsigned long> > >, std::allocator<std::__cxx11::list<std::pair<un
signed long, unsigned long>, std::allocator<std::pair<unsigned long, unsigned long> > > > > >(boost::adjacency_list<boost::vecS, boost::vecS, boost::undirectedS, RDKit::Atom*, RDKit::Bond*, boost::no_property, boost::listS> const&,
boost::adjacency_list<boost::vecS, boost::vecS, boost::undirectedS, RDKit::Atom*, RDKit::Bond*, boost::no_property, boost::listS> const&, RDKit::detail::AtomLabelFunctor&, RDKit::detail::BondLabelFunctor&, RDKit::MolMatchFinalCheckF
unctor&, std::__cxx11::list<std::__cxx11::list<std::pair<unsigned long, unsigned long>, std::allocator<std::pair<unsigned long, unsigned long> > >, std::allocator<std::__cxx11::list<std::pair<unsigned long, unsigned long>, std::allo
cator<std::pair<unsigned long, unsigned long> > > > >&, unsigned int) (vf2.hpp:667)
==255551==    by 0x516413A: RDKit::SubstructMatch(RDKit::ROMol const&, RDKit::ROMol const&, RDKit::SubstructMatchParameters const&) (SubstructMatch.cpp:512)
==255551==    by 0x5011D5E: bool RDKit::SubstructMatch<RDKit::ROMol const, RDKit::ROMol>(RDKit::ROMol const&, RDKit::ROMol const&, std::vector<std::pair<int, int>, std::allocator<std::pair<int, int> > >&, bool, bool, bool) (Substruc
tMatch.h:136)
==255551==    by 0x5381346: (anonymous namespace)::GenerateFP(RDKit::ROMol const&, ExplicitBitVect&) [clone .constprop.0] (MACCS.cpp:600)
==255551==    by 0x5382EBD: RDKit::MACCSFingerprints::getFingerprintAsBitVect(RDKit::ROMol const&) (MACCS.cpp:944)
==255551==    by 0x4FDB3DC: RDKit::MinimalLib::maccs_fp_as_bitvect(RDKit::RWMol const&) (common.h:829)
==255551==    by 0x4FE9A12: get_maccs_fp (cffiwrapper.cpp:636)
==255551==    by 0x111ABA: test_partial_sanitization (cffi_test.c:2157)
==255551==    by 0x10A11E: main (cffi_test.c:2192)
==255551==
==255551== 60 (24 direct, 36 indirect) bytes in 1 blocks are definitely lost in loss record 6 of 7
==255551==    at 0x4C2B788: operator new(unsigned long) (vg_replace_malloc.c:417)
==255551==    by 0x5391C66: RDKit::MorganFingerprint::MorganAtomInvGenerator::getAtomInvariants(RDKit::ROMol const&) const (MorganGenerator.cpp:37)
==255551==    by 0x538CA61: RDKit::FingerprintGenerator<unsigned int>::getFingerprintHelper(RDKit::ROMol const&, RDKit::FingerprintFuncArguments&, unsigned long) const (FingerprintGenerator.cpp:161)
==255551==    by 0x538D212: RDKit::FingerprintGenerator<unsigned int>::getFingerprint(RDKit::ROMol const&, RDKit::FingerprintFuncArguments&) const (FingerprintGenerator.cpp:409)
==255551==    by 0x5376671: RDKit::MorganFingerprints::getFingerprintAsBitVect(RDKit::ROMol const&, unsigned int, unsigned int, std::vector<unsigned int, std::allocator<unsigned int> >*, std::vector<unsigned int, std::allocator<unsi
gned int> > const*, bool, bool, bool, std::map<unsigned int, std::vector<std::pair<unsigned int, unsigned int>, std::allocator<std::pair<unsigned int, unsigned int> > >, std::less<unsigned int>, std::allocator<std::pair<unsigned int
 const, std::vector<std::pair<unsigned int, unsigned int>, std::allocator<std::pair<unsigned int, unsigned int> > > > > >*, bool) (MorganFingerprints.cpp:145)
==255551==    by 0x4FF24B1: RDKit::MinimalLib::morgan_fp_as_bitvect(RDKit::RWMol const&, char const*) (common.h:740)
==255551==    by 0x4FF27FA: get_morgan_fp (cffiwrapper.cpp:481)
==255551==    by 0x111A37: test_partial_sanitization (cffi_test.c:2146)
==255551==    by 0x10A11E: main (cffi_test.c:2192)
==255551==
==255551== 80 (24 direct, 56 indirect) bytes in 1 blocks are definitely lost in loss record 7 of 7
==255551==    at 0x4C2B788: operator new(unsigned long) (vg_replace_malloc.c:417)
==255551==    by 0x5382E83: RDKit::MACCSFingerprints::getFingerprintAsBitVect(RDKit::ROMol const&) (MACCS.cpp:943)
==255551==    by 0x4FDB3DC: RDKit::MinimalLib::maccs_fp_as_bitvect(RDKit::RWMol const&) (common.h:829)
==255551==    by 0x4FE9A12: get_maccs_fp (cffiwrapper.cpp:636)
==255551==    by 0x111ABA: test_partial_sanitization (cffi_test.c:2157)
==255551==    by 0x10A11E: main (cffi_test.c:2192)
==255551==
==255551== LEAK SUMMARY:
==255551==    definitely lost: 100 bytes in 4 blocks
==255551==    indirectly lost: 92 bytes in 3 blocks
==255551==      possibly lost: 0 bytes in 0 blocks
==255551==    still reachable: 0 bytes in 0 blocks
==255551==         suppressed: 0 bytes in 0 blocks
==255551==
==255551== For lists of detected and suppressed errors, rerun with: -s
==255551== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)

This PR avoid all those leaks by wrapping pointer into unique_ptr objects which are only released before returning.

@greglandrum greglandrum added bug Cleanup Code cleanup and refactoring labels Aug 15, 2023
@greglandrum greglandrum added this to the 2023_03_3 milestone Aug 15, 2023
Copy link
Member

@greglandrum greglandrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks!

@greglandrum greglandrum merged commit a384878 into rdkit:master Aug 15, 2023
10 checks passed
greglandrum pushed a commit that referenced this pull request Aug 17, 2023
…Ps (#6630)

Co-authored-by: ptosco <paolo.tosco@novartis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Cleanup Code cleanup and refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants