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

Fix some leaks #121

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Fix some leaks #121

merged 3 commits into from
Feb 1, 2023

Conversation

ricrogz
Copy link
Collaborator

@ricrogz ricrogz commented Jan 27, 2023

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 ricrogz changed the title Fix a leak in CoordgenMacrocycleBuilder Fix some leaks Jan 27, 2023
@ricrogz
Copy link
Collaborator Author

ricrogz commented Jan 27, 2023

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

@ricrogz
Copy link
Collaborator Author

ricrogz commented Jan 27, 2023

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.

@@ -15,6 +15,9 @@
#include "maeparser/MaeConstants.hpp"
#include "maeparser/Reader.hpp"

// The sketcherMinimizerFragment class is not exposed in the library
#include "../sketcherMinimizerFragment.cpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this cause a double definition of the class?

I've seen weird issues in Maestro tests when they do this.

I guess I'd rather expose some sort of cleanuper.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'd have to expose the whole class to get access to the destructor.

@d-b-w d-b-w merged commit ae6bc26 into schrodinger:master Feb 1, 2023
@ricrogz ricrogz deleted the fix_leak branch February 1, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants