schrodinger / coordgenlibs

Schrodinger-developed 2D Coordinate Generation
BSD 3-Clause "New" or "Revised" License
42 stars 28 forks source link

Fix some leaks #121

Closed ricrogz closed 1 year ago

ricrogz commented 1 year ago

I was running asan on RDKit's test when I noticed this leak:

6: Direct leak of 144 byte(s) in 1 object(s) allocated from:
6:     #0 0x7fe6e58c0488 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:95
6:     #1 0x7fe6e5089029 in CoordgenMacrocycleBuilder::openCycleAndGenerateCoords(sketcherMinimizerRing*) const coordgenlibs/CoordgenMacrocycleBuilder.cpp:768
6:     #2 0x7fe6e5045820 in CoordgenFragmentBuilder::generateCoordinatesCentralRings(std::vector<sketcherMinimizerRing*, std::allocator<sketcherMinimizerRing*> >) const coordgenlibs/CoordgenFragmentBuilder.cpp:231
6:     #3 0x7fe6e50522e9 in CoordgenFragmentBuilder::buildRings(sketcherMinimizerFragment*) const coordgenlibs/CoordgenFragmentBuilder.cpp:1087
6:     #4 0x7fe6e5053a45 in CoordgenFragmentBuilder::buildFragment(sketcherMinimizerFragment*) const coordgenlibs/CoordgenFragmentBuilder.cpp:854
6:     #5 0x7fe6e5053b24 in CoordgenFragmentBuilder::initializeCoordinates(sketcherMinimizerFragment*) const coordgenlibs/CoordgenFragmentBuilder.cpp:34
6:     #6 0x7fe6e5194b2a in sketcherMinimizer::initializeFragments() coordgenlibs/sketcherMinimizer.cpp:2727
6:     #7 0x7fe6e51968bd in sketcherMinimizer::findFragments() coordgenlibs/sketcherMinimizer.cpp:1069
6:     #8 0x7fe6e51b3b5f in sketcherMinimizer::runGenerateCoordinates() coordgenlibs/sketcherMinimizer.cpp:299
6:     #9 0x55591aac7456 in unsigned int RDKit::CoordGen::addCoords<RDKit::ROMol>(RDKit::ROMol&, RDKit::CoordGen::CoordGenParams const*) rdkit/External/CoordGen/CoordGen.h:173
6:     #10 0x55591aa9f82a in test1() rdkit/External/CoordGen/test.cpp:119
6:     #11 0x55591aab8fc8 in main rdkit/External/CoordGen/test.cpp:574
6:     #12 0x7fe6e2829d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

This is coming from auto* minMol = new sketcherMinimizerMolecule;, which was never being deleted. This fixes the issue by allocating the sketcherMinimizerMolecule on the stack, so that it gets destroyed when it goes out of scope at the end of the function.

I'm not adding a test because this is checked in RDKit, and I don't know how to trigger this issue from bare coordgen.

ricrogz commented 1 year ago

Turns out I completely misunderstood what's going on here.

ricrogz commented 1 year ago

Ok, should be fixed now.

The issue was happening only when we exited the method early. Fixed by moving the allocation of the pointer to happen after the early exit. The pointer doesn't need to be deleted, since the sketcherMinimizer will take care of it.