From ccfb1fa688b5591c1b302ae4b4a9f92c1a1ca9b3 Mon Sep 17 00:00:00 2001 From: Ricardo Rodriguez Date: Fri, 25 Oct 2024 01:01:34 -0400 Subject: [PATCH] ... and more mem errors fixed (#7924) * fixleak in CIP labels catch test * fix leak in Murtagh clustering * do not leak writers in streambuf * fix leaks in fingerprintgeneratorwrapper * remove 'minor leak' comments --- Code/GraphMol/CIPLabeler/catch_tests.cpp | 16 ++-- .../Wrap/FingerprintGeneratorWrapper.cpp | 90 ++++++++----------- Code/GraphMol/Wrap/MolOps.cpp | 29 +++--- Code/GraphMol/Wrap/PDBWriter.cpp | 3 +- Code/GraphMol/Wrap/SDWriter.cpp | 3 +- Code/GraphMol/Wrap/SmilesWriter.cpp | 3 +- Code/GraphMol/Wrap/TDTWriter.cpp | 3 +- Code/ML/Cluster/Murtagh/Clustering.cpp | 28 +++++- Code/RDBoost/python_streambuf.h | 9 ++ 9 files changed, 95 insertions(+), 89 deletions(-) diff --git a/Code/GraphMol/CIPLabeler/catch_tests.cpp b/Code/GraphMol/CIPLabeler/catch_tests.cpp index a694de101..20743b3c1 100644 --- a/Code/GraphMol/CIPLabeler/catch_tests.cpp +++ b/Code/GraphMol/CIPLabeler/catch_tests.cpp @@ -944,22 +944,20 @@ TEST_CASE("atropisomers", "[basic]") { } } -std::string cipLabels(std::string molBlock) { - RDKit::ROMol *mol = nullptr; +std::string cipLabels(const std::string &molBlock) { + std::unique_ptr mol; std::string result = ""; try { // try parsing the mol block with sanitize ob try { - mol = MolBlockToMol(molBlock, true, false); + mol.reset(MolBlockToMol(molBlock, true, false)); } catch (...) { - delete mol; - mol = nullptr; } // if parsing with Sanitize on did NOT work, try it without - if (mol == NULL) { - mol = MolBlockToMol(molBlock, false, false); + if (mol == nullptr) { + mol.reset(MolBlockToMol(molBlock, false, false)); mol->updatePropertyCache(false); } @@ -991,12 +989,8 @@ std::string cipLabels(std::string molBlock) { boost::algorithm::join(bondsArray, ", "); return result; } catch (const CIPLabeler::MaxIterationsExceeded &e) { - delete mol; - mol = nullptr; return ""; } catch (const std::exception &e) { - delete mol; - mol = nullptr; return e.what(); } } diff --git a/Code/GraphMol/Fingerprints/Wrap/FingerprintGeneratorWrapper.cpp b/Code/GraphMol/Fingerprints/Wrap/FingerprintGeneratorWrapper.cpp index b7d75ef36..2f83f0887 100644 --- a/Code/GraphMol/Fingerprints/Wrap/FingerprintGeneratorWrapper.cpp +++ b/Code/GraphMol/Fingerprints/Wrap/FingerprintGeneratorWrapper.cpp @@ -36,18 +36,18 @@ namespace np = boost::python::numpy; namespace RDKit { namespace FingerprintWrapper { -void convertPyArguments(python::object py_fromAtoms, - python::object py_ignoreAtoms, - python::object py_atomInvs, python::object py_bondInvs, - std::vector *&fromAtoms, - std::vector *&ignoreAtoms, - std::vector *&customAtomInvariants, - std::vector *&customBondInvariants) { +void convertPyArguments( + python::object py_fromAtoms, python::object py_ignoreAtoms, + python::object py_atomInvs, python::object py_bondInvs, + std::unique_ptr> &fromAtoms, + std::unique_ptr> &ignoreAtoms, + std::unique_ptr> &customAtomInvariants, + std::unique_ptr> &customBondInvariants) { if (!py_fromAtoms.is_none()) { unsigned int len = python::extract(py_fromAtoms.attr("__len__")()); if (len) { - fromAtoms = new std::vector(); + fromAtoms.reset(new std::vector()); fromAtoms->reserve(len); for (unsigned int i = 0; i < len; ++i) { fromAtoms->push_back(python::extract(py_fromAtoms[i])); @@ -59,7 +59,7 @@ void convertPyArguments(python::object py_fromAtoms, unsigned int len = python::extract(py_ignoreAtoms.attr("__len__")()); if (len) { - ignoreAtoms = new std::vector(); + ignoreAtoms.reset(new std::vector()); ignoreAtoms->reserve(len); for (unsigned int i = 0; i < len; ++i) { ignoreAtoms->push_back( @@ -72,7 +72,7 @@ void convertPyArguments(python::object py_fromAtoms, unsigned int len = python::extract(py_atomInvs.attr("__len__")()); if (len) { - customAtomInvariants = new std::vector(); + customAtomInvariants.reset(new std::vector()); customAtomInvariants->reserve(len); for (unsigned int i = 0; i < len; ++i) { customAtomInvariants->push_back( @@ -85,7 +85,7 @@ void convertPyArguments(python::object py_fromAtoms, unsigned int len = python::extract(py_bondInvs.attr("__len__")()); if (len) { - customBondInvariants = new std::vector(); + customBondInvariants.reset(new std::vector()); customBondInvariants->reserve(len); for (unsigned int i = 0; i < len; ++i) { customBondInvariants->push_back( @@ -103,10 +103,10 @@ SparseIntVect *getSparseCountFingerprint( python::object py_fromAtoms, python::object py_ignoreAtoms, const int confId, python::object py_atomInvs, python::object py_bondInvs, python::object py_additionalOutput) { - std::vector *fromAtoms = nullptr; - std::vector *ignoreAtoms = nullptr; - std::vector *customAtomInvariants = nullptr; - std::vector *customBondInvariants = nullptr; + std::unique_ptr> fromAtoms; + std::unique_ptr> ignoreAtoms; + std::unique_ptr> customAtomInvariants; + std::unique_ptr> customBondInvariants; convertPyArguments(py_fromAtoms, py_ignoreAtoms, py_atomInvs, py_bondInvs, fromAtoms, ignoreAtoms, customAtomInvariants, @@ -116,14 +116,11 @@ SparseIntVect *getSparseCountFingerprint( additionalOutput = python::extract(py_additionalOutput); } - FingerprintFuncArguments args(fromAtoms, ignoreAtoms, confId, - additionalOutput, customAtomInvariants, - customBondInvariants); + FingerprintFuncArguments args(fromAtoms.get(), ignoreAtoms.get(), confId, + additionalOutput, customAtomInvariants.get(), + customBondInvariants.get()); auto result = fpGen->getSparseCountFingerprint(mol, args); - delete fromAtoms; - delete ignoreAtoms; - return result.release(); } @@ -133,10 +130,10 @@ SparseBitVect *getSparseFingerprint( python::object py_fromAtoms, python::object py_ignoreAtoms, const int confId, python::object py_atomInvs, python::object py_bondInvs, python::object py_additionalOutput) { - std::vector *fromAtoms = nullptr; - std::vector *ignoreAtoms = nullptr; - std::vector *customAtomInvariants = nullptr; - std::vector *customBondInvariants = nullptr; + std::unique_ptr> fromAtoms; + std::unique_ptr> ignoreAtoms; + std::unique_ptr> customAtomInvariants; + std::unique_ptr> customBondInvariants; convertPyArguments(py_fromAtoms, py_ignoreAtoms, py_atomInvs, py_bondInvs, fromAtoms, ignoreAtoms, customAtomInvariants, customBondInvariants); @@ -145,14 +142,11 @@ SparseBitVect *getSparseFingerprint( additionalOutput = python::extract(py_additionalOutput); } - FingerprintFuncArguments args(fromAtoms, ignoreAtoms, confId, - additionalOutput, customAtomInvariants, - customBondInvariants); + FingerprintFuncArguments args(fromAtoms.get(), ignoreAtoms.get(), confId, + additionalOutput, customAtomInvariants.get(), + customBondInvariants.get()); auto result = fpGen->getSparseFingerprint(mol, args); - delete fromAtoms; - delete ignoreAtoms; - return result.release(); } @@ -162,10 +156,10 @@ SparseIntVect *getCountFingerprint( python::object py_fromAtoms, python::object py_ignoreAtoms, const int confId, python::object py_atomInvs, python::object py_bondInvs, python::object py_additionalOutput) { - std::vector *fromAtoms = nullptr; - std::vector *ignoreAtoms = nullptr; - std::vector *customAtomInvariants = nullptr; - std::vector *customBondInvariants = nullptr; + std::unique_ptr> fromAtoms; + std::unique_ptr> ignoreAtoms; + std::unique_ptr> customAtomInvariants; + std::unique_ptr> customBondInvariants; convertPyArguments(py_fromAtoms, py_ignoreAtoms, py_atomInvs, py_bondInvs, fromAtoms, ignoreAtoms, customAtomInvariants, customBondInvariants); @@ -174,14 +168,11 @@ SparseIntVect *getCountFingerprint( additionalOutput = python::extract(py_additionalOutput); } - FingerprintFuncArguments args(fromAtoms, ignoreAtoms, confId, - additionalOutput, customAtomInvariants, - customBondInvariants); + FingerprintFuncArguments args(fromAtoms.get(), ignoreAtoms.get(), confId, + additionalOutput, customAtomInvariants.get(), + customBondInvariants.get()); auto result = fpGen->getCountFingerprint(mol, args); - delete fromAtoms; - delete ignoreAtoms; - return result.release(); } @@ -192,10 +183,10 @@ ExplicitBitVect *getFingerprint(const FingerprintGenerator *fpGen, python::object py_atomInvs, python::object py_bondInvs, python::object py_additionalOutput) { - std::vector *fromAtoms = nullptr; - std::vector *ignoreAtoms = nullptr; - std::vector *customAtomInvariants = nullptr; - std::vector *customBondInvariants = nullptr; + std::unique_ptr> fromAtoms; + std::unique_ptr> ignoreAtoms; + std::unique_ptr> customAtomInvariants; + std::unique_ptr> customBondInvariants; convertPyArguments(py_fromAtoms, py_ignoreAtoms, py_atomInvs, py_bondInvs, fromAtoms, ignoreAtoms, customAtomInvariants, customBondInvariants); @@ -204,14 +195,11 @@ ExplicitBitVect *getFingerprint(const FingerprintGenerator *fpGen, additionalOutput = python::extract(py_additionalOutput); } - FingerprintFuncArguments args(fromAtoms, ignoreAtoms, confId, - additionalOutput, customAtomInvariants, - customBondInvariants); + FingerprintFuncArguments args(fromAtoms.get(), ignoreAtoms.get(), confId, + additionalOutput, customAtomInvariants.get(), + customBondInvariants.get()); auto result = fpGen->getFingerprint(mol, args); - delete fromAtoms; - delete ignoreAtoms; - return result.release(); } diff --git a/Code/GraphMol/Wrap/MolOps.cpp b/Code/GraphMol/Wrap/MolOps.cpp index feed824b2..e9231c963 100644 --- a/Code/GraphMol/Wrap/MolOps.cpp +++ b/Code/GraphMol/Wrap/MolOps.cpp @@ -191,19 +191,19 @@ ROMol *renumberAtomsHelper(const ROMol &mol, python::object &pyNewOrder) { namespace { std::string getResidue(const ROMol &, const Atom *at) { auto monomerInfo = at->getMonomerInfo(); - if (!monomerInfo || monomerInfo->getMonomerType() != AtomMonomerInfo::PDBRESIDUE) { + if (!monomerInfo || + monomerInfo->getMonomerType() != AtomMonomerInfo::PDBRESIDUE) { return ""; } - return static_cast(monomerInfo) - ->getResidueName(); + return static_cast(monomerInfo)->getResidueName(); } std::string getChainId(const ROMol &, const Atom *at) { -auto monomerInfo = at->getMonomerInfo(); - if (!monomerInfo || monomerInfo->getMonomerType() != AtomMonomerInfo::PDBRESIDUE) { + auto monomerInfo = at->getMonomerInfo(); + if (!monomerInfo || + monomerInfo->getMonomerType() != AtomMonomerInfo::PDBRESIDUE) { return ""; } - return static_cast(monomerInfo) - ->getChainId(); + return static_cast(monomerInfo)->getChainId(); } } // namespace python::dict splitMolByPDBResidues(const ROMol &mol, python::object pyWhiteList, @@ -266,12 +266,10 @@ python::dict parseQueryDefFileHelper(python::object &input, bool standardize, parseQueryDefFile(get_filename(), queryDefs, standardize, delimiter, comment, nameColumn, smartsColumn); } else { - auto *sb = new streambuf(input); - std::istream *istr = new streambuf::istream(*sb); - parseQueryDefFile(istr, queryDefs, standardize, delimiter, comment, + std::unique_ptr sb(new streambuf(input)); + std::unique_ptr istr(new streambuf::istream(*sb)); + parseQueryDefFile(istr.get(), queryDefs, standardize, delimiter, comment, nameColumn, smartsColumn); - delete istr; - delete sb; } python::dict res; @@ -621,9 +619,9 @@ ExplicitBitVect *wrapLayeredFingerprint( python::object fromAtoms) { std::unique_ptr> lFromAtoms = pythonObjectToVect(fromAtoms, mol.getNumAtoms()); - std::vector *atomCountsV = nullptr; + std::unique_ptr> atomCountsV; if (atomCounts) { - atomCountsV = new std::vector; + atomCountsV.reset(new std::vector); unsigned int nAts = python::extract(atomCounts.attr("__len__")()); if (nAts < mol.getNumAtoms()) { @@ -637,14 +635,13 @@ ExplicitBitVect *wrapLayeredFingerprint( ExplicitBitVect *res; res = RDKit::LayeredFingerprintMol(mol, layerFlags, minPath, maxPath, fpSize, - atomCountsV, includeOnlyBits, + atomCountsV.get(), includeOnlyBits, branchedPaths, lFromAtoms.get()); if (atomCountsV) { for (unsigned int i = 0; i < atomCountsV->size(); ++i) { atomCounts[i] = (*atomCountsV)[i]; } - delete atomCountsV; } return res; diff --git a/Code/GraphMol/Wrap/PDBWriter.cpp b/Code/GraphMol/Wrap/PDBWriter.cpp index fea764977..3c6bc0715 100644 --- a/Code/GraphMol/Wrap/PDBWriter.cpp +++ b/Code/GraphMol/Wrap/PDBWriter.cpp @@ -25,9 +25,8 @@ namespace RDKit { using boost_adaptbx::python::streambuf; namespace { PDBWriter *getPDBWriter(python::object &fileobj, unsigned int flavor = 0) { - // FIX: minor leak here auto *sb = new streambuf(fileobj, 't'); - auto *ost = new streambuf::ostream(*sb); + auto *ost = new streambuf::ostream(sb); return new PDBWriter(ost, true, flavor); } } // namespace diff --git a/Code/GraphMol/Wrap/SDWriter.cpp b/Code/GraphMol/Wrap/SDWriter.cpp index 13aad87bd..88a92d859 100644 --- a/Code/GraphMol/Wrap/SDWriter.cpp +++ b/Code/GraphMol/Wrap/SDWriter.cpp @@ -25,9 +25,8 @@ using boost_adaptbx::python::streambuf; namespace RDKit { SDWriter *getSDWriter(python::object &fileobj) { - // FIX: minor leak here auto *sb = new streambuf(fileobj, 't'); - auto *ost = new streambuf::ostream(*sb); + auto *ost = new streambuf::ostream(sb); return new SDWriter(ost, true); } diff --git a/Code/GraphMol/Wrap/SmilesWriter.cpp b/Code/GraphMol/Wrap/SmilesWriter.cpp index 398779baa..28e8399d0 100644 --- a/Code/GraphMol/Wrap/SmilesWriter.cpp +++ b/Code/GraphMol/Wrap/SmilesWriter.cpp @@ -29,9 +29,8 @@ SmilesWriter *getSmilesWriter(python::object &fileobj, bool includeHeader = true, bool isomericSmiles = true, bool kekuleSmiles = false) { - // FIX: minor leak here auto *sb = new streambuf(fileobj, 't'); - auto *ost = new streambuf::ostream(*sb); + auto *ost = new streambuf::ostream(sb); return new SmilesWriter(ost, delimiter, nameHeader, includeHeader, true, isomericSmiles, kekuleSmiles); } diff --git a/Code/GraphMol/Wrap/TDTWriter.cpp b/Code/GraphMol/Wrap/TDTWriter.cpp index 92834822e..e6993728c 100644 --- a/Code/GraphMol/Wrap/TDTWriter.cpp +++ b/Code/GraphMol/Wrap/TDTWriter.cpp @@ -25,9 +25,8 @@ namespace python = boost::python; namespace RDKit { using boost_adaptbx::python::streambuf; TDTWriter *getTDTWriter(python::object &fileobj) { - // FIX: minor leak here auto *sb = new streambuf(fileobj, 't'); - auto *ost = new streambuf::ostream(*sb); + auto *ost = new streambuf::ostream(sb); return new TDTWriter(ost, true); } diff --git a/Code/ML/Cluster/Murtagh/Clustering.cpp b/Code/ML/Cluster/Murtagh/Clustering.cpp index 5f35d2a53..6969a6bf4 100644 --- a/Code/ML/Cluster/Murtagh/Clustering.cpp +++ b/Code/ML/Cluster/Murtagh/Clustering.cpp @@ -28,9 +28,9 @@ extern "C" void distdriver_(boost::int64_t *n, boost::int64_t *len, real *dists, // (thus drowning in the waves of f2c hate), we'll generate // the distance matrix on our own here and then call distdriver_ // -void clusterit(real *dataP, boost::int64_t n, boost::int64_t m, - boost::int64_t iopt, boost::int64_t *ia, boost::int64_t *ib, - real *crit) { +static void clusterit(real *dataP, boost::int64_t n, boost::int64_t m, + boost::int64_t iopt, boost::int64_t *ia, + boost::int64_t *ib, real *crit) { real *dists; boost::int64_t len; boost::int64_t pos = 0; @@ -54,6 +54,11 @@ void clusterit(real *dataP, boost::int64_t n, boost::int64_t m, free(dists); }; +static void capsule_cleanup(PyObject *capsule) { + void *ptr = PyCapsule_GetPointer(capsule, nullptr); + free(ptr); +} + static PyObject *Clustering_MurtaghCluster(python::object data, int nPts, int sz, int option) { PyArrayObject *dataContig; @@ -72,8 +77,13 @@ static PyObject *Clustering_MurtaghCluster(python::object data, int nPts, } ia = (boost::int64_t *)calloc(nPts, sizeof(boost::int64_t)); + auto ia_capsule = PyCapsule_New(ia, nullptr, capsule_cleanup); + ib = (boost::int64_t *)calloc(nPts, sizeof(boost::int64_t)); + auto ib_capsule = PyCapsule_New(ib, nullptr, capsule_cleanup); + crit = (real *)calloc(nPts, sizeof(real)); + auto crit_capsule = PyCapsule_New(crit, nullptr, capsule_cleanup); clusterit((real *)PyArray_DATA(dataContig), nPts, sz, option, ia, ib, crit); @@ -85,12 +95,15 @@ static PyObject *Clustering_MurtaghCluster(python::object data, int nPts, // Python will take care of it for us. // tmp = PyArray_SimpleNewFromData(1, dims, NPY_LONG, (void *)ia); + PyArray_SetBaseObject((PyArrayObject *)tmp, ia_capsule); PyTuple_SetItem(res, 0, (PyObject *)tmp); tmp = PyArray_SimpleNewFromData(1, dims, NPY_LONG, (void *)ib); + PyArray_SetBaseObject((PyArrayObject *)tmp, ib_capsule); PyTuple_SetItem(res, 1, (PyObject *)tmp); tmp = PyArray_SimpleNewFromData(1, dims, NPY_DOUBLE, (void *)crit); + PyArray_SetBaseObject((PyArrayObject *)tmp, crit_capsule); PyTuple_SetItem(res, 2, (PyObject *)tmp); return res; @@ -122,8 +135,14 @@ static PyObject *Clustering_MurtaghDistCluster(python::object data, int nPts, } ia = (boost::int64_t *)calloc(nPts, sizeof(boost::int64_t)); + auto ia_capsule = PyCapsule_New(ia, nullptr, capsule_cleanup); + ib = (boost::int64_t *)calloc(nPts, sizeof(boost::int64_t)); + auto ib_capsule = PyCapsule_New(ib, nullptr, capsule_cleanup); + crit = (real *)calloc(nPts, sizeof(real)); + auto crit_capsule = PyCapsule_New(crit, nullptr, capsule_cleanup); + distclusterit((real *)PyArray_DATA(dataContig), nPts, option, ia, ib, crit); dims[0] = nPts; @@ -134,12 +153,15 @@ static PyObject *Clustering_MurtaghDistCluster(python::object data, int nPts, // Python will take care of it for us. // tmp = PyArray_SimpleNewFromData(1, dims, NPY_LONG, (void *)ia); + PyArray_SetBaseObject((PyArrayObject *)tmp, ia_capsule); PyTuple_SetItem(res, 0, tmp); tmp = PyArray_SimpleNewFromData(1, dims, NPY_LONG, (void *)ib); + PyArray_SetBaseObject((PyArrayObject *)tmp, ib_capsule); PyTuple_SetItem(res, 1, tmp); tmp = PyArray_SimpleNewFromData(1, dims, NPY_DOUBLE, (void *)crit); + PyArray_SetBaseObject((PyArrayObject *)tmp, crit_capsule); PyTuple_SetItem(res, 2, tmp); return res; diff --git a/Code/RDBoost/python_streambuf.h b/Code/RDBoost/python_streambuf.h index ce923496e..d461c7c58 100644 --- a/Code/RDBoost/python_streambuf.h +++ b/Code/RDBoost/python_streambuf.h @@ -529,11 +529,20 @@ class streambuf : public std::basic_streambuf { exceptions(std::ios_base::badbit); } + // overload that takes ownership of the streambuf ptr + ostream(streambuf *buf) : std::ostream(buf), m_buf(buf) { + exceptions(std::ios_base::badbit); + } + ~ostream() override { if (this->good()) { this->flush(); } + delete m_buf; } + + private: + streambuf *m_buf = nullptr; }; };